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

Fix native routing cidr missing flag in daemon #12180

Merged
merged 3 commits into from Jun 18, 2020
Merged

Fix native routing cidr missing flag in daemon #12180

merged 3 commits into from Jun 18, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Jun 18, 2020

Fixes: #12130

@aanm aanm added kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. labels Jun 18, 2020
@aanm aanm requested a review from a team June 18, 2020 15:10
@aanm aanm requested a review from a team as a code owner June 18, 2020 15:10
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 18, 2020
@aanm aanm force-pushed the pr/fix-native-routing-cidr branch from 5d02782 to 153a639 Compare June 18, 2020 15:10
@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 18, 2020
@aanm aanm force-pushed the pr/fix-native-routing-cidr branch from 153a639 to 9dfa106 Compare June 18, 2020 15:28
@aanm aanm requested a review from a team as a code owner June 18, 2020 15:28
@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

test-gke

brb
brb reviewed Jun 18, 2020
View reviewed changes
daemon/cmd/daemon_main.go
@@ -542,6 +542,9 @@ func init() {
flags.Bool(option.EnableHostFirewall, false, "Enable host network policies")
option.BindEnv(option.EnableHostFirewall)

flags.String(option.IPv4NativeRoutingCIDR, "", "Allows to explicitly specify the CIDR for native routing. This value corresponds to the configured cluster-cidr.")
Copy link
Member

Choose a reason for hiding this comment

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

Not related to your PR, but why we didn't call the flag --cluster-cidr?

Copy link
Member

Choose a reason for hiding this comment

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

Because it is not the cluster CIDR. The native routing space might be much larger, spanning multiple clusters or even non-k8s space.

@joestringer
Copy link
Member

Ah I noticed this on v1.7 and just assumed that it was a bad backport.. we're depending on this helm option in the v1.7 GKE GSG at the moment as well, might be a backport candidate:

https://docs.cilium.io/en/v1.7/gettingstarted/k8s-install-gke/

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.6 Jun 18, 2020
@pchaigno
Copy link
Member

Ah I noticed this on v1.7 and just assumed that it was a bad backport.. we're depending on this helm option in the v1.7 GKE GSG at the moment as well, might be a backport candidate:
https://docs.cilium.io/en/v1.7/gettingstarted/k8s-install-gke/

That guide has global.native-routing-cidr; shouldn't it be global.nativeRoutingCidr?

@aanm
Copy link
Member Author

aanm commented Jun 18, 2020 •
edited

runtime failed because of missing flag in the --help section. I'll repush as soon all other CI tests are completed

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 37.103% when pulling 9dfa1068cc6a198adf197c8a463352b49e496c9e on aanm:pr/fix-native-routing-cidr into a1cc34d on cilium:master.

aanm added 3 commits June 18, 2020 18:45
@aanm
pkg/option: return error in case validation of IPv4NativeRoutingCIDR …
2258ff3 
…fail

Fixes: e7d4f5c ("daemon: validate IPv4NativeRoutingCIDR value in DaemonConfig")
Signed-off-by: André Martins <andre@cilium.io>
@aanm
daemon: add native-routing-cidr as part of the daemon flags
2e59c93 
Fixes: c496e25 ("eni: Support masquerading")
Signed-off-by: André Martins <andre@cilium.io>
@aanm
test: set nativeRoutingCIDR
ca64502 
As we are currently running our CI with a CIDR from the Cilium-Operator,
which is "10.0.0.0/16", we should set it as part of our
'nativeRoutingCIDR'.

Fixes: ace902d ("helm: Enable BPF masquerading by default")
Signed-off-by: André Martins <andre@cilium.io>
@christarazi
Copy link
Member

Ah I noticed this on v1.7 and just assumed that it was a bad backport.. we're depending on this helm option in the v1.7 GKE GSG at the moment as well, might be a backport candidate:
https://docs.cilium.io/en/v1.7/gettingstarted/k8s-install-gke/

That guide has global.native-routing-cidr; shouldn't it be global.nativeRoutingCidr?

@pchaigno I believe it's being fixed in #12087 (comment)

@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

k8s 4.19 failed with the flake that is going to be fixed in #12179
runtime 4.9 failed because of the missing help option
travis failed because of the missing help option
k8s-1.18: green
k8s-1.11: green

will re push with the flag set, we only need to retest-4.19

@aanm aanm force-pushed the pr/fix-native-routing-cidr branch from 9dfa106 to ca64502 Compare June 18, 2020 17:45
@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

retest-4.19

@aanm
Copy link
Member Author

aanm commented Jun 18, 2020

retest-runtime

@aanm aanm merged commit fc94aa1 into cilium:master Jun 18, 2020
1.8.0 automation moved this from In progress to Merged Jun 18, 2020
@aanm aanm deleted the pr/fix-native-routing-cidr branch June 18, 2020 18:59
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 18, 2020
@jrajahalme jrajahalme mentioned this pull request Jun 18, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 18, 2020
This was referenced Jun 30, 2020
Merged
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.6 Jul 1, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.6 Jul 1, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.6 Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb left review comments

@tklauser tklauser tklauser approved these changes

@tgraf tgraf tgraf approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.7.6
Backport done to v1.7
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: K8sServicesTest Bookinfo Demo Tests bookinfo demo
9 participants
@aanm @joestringer @pchaigno @coveralls @christarazi @brb @tklauser @tgraf @borkmann