Skip to content

nodeinit: only bypass IP-MASQ chain if Cilium manages masquerade#12952

Merged
joestringer merged 3 commits intocilium:masterfrom
planetscale:gke-disable-default-snat
Aug 26, 2020
Merged

nodeinit: only bypass IP-MASQ chain if Cilium manages masquerade#12952
joestringer merged 3 commits intocilium:masterfrom
planetscale:gke-disable-default-snat

Conversation

@dctrwatson
Copy link
Copy Markdown
Contributor

@dctrwatson dctrwatson commented Aug 21, 2020

#11782 introduced unexpected behavior of outright disabling ip-masquerade-agent functionality in GKE clusters with Cilium installed.

See: #11782 (comment)

Instead of always bypassing the IP-MASQ chain managed by GKE nodeinit and the ip-masquerade-agent, only bypass it if the user wants Cilium to manage masquerading (global.masquerade=true) or if they want to explicitly disable masquerade (global.gke.disableDefaultSnat)

@dctrwatson dctrwatson requested review from a team as code owners August 21, 2020 22:10
@dctrwatson dctrwatson requested a review from a team August 21, 2020 22:10
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Aug 21, 2020

Commit 721dd07d3423b7f9fa6c8aad25e3e4f7052acdf7 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 dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 21, 2020
@dctrwatson
nodeinit: only bypass IP-MASQ if Cilium manages masquerade
9f7a7fa 
Signed-off-by: John Watson <johnw@planetscale.com>
@dctrwatson dctrwatson force-pushed the gke-disable-default-snat branch from 721dd07 to 9f7a7fa Compare August 21, 2020 22:10
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Aug 21, 2020

Commit 721dd07d3423b7f9fa6c8aad25e3e4f7052acdf7 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

@dctrwatson dctrwatson mentioned this pull request Aug 21, 2020
Merged
joestringer
joestringer approved these changes Aug 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for following up.

I'll /cc @David0922 @Weil0ng to double-check that this satisfies their concerns.

@joestringer joestringer added priority/release-blocker release-note/misc This PR makes changes that have no direct user impact. labels Aug 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 21, 2020
@joestringer joestringer added integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Aug 21, 2020
@joestringer
Copy link
Copy Markdown
Member

joestringer commented Aug 21, 2020

The bot didn't seem to detect that sign-off was added, it seems right to me. So I dropped that label.

sayboras
sayboras reviewed Aug 24, 2020
View reviewed changes
Comment thread install/kubernetes/cilium/charts/nodeinit/templates/daemonset.yaml Outdated
{{- end }}

{{- if .Values.global.gke.enabled }}
{{- if (or (and .Values.global.gke.enabled .Values.global.masquerade) (and .Values.global.gke.enabled .Values.global.gke.disableDefaultSnat))}}
Copy link
Copy Markdown
Member

@sayboras sayboras Aug 24, 2020

Choose a reason for hiding this comment

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

nit: can be as per below, feel free to ignore

Suggested change
{{- if (or (and .Values.global.gke.enabled .Values.global.masquerade) (and .Values.global.gke.enabled .Values.global.gke.disableDefaultSnat))}}
{{- if (and ( .Values.global.gke.enabled (or .Values.global.masquerade .Values.global.gke.disableDefaultSnat)))}}

Copy link
Copy Markdown
Contributor Author

@dctrwatson dctrwatson Aug 25, 2020

Choose a reason for hiding this comment

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

done

sayboras
sayboras approved these changes Aug 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@sayboras sayboras requested a review from a team August 24, 2020 10:21
@jrfastab
Copy link
Copy Markdown
Contributor

jrfastab commented Aug 24, 2020

LGTM. Nice catch.

@dctrwatson
Simplify gate expression
bb0cdd5 
Signed-off-by: John Watson <johnw@planetscale.com>
@dctrwatson
Fix expression per lint error
9ae8796 
Signed-off-by: John Watson <johnw@planetscale.com>
@joestringer
Copy link
Copy Markdown
Member

joestringer commented Aug 26, 2020

test-gke

@joestringer joestringer merged commit 7457ce6 into cilium:master Aug 26, 2020
@kaworu kaworu mentioned this pull request Aug 27, 2020
Merged
@EronWright EronWright mentioned this pull request Apr 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joestringer joestringer joestringer approved these changes

@sayboras sayboras sayboras 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

integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. 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.

7 participants

@dctrwatson @joestringer @jrfastab @tgraf @sayboras @kaworu @pchaigno