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.

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

Pass native-routing-cidr to ENI CNI for route rules #10887

Merged
merged 1 commit into from May 1, 2020

Conversation

dctrwatson
Copy link
Contributor

When using a configured native-routing-cidr that encompasses the VPC subnet, due to using VPC Peering or similar, the Linux routing rules need to be configured to properly route back out the ENI which the IP is attached when masquerade is disabled.

This PR adds native-routing-cidr to the IPAM CRD AllocationResult. In the ENI cilium-cni plugin it coalesces all returned CIDRs into the minimum set needed for the Linux routing rules.

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

@dctrwatson dctrwatson requested a review from a team as a code owner April 7, 2020 23:58
@dctrwatson dctrwatson requested a review from a team April 7, 2020 23:58
@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
@aanm aanm added area/eni Impacts ENI based IPAM. sig/ipam IP address management, including cloud IPAM pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 9, 2020
@aanm aanm requested a review from tgraf April 9, 2020 18:32
@aanm
Copy link
Member

aanm commented Apr 9, 2020

test-me-please

@coveralls
Copy link

coveralls commented Apr 9, 2020

Coverage Status

Coverage decreased (-0.01%) to 44.571% when pulling 4b47aa7 on planetscale:jw-route-rule-for-native into 4b865d5 on cilium:master.

@aanm
Copy link
Member

aanm commented Apr 15, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Apr 15, 2020

PTAL @tgraf

Copy link
Member

@tgraf tgraf left a comment

Choose a reason for hiding this comment

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

I was puzzled at first but assuming that the native-routing-cidr equals the CIDR of the VPC then this is correct.

@aanm
Copy link
Member

aanm commented Apr 27, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Apr 27, 2020

re-running CI since jenkins build was gone

@qmonnet
Copy link
Member

qmonnet commented Apr 28, 2020

test-focus K8sServicesTest External services To Services first endpoint creation

@qmonnet qmonnet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 28, 2020
@qmonnet
Copy link
Member

qmonnet commented Apr 28, 2020

There's a conflict in plugins/cilium-cni/eni.go, could you rebase this PR on the current master branch please?

@dctrwatson
Copy link
Contributor Author

There's a conflict in plugins/cilium-cni/eni.go, could you rebase this PR on the current master branch please?

Rebased.

🎉 smaller change now 😀

@aanm aanm removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 29, 2020
@aanm
Copy link
Member

aanm commented Apr 29, 2020

test-me-please

@qmonnet
Copy link
Member

qmonnet commented Apr 29, 2020

Thanks for the rebase, but it looks like there is a build error now 😢:

08:43:35  # github.com/cilium/cilium/plugins/cilium-cni
[2020-04-29T07:43:35.062Z] ./eni.go:40:11: cannot assign []*net.IPNet to cidrs (type []net.IPNet) in multiple assignment
[2020-04-29T07:43:35.062Z] ./eni.go:40:29: cannot use cidrs (type []net.IPNet) as type []*net.IPNet in argument to ip.CoalesceCIDRs
[2020-04-29T07:43:35.062Z] make[1]: *** [cilium-cni] Error 2
[2020-04-29T07:43:35.062Z] Makefile:13: recipe for target 'cilium-cni' failed
08:43:35  make[1]: Leaving directory '/go/src/github.com/cilium/cilium/plugins/cilium-cni'
08:43:35  make: *** [build-container] Error 2
[2020-04-29T07:43:35.062Z] Makefile:76: recipe for target 'build-container' failed

Please have a look.

@dctrwatson
Copy link
Contributor Author

but it looks like there is a build error now

Sorry about that :(

Just pushed up a fix.

@qmonnet
Copy link
Member

qmonnet commented Apr 30, 2020

test-me-please

@qmonnet
Copy link
Member

qmonnet commented Apr 30, 2020

Timed out.

restart-ginkgo

@qmonnet
Copy link
Member

qmonnet commented Apr 30, 2020

Sorry, one last thing. Could you please squash your fix commit with the one that failed to compile, before we merge? This is so that it does not break git bisect workflows by introducing a broken build in the git history.

@dctrwatson
Copy link
Contributor Author

Could you please squash your fix commit with the one that failed to compile, before we merge?

Squashed and force pushed.

@qmonnet
Copy link
Member

qmonnet commented Apr 30, 2020

Thanks, but I'm afraid you squashed with the wrong commit :( I tried building with your first commit applied only, and it fails. Would you mind double-checking, please?

@dctrwatson
Copy link
Contributor Author

Thanks, but I'm afraid you squashed with the wrong commit :( I tried building with your first commit applied only, and it fails. Would you mind double-checking, please?

Ohhh, I'll need to squash all 3 commits then

Signed-off-by: John Watson <johnw@planetscale.com>
@dctrwatson
Copy link
Contributor Author

Squashed the original change with the rebase fix commits. confirmed cilium-cni does build.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

All good this time, thank you!

@qmonnet qmonnet merged commit 0b203d4 into cilium:master May 1, 2020
1.8.0 automation moved this from In progress to Merged May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/ipam IP address management, including cloud IPAM
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants