Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg/ipam: Don't let ENI IPAM override native-routing-cidr #10886

Merged
merged 2 commits into from Apr 29, 2020
Merged

pkg/ipam: Don't let ENI IPAM override native-routing-cidr #10886

merged 2 commits into from Apr 29, 2020

Conversation

dctrwatson
Copy link
Contributor

When using VPC Peering or similar, the L2 domain can be larger than the VPC CIDR from the ec2 instance's primary ENI.

The cilium agent has the --native-routing-cidr flag which can be used to manually configure this. However, when using ENI for IPAM, it always overwrites the configured value.

This PR changes this behavior, deferring to what the configured value for native-routing-cidr, and verifies that the native-routing-cidr is valid by checking that the VPC CIDR is a subnet of it.

Signed-off-by: John Watson johnw@planetscale.com

@dctrwatson
Don't let ENI IPAM override native-routing-cidr
7972c69 
Signed-off-by: John Watson <johnw@planetscale.com>
@dctrwatson dctrwatson requested a review from a team as a code owner April 7, 2020 23:44
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 7, 2020
@tgraf tgraf self-requested a review April 8, 2020 18:13
@aanm aanm added area/eni Impacts ENI based IPAM. sig/ipam IP address management, including cloud IPAM pending-review release-note/misc This PR makes changes that have no direct user impact. labels Apr 9, 2020
@aanm
Copy link
Member

aanm commented Apr 9, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Apr 9, 2020

@dctrwatson the CI failed:

make govet
make[1]: Entering directory `/home/travis/gopath/src/github.com/cilium/cilium'
GO111MODULE=off go vet \
    ./api/... \
    ./bugtool/... \
    ./cilium/... \
    ./cilium-health/... \
    ./common/... \
    ./daemon/... \
    ./operator/... \
    ./pkg/... \
    ./plugins/... \
    ./proxylib/... \
    ./test/. \
    ./test/config/... \
    ./test/ginkgo-ext/... \
    ./test/helpers/... \
    ./test/runtime/... \
    ./test/k8sT/... \
    ./tools/...
# github.com/cilium/cilium/pkg/ipam
pkg/ipam/crd.go:245:28: n.conf.IPv4NativeRoutingCIDR undefined (type Configuration has no field or method IPv4NativeRoutingCIDR)
# github.com/cilium/cilium/pkg/ipam
vet: pkg/ipam/crd.go:245:29: n.conf.IPv4NativeRoutingCIDR undefined (type Configuration has no field or method IPv4NativeRoutingCIDR)
make[1]: *** [govet] Error 2
make[1]: Leaving directory `/home/travis/gopath/src/github.com/cilium/cilium'
make: *** [unit-tests] Error 2
The command "./.travis/build.sh" exited with 2.

@dctrwatson
Copy link
Contributor Author

@dctrwatson the CI failed:

Sorry. I wrote this against the v1.7 branch originally. Let me fix it up.

@maintainer-s-little-helper
Copy link

Commit d7feaab93260872a8969246ee5b7eb46b87bfca5 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 9, 2020
@dctrwatson
Finish forward porting and double-check build+unit test
1018a4a 
Signed-off-by: John Watson <johnw@planetscale.com>
@maintainer-s-little-helper
Copy link

Commit d7feaab93260872a8969246ee5b7eb46b87bfca5 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 46.601% when pulling 1018a4a on planetscale:jw-override-vpc-cir into 7e5f30d on cilium:master.

@aanm aanm removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 16, 2020
@aanm
Copy link
Member

aanm commented Apr 16, 2020

test-me-please

1 similar comment
@aanm
Copy link
Member

aanm commented Apr 29, 2020

test-me-please

tgraf
tgraf approved these changes Apr 29, 2020
View reviewed changes
pkg/ipam/crd.go

ranges4, _ := ip.CoalesceCIDRs([]*net.IPNet{nativeCIDR.IPNet, vpcCIDR.IPNet})
if len(ranges4) != 1 {
log.WithFields(logFields).Fatal("Native routing CIDR does not contain VPC CIDR.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this Fatal and whether it could come as a surprise but I think it's fine. It seems unlikely that the VPC CIDR changes all of a sudden. It seems more likely that a user reconfigures the NativeRoutingCIDR to no longer include the VPC CIDR in which case the Fatal is justified.

@qmonnet qmonnet merged commit aeb5bcc into cilium:master Apr 29, 2020
1.8.0 automation moved this from In progress to Merged Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgraf tgraf tgraf approved these changes

Assignees
No one assigned
Labels
area/eni Impacts ENI based IPAM. release-note/misc This PR makes changes that have no direct user impact. sig/ipam IP address management, including cloud IPAM
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dctrwatson @aanm @coveralls @tgraf @qmonnet