Skip to content

Fix native routing cidr missing flag in daemon#12180

Merged
aanm merged 3 commits intocilium:masterfrom
aanm:pr/fix-native-routing-cidr
Jun 18, 2020
Merged

Fix native routing cidr missing flag in daemon#12180
aanm merged 3 commits intocilium:masterfrom
aanm:pr/fix-native-routing-cidr

Conversation

@aanm
Copy link
Copy Markdown
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
@aanm aanm force-pushed the pr/fix-native-routing-cidr branch from 5d02782 to 153a639 Compare June 18, 2020 15:10
@aanm
Copy link
Copy Markdown
Member Author

aanm commented Jun 18, 2020

test-me-please

@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
Copy Markdown
Member Author

aanm commented Jun 18, 2020

test-me-please

@aanm
Copy link
Copy Markdown
Member Author

aanm commented Jun 18, 2020

test-gke

brb
brb reviewed Jun 18, 2020
View reviewed changes
Comment thread daemon/cmd/daemon_main.go Outdated
Copy link
Copy Markdown
Member

@brb brb Jun 18, 2020

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
Copy Markdown
Contributor

@tgraf tgraf Jun 18, 2020

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
Copy Markdown
Member

joestringer commented Jun 18, 2020

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/

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jun 18, 2020

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
Copy Markdown
Member Author

aanm commented Jun 18, 2020 •
edited
Loading

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

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 18, 2020

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
Copy Markdown
Member

christarazi commented Jun 18, 2020

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
Copy Markdown
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
Copy Markdown
Member Author

aanm commented Jun 18, 2020

retest-4.19

@aanm
Copy link
Copy Markdown
Member Author

aanm commented Jun 18, 2020

retest-runtime

@aanm aanm merged commit fc94aa1 into cilium:master Jun 18, 2020
@aanm aanm deleted the pr/fix-native-routing-cidr branch June 18, 2020 18:59
@jrajahalme jrajahalme mentioned this pull request Jun 18, 2020
Merged
This was referenced Jun 30, 2020
Merged
Merged
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

@pchaigno pchaigno pchaigno approved these changes

+1 more reviewer

@tgraf tgraf tgraf approved these changes

Reviewers whose approvals may not affect merge requirements

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

None yet

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