Validate when Cilium is in ENI mode that IPv4 is enabled by soumynathan · Pull Request #11328 · cilium/cilium

soumynathan · 2020-05-04T22:46:13Z

The patch checks the Cilium ENI mode and IPv4 when ENI functions
are called.

Fixes: #11272
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

maintainer-s-little-helper · 2020-05-04T22:46:15Z

Please set the appropriate release note label.

coveralls · 2020-05-04T23:08:18Z

Coverage decreased (-0.02%) to 37.849% when pulling f62a4a0 on soumynathan:check-ipv4-with-eni-mode into 78739a1 on cilium:master.

aanm · 2020-05-05T13:54:00Z

if you invert this if statement this function can return earlier and we can avoid touching the remaining code of this function.

aanm · 2020-05-05T13:54:19Z

if you invert this if statement this function can return earlier and we can avoid touching the remaining code of this function.

christarazi · 2020-05-06T05:00:16Z

test-me-please

Edit: vagrant failure https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/19499/

christarazi

Thanks for the PR! Overall, I think the changes are good. There's currently this PR pending which should be merging pretty soon. I'd like to get that in before merging this. #11208

I am wondering though if just validating whether we have option.Config.EnableIPv4 and option.Config.IPAM == option.IPAMENI at the daemon level is enough. We could just have it be fatal if the above condition does not hold. 🤔 /cc @aanm

@joestringer I recall we had a conversation about this in another PR. Any thoughts?

christarazi · 2020-05-06T05:13:06Z

restart-ginkgo

Edit: weird flake with vagrant index

22:22:10  Pruning vagrant images
22:22:11  The following boxes will be kept...
22:22:11  cilium/ubuntu      (virtualbox, 170)
22:22:11  cilium/ubuntu-4-19 (virtualbox, 12)
22:22:11  cilium/ubuntu-next (virtualbox, 53)
22:22:11  
22:22:11  Checking for older boxes...
22:22:11  The machine index which stores all required information about
22:22:11  running Vagrant environments has become corrupt. This is usually
22:22:11  caused by external tampering of the Vagrant data folder.
22:22:11  
22:22:11  Vagrant cannot manage any Vagrant environments if the index is
22:22:11  corrupt. Please attempt to manually correct it. If you are unable
22:22:11  to manually correct it, then remove the data file at the path below.
22:22:11  This will leave all existing Vagrant environments "orphaned" and
22:22:11  they'll have to be destroyed manually.
22:22:11  
22:22:11  Path: /root/.vagrant.d/data/machine-index/index

soumynathan · 2020-05-06T06:07:03Z

Thanks for the PR! Overall, I think the changes are good. There's currently this PR pending which should be merging pretty soon. I'd like to get that in before merging this. #11208

I am wondering though if just validating whether we have option.Config.EnableIPv4 and option.Config.IPAM == option.IPAMENI at the daemon level is enough. We could just have it be fatal if the above condition does not hold. /cc @aanm

@joestringer I recall we had a conversation about this in another PR. Any thoughts?

@christarazi @joestringer At the daemon level there is already a check between IPAMMode and IPv6 and throws an error if IPv6 is configured along with IPAMMode, so I think indirectly it is testing for the IPv4 with IPAMMode there. But there was comment in this bug that said that we are not validating in the calling function. Let me know your thoughts.

Also I had added couple of comments in two files where we can split. Let me if that is required.

christarazi · 2020-05-06T19:37:17Z

Same as the other similar spot.

christarazi · 2020-05-06T19:40:35Z

restart-ginkgo

joestringer · 2020-05-06T21:46:40Z

@christarazi @joestringer At the daemon level there is already a check between IPAMMode and IPv6 and throws an error if IPv6 is configured along with IPAMMode, so I think indirectly it is testing for the IPv4 with IPAMMode there. But there was comment in this bug that said that we are not validating in the calling function. Let me know your thoughts.

If we're already validating the commandline arguments to ensure that this case won't be hit, then I think we're satisfying the main concern here. It's slightly nicer to make sure that this errors out if the wrong address type is specified just to future-proof a bit but I wouldn't expect commandline argument combinations to be validated this deep; it should be occurring much earlier in the initial daemon configuration checks.

christarazi

Some minor nits. There also seems to be a conflict, please rebase.

christarazi

Looking good, a few minor comments. Also, please add test coverage now that there's an extra case for both functions.

christarazi

Just a minor change and we should be good to go!

The patch checks the Cilium ENI mode and IPv4 when ENI functions are called. Fixes: cilium#11272 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

christarazi

🎉

christarazi · 2020-05-08T22:14:39Z

test-me-please

