8000 nonMasqueradeCIDRs configuration added to the helm chart by cyclinder · Pull Request #20137 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

nonMasqueradeCIDRs configuration added to the helm chart #20137

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
Merged
merged 1 commit into from
Aug 22, 2022
Merged

nonMasqueradeCIDRs configuration added to the helm chart #20137

merged 1 commit into from
Aug 22, 2022

Conversation

cyclinder
Copy link
Contributor
@cyclinder cyclinder commented Jun 9, 2022 •
edited
Loading

Signed-off-by: cyclinder qifeng.guo@daocloud.io

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #20051

add  `nonMasqueradeCIDRs` configuration to the ipMasqAgent section in Helm Chart values.

@cyclinder cyclinder requested a review from a team June 9, 2022 14:27
@cyclinder cyclinder requested a review from a team as a code owner June 9, 2022 14:27
@cyclinder cyclinder requested a review from sayboras June 9, 2022 14:27
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 9, 2022
@cyclinder cyclinder mentioned this pull request Jun 9, 2022
Closed
brb
brb reviewed Jun 10, 2022
View reviewed changes
@cyclinder cyclinder force-pushed the ipMasqAgent_helm branch 3 times, most recently from d01e143 to 7cf5e5e Compare June 10, 2022 12:56
joestringer
joestringer requested changes Jun 13, 2022
View reviewed changes
@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Jun 13, 2022
@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 Jun 13, 2022
joestringer
joestringer reviewed Jun 15, 2022
View reviewed changes
@cyclinder cyclinder force-pushed the ipMasqAgent_helm branch 2 times, most recently from bd89b2a to ff20223 Compare June 15, 2022 14:01
@joestringer
Copy link
Member
joestringer commented Jun 16, 2022 •
edited
Loading

I'm a bit confused what the core proposal is here. I suspect we just need some documentation about how to use the feature, not Helm changes.

I believe you should be able to do:

helm install ... --namespace kube-system --set ipMasqAgent.enabled=true

And then create a new ConfigMap in kube-system namespace with the complete ipmasq agent configuration, w 8000 hatever this is generating:

apiVersion: v1
kind: ConfigMap
metadata:
  name: ip-masq-agent
  namespace: {{ .Release.Namespace }}
data:
  enable-ip-masq-agent: "true"
  config: |-
{{ toJson .Values.ipMasqAgent.config | indent 4 }}
{{- end }}

If we put this into the Helm charts, then my concern is that users who use this feature and upgrade will have their settings overwritten. I'm not sure if it even makes sense for Cilium's Helm configuration to control this configmap?

@joestringer
Copy link
Member
joestringer commented Jun 16, 2022 •
edited
Loading

One option is that we could include an example copy of this configmap in examples/kubernetes/ip-masq-agent/ip-masq-agent-config.yaml, and then in the docs tell people to edit & apply that YAML directly..?

brb
brb requested changes Jun 17, 2022
View reviewed changes
@alphabet5
Copy link

@joestringer the specific use-case I have is to eliminate a separate manifest for the configmap that isn't tied to a deployment. Having all of ciliums components managed through a single flux helmdeployment would simplify my deployment repo a bit.

I think wrapping the whole configmap in an if statement, and leaving .Values.ipMasqAgent.config commented out would keep existing functionality the same. Then by default it's managed separately as it is now, but can be created with the helm chart.

something like

{{- if and .Values.ipMasqAgent .Values.ipMasqAgent.enabled .Values.ipMasqAgent.config }}
apiVersion: v1
kind: ConfigMap
metadata:
  name: ip-masq-agent
  namespace: {{ .Release.Namespace }}
data:
  config: |
{{ toJson .Values.ipMasqAgent.config | indent 4 }}
{{- end }}

It's not a huge deal, but could make things a bit nicer.

@cyclinder
Copy link
Contributor Author

I'm sorry for not replying for a long time, I've been busy with work these days..

@aanm aanm requested review from joestringer and brb July 8, 2022 11:11
brb
brb requested changes Jul 11, 2022
View reviewed changes
Copy link
Member
@brb brb left a comment

Choose a reason for hiding this comment

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

Could you extend Documentation/concepts/networking/masquerading.rst to show how to use the new way of configuring the ipmasq?

@cyclinder cyclinder requested a review from a team as a code owner July 11, 2022 13:57
@cyclinder
Copy link
Contributor Author
cyclinder commented Jul 11, 2022 •
edited
Loading

Could you extend Documentation/concepts/networking/masquerading.rst to show how to use the new way of configuring the ipmasq?

Sure! Hi @brb , I'm not a english speaker, so If you could give me some advice for new changes, I would be very grateful!

brb
brb reviewed Jul 12, 2022
View reviewed changes
@cyclinder
Copy link
Contributor Author

It seems to me like 90% of the test changes are already done in the diff above, the configuration just needs to be removed. Is that more than just a kubectl delete ... command? It would be nice to cover this in the tests when this lands, so that we can confirm that it works and avoid regressions.

@joestringer @brb Thanks for your suggestions. Now all the changes in the PR.

I don't think we can delete the configMap directly here, After my testing, the configMap mounted by the agent pod does not changed, so we should use patch here, but I'm not sure which namespace the configMap is created in, I guess it is helpers. LogGathererNamespace?

@brb
Copy link
Member
brb commented Jul 21, 2022

After my testing, the configMap mounted by the agent pod does not changed, so we should use patch here, but I'm not sure which namespace the configMap is created in, I guess it is helpers. LogGathererNamespace?

Yes, you can patch it. Please use helpers.CiliumNamespace.

@cyclinder
Copy link
Contributor Author

helpers.CiliumNamespace

Okay, Thanks.

@joestringer joestringer removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 25, 2022
@joestringer
Copy link
Member

/test

kaworu
kaworu requested changes Aug 4, 2022
View reviewed changes
Copy link
Member
@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Minor comment, other than that Helm related changes LGTM.

@cyclinder cyclinder requested review from a team as code owners August 6, 2022 02:05
sayboras
sayboras approved these changes Aug 8, 2022
View reviewed changes
Copy link
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 on helm changes, thanks 🥇

kaworu
kaworu approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member
@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thank you!

@sayboras sayboras added the area/helm Impacts helm charts and user deployment experience label Aug 11, 2022
@cyclinder
Copy link
Contributor Author

Thanks @kaworu @sayboras ，Hi @joestringer ， Now this PR can be merge？

@joestringer joestringer requested a review from brb August 11, 2022 21:52
@joestringer
Copy link
Member

/test

@joestringer
Copy link
Member

At a glance it looked like @brb's latest feedback was addressed, so I just triggered CI to double-check that it's passing. Assuming that's all good and @brb didn't have further feedback, this should be good to merge.

brb
brb reviewed Aug 12, 2022
View reviewed changes
test/k8s/datapath_configuration.go Outdated
@@ -463,81 +449,53 @@ var _ = Describe("K8sDatapathConfig", func() {
res.ExpectSuccess()
tmpEchoPodPath = strings.Trim(res.Stdout(), "\n")
kubectl.ExecMiddle(fmt.Sprintf("sed 's/NODE_WITHOUT_CILIUM/%s/' %s > %s",
helpers.GetFirstNodeWithoutCilium(), echoPodPath, tmpEchoPodPath)).ExpectSuccess()
"k8s3", echoPodPath, tmpEchoPodPath)).ExpectSuccess()
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author
@cyclinder cyclinder Aug 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. This change comes from your suggestion. #20137 (comment), Is the CI failure related to this change?

@brb
Copy link
Member
brb commented Aug 15, 2022

@cyclinder Thanks. Could you rebase your code against the latest Cilium's master. It should fix some GH action failures. Once it's done, I will rerun the CI.

@cyclinder
nonMasqueradeCIDRs configuration added to the helm chart
dffccdf 
Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
@cyclinder
Copy link
Contributor Author

@cyclinder Thanks. Could you rebase your code against the latest Cilium's master. It should fix some GH action failures. Once it's done, I will rerun the CI.

Thanks @brb , I've already rebased with the latest master branch.

@brb
Copy link
Member
brb commented Aug 15, 2022

/test

@cyclinder
Copy link
Contributor Author
cyclinder commented Aug 15, 2022 •
edited
Loading

It seems to the CI failures: ConformanceGKE(ci-gke) has no relation with these changes ?

brb
brb approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member
@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 22, 2022
@aditighag aditighag merged commit 998ee12 into cilium:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@brb brb brb approved these changes

@joestringer joestringer joestringer approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cyclinder @joestringer @alphabet5 @brb @kaworu @sayboras @aditighag
0