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

Add possibility to configure native-routing-cidr in helm chart #11132

Merged
merged 2 commits into from Apr 27, 2020
Merged

Add possibility to configure native-routing-cidr in helm chart #11132

merged 2 commits into from Apr 27, 2020

Conversation

zbindenren
Copy link
Contributor

With this change it is possible to configure native-routing-cidr option in helm chart. Additionally it makes sure, that this value is set when:

  • masquerade: true
  • ... and tunnel: disabled
  • ... and NOT ipam: eni

Fixes: #11096 ("K8s Network Policy not working with configured auto-direct-node-routes")

Add possibility to configure native-routing-cidr in helm chart.

Signed-off-by: Rene Zbinden rene.zbinden@postfinance.ch

@zbindenren zbindenren requested a review from a team April 24, 2020 08:47
@maintainer-s-little-helper
Copy link

Commits 1f23507b5e18926dcfbbaefc4e83da0bc61d2014, 6a5f52db27768c2a87ca6969a32c6a39c1fa43fb do 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
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note labels Apr 24, 2020
@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 24, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 24, 2020
@pchaigno pchaigno added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 24, 2020
@pchaigno pchaigno added the area/helm Impacts helm charts and user deployment experience label Apr 24, 2020
@coveralls
Copy link

coveralls commented Apr 24, 2020 •
edited

Coverage Status

Coverage increased (+0.03%) to 44.801% when pulling 8124abe on zbindenren:master into 86045f7 on cilium:master.

aanm
aanm approved these changes Apr 24, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

💯

@aanm aanm requested a review from tgraf April 24, 2020 12:04
@aanm
Copy link
Member

aanm commented Apr 24, 2020

Requested review of @tgraf since he is assigned for #11096

@aanm
Copy link
Member

aanm commented Apr 24, 2020

test-me-please

tgraf
tgraf requested changes Apr 24, 2020
View reviewed changes
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.

LGTM. One small issue left.

pkg/option/config.go Outdated
func (c *DaemonConfig) checkIPv4NativeRoutingCIDR() error {
if c.IPv4NativeRoutingCIDR() == nil && c.Masquerade && c.Tunnel == TunnelDisabled && c.IPAMMode() != IPAMENI {
return fmt.Errorf("native routing cidr must be configured with option --%s in combination with --%s --%s=%s --%s=%s",
Masquerade, IPv4NativeRoutingCIDR, TunnelName, c.Tunnel, IPAM, c.IPAMMode())
Copy link
Member

Choose a reason for hiding this comment

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

I think the argument order is still off here. I think you want IPv4NativeRoutingCIDR for the first %s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are of course right. Fixed.

@maintainer-s-little-helper
Copy link

Commit 902f9214921fa4b0d5178343c972843280aef9e1 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 24, 2020
@tgraf
Copy link
Member

tgraf commented Apr 24, 2020

@zbindenren Change looks good now. Can you rebase to make sure all commits are signed off?

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 24, 2020
@zbindenren
Copy link
Contributor Author

Rebase done.

@christarazi
Copy link
Member

test-me-please

@maintainer-s-little-helper
Copy link

Commit a6b2684b2a68ac361150af01268bb9425bbdc50a 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 27, 2020
@zbindenren
helm: add possibility to configure native-routing-cidr
5de8b14 
Signed-off-by: Rene Zbinden <rene.zbinden@postfinance.ch>
@zbindenren
daemon: validate IPv4NativeRoutingCIDR value in DaemonConfig
8124abe 
Signed-off-by: Rene Zbinden <rene.zbinden@postfinance.ch>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 27, 2020
@zbindenren
Copy link
Contributor Author

test-me-please

1 similar comment
@aanm
Copy link
Member

aanm commented Apr 27, 2020

test-me-please

@zbindenren
Copy link
Contributor Author

zbindenren commented Apr 27, 2020 •
edited

@aanm I am not sure what those failures have to do with my changes? Should I update and rebase again?

@aanm aanm merged commit e7d4f5c into cilium:master Apr 27, 2020
1.8.0 automation moved this from In progress to Merged Apr 27, 2020
@aanm
Copy link
Member

aanm commented Apr 27, 2020

thanks for the PR @zbindenren !

@christarazi christarazi mentioned this pull request Jul 1, 2020
Merged
@pchaigno pchaigno mentioned this pull request Jul 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@tgraf tgraf tgraf approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

K8s Network Policy not working with configured auto-direct-node-routes
6 participants
@zbindenren @coveralls @aanm @tgraf @christarazi @pchaigno