christarazi · 2020-05-10T23:14:48Z

Added backport label as I'd like to backport this to 1.7 since it'll be a branch we support for a while and this PR has changed code which was recently backported, so it'll be good to keep the divergence to a minimum.

soumynathan requested a review from a team May 4, 2020 22:46

maintainer-s-little-helper Bot added the dont-merge/needs-release-note label May 4, 2020

aanm requested changes May 5, 2020

View reviewed changes

aanm requested a review from christarazi May 5, 2020 13:54

aanm added area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact. labels May 5, 2020

maintainer-s-little-helper Bot removed the dont-merge/needs-release-note label May 5, 2020

soumynathan force-pushed the check-ipv4-with-eni-mode branch from d4a6ad0 to cdcd948 Compare May 6, 2020 00:25

christarazi reviewed May 6, 2020

View reviewed changes

aanm approved these changes May 6, 2020

View reviewed changes

aanm requested a review from christarazi May 6, 2020 09:47

aanm added the conflicts-with-pr label May 6, 2020

christarazi requested changes May 6, 2020

View reviewed changes

christarazi added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed conflicts-with-pr labels May 7, 2020

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 7, 2020

christarazi mentioned this pull request May 7, 2020

ready-to-merge label is incorrectly applied even though needs-rebase has been set cilium/github-actions#7

Open

christarazi removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 7, 2020

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 7, 2020

soumynathan force-pushed the check-ipv4-with-eni-mode branch 2 times, most recently from 2d4f5c5 to cb84e53 Compare May 7, 2020 05:30

christarazi requested changes May 7, 2020

View reviewed changes

Comment thread pkg/aws/eni/routing/routing.go Outdated

Comment thread pkg/aws/eni/routing/routing.go Outdated

Comment thread pkg/aws/eni/routing/routing.go Outdated

Comment thread pkg/aws/eni/routing/routing.go Outdated

christarazi removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 7, 2020

soumynathan force-pushed the check-ipv4-with-eni-mode branch 3 times, most recently from 6d32e8f to e99a9e2 Compare May 7, 2020 19:06

christarazi requested changes May 7, 2020

View reviewed changes

Comment thread pkg/aws/eni/routing/routing.go Outdated

Comment thread pkg/aws/eni/routing/routing.go Outdated

Comment thread pkg/aws/eni/routing/routing.go Outdated

soumynathan force-pushed the check-ipv4-with-eni-mode branch 3 times, most recently from 9c0e709 to e69fa05 Compare May 8, 2020 18:55

christarazi requested changes May 8, 2020

View reviewed changes

Comment thread pkg/aws/eni/routing/routing_test.go Outdated

Validate when Cilium is in ENI mode that IPv4 is enabled

f62a4a0

The patch checks the Cilium ENI mode and IPv4 when ENI functions are called. Fixes: cilium#11272 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>

soumynathan force-pushed the check-ipv4-with-eni-mode branch from e69fa05 to f62a4a0 Compare May 8, 2020 21:49

christarazi approved these changes May 8, 2020

View reviewed changes

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2020

christarazi added needs-backport/1.7 and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels May 10, 2020

tgraf merged commit 7ae916a into cilium:master May 11, 2020

tklauser mentioned this pull request May 11, 2020

cilium: rename aws/eni/routing to pkg/datapath/linux/routing #11452

Merged

christarazi mentioned this pull request May 11, 2020

v1.7 backports 2020-05-08 #11441

Merged

christarazi added backport-pending/1.7 and removed needs-backport/1.7 labels May 11, 2020

aanm added backport-done/1.7 and removed backport-pending/1.7 labels May 13, 2020

Conversation

soumynathan commented May 4, 2020

Uh oh!

maintainer-s-little-helper Bot commented May 4, 2020

Uh oh!

coveralls commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm May 5, 2020

Choose a reason for hiding this comment

Uh oh!

soumynathan May 5, 2020

Choose a reason for hiding this comment

Uh oh!

aanm May 5, 2020

Choose a reason for hiding this comment

Uh oh!

soumynathan May 5, 2020

Choose a reason for hiding this comment

Uh oh!

christarazi commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christarazi commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soumynathan commented May 6, 2020

Uh oh!

Uh oh!

Uh oh!

christarazi May 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christarazi commented May 6, 2020

Uh oh!

joestringer commented May 6, 2020

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi commented May 8, 2020

Uh oh!

christarazi commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented May 4, 2020 •

edited

Loading

christarazi commented May 6, 2020 •

edited

Loading

christarazi left a comment •

edited

Loading

christarazi commented May 6, 2020 •

edited

Loading

christarazi commented May 10, 2020 •

edited

Loading