Skip to content

helm: Fix duplicate tunnel-protocol configuration in cilium-config configmap #36226

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 2 commits into from
Nov 28, 2024
Merged

helm: Fix duplicate tunnel-protocol configuration in cilium-config configmap #36226

merged 2 commits into from
Nov 28, 2024

Conversation

jonasbadstuebner
Copy link
Contributor

@jonasbadstuebner jonasbadstuebner commented Nov 28, 2024 •
edited
Loading

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.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this 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.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

We had trouble setting this up too, and as @haribhusal2025 stated in #33456 (comment), setting the routingMode and tunnelProtocol explicitly works around the helm templating issues.

I am aware of #34315 attempting to fix this issue, but I think the approach of using a variable and overwriting it, is not the best for user experience.

As a heavy user of helm, having to look at the helm template itself to know what is happening or where values come from is more frustrating than simply being shown an error.
So I propose a solution to the problem that gives an early error message, without introducing more edge cases to check for with nested if-statements.

Fixes: #33456
Closes: #34315

Adjust verification for tunnel-protocol and routing-mode in helm templates to remove occurrence of duplicate entries in rendered configmap.
Remove constraint on tunnelProtocol for aksbyocni.

@maintainer-s-little-helper
Copy link

Commit ab036db does not match "(?m)^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 Nov 28, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 28, 2024
@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from ab036db to da0bfdc Compare November 28, 2024 08:34
@maintainer-s-little-helper
Copy link

Commit ab036db does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from da0bfdc to a5ae039 Compare November 28, 2024 08:35
@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 Nov 28, 2024
@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch 2 times, most recently from 3a50837 to cce49a9 Compare November 28, 2024 08:39
@jonasbadstuebner jonasbadstuebner marked this pull request as ready for review November 28, 2024 08:50
@jonasbadstuebner jonasbadstuebner requested review from a team as code owners November 28, 2024 08:50
@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from cce49a9 to 00229ab Compare November 28, 2024 08:51
@jonasbadstuebner
Copy link
Contributor Author

I added the release-note by editing the initial comment, but the label did not get removed.

Please let me know if I should add something to this PR etc.

@gandro gandro added the release-note/misc This PR makes changes that have no direct user impact. label Nov 28, 2024
@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 Nov 28, 2024
gandro
gandro requested changes Nov 28, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Unfortunately this is a breaking change - we either need to come up with a solution that doesn't break existing installation commands - or patch all the docs and tooling before we can ship this PR.

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience and removed release-note/misc This PR makes changes that have no direct user impact. labels Nov 28, 2024
@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from 00229ab to 2af7447 Compare November 28, 2024 09:42
gandro
gandro reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks better to me! I think we should also get a review from @giorio94 to make sure we're fine with tunnel-protocol: vxlan if the user specifies an explicitly sets the native routing mode (I believe it should be fine since the agent also defaults to that)

@gandro gandro requested a review from giorio94 November 28, 2024 10:08
giorio94
giorio94 reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks. Changes look good to me, just a couple of comments inline.

I think we should also get a review from @giorio94 to make sure we're fine with tunnel-protocol: vxlan if the user specifies an explicitly sets the native routing mode (I believe it should be fine since the agent also defaults to that)

Yes, the agent defaults to vxlan as tunnel protocol in all cases, so that's consistent.

cilium/pkg/datapath/tunnel/tunnel.go

Lines 235 to 238 in 89c98fb

var defaultConfig = userCfg{
TunnelProtocol: defaults.TunnelProtocol,
TunnelPort: 0, // auto-detect based on the protocol.
}

cilium/pkg/defaults/defaults.go

Lines 514 to 515 in 89c98fb

// TunnelProtocol is the default tunneling protocol
TunnelProtocol = "vxlan"

@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from 2af7447 to fbc81f4 Compare November 28, 2024 13:08
gandro
gandro approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@gandro gandro 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 now, thank you! I think we can drop the aksbyocni check too

After that, let's see what CI says

jonasbadstuebner added a commit to jonasbadstuebner/cilium that referenced this pull request Nov 28, 2024
@jonasbadstuebner
helm: Remove tunnelProtocol limitation to vxlan for aksbyocni
b02f6e5 
Previously the tunnel-protocol was limited to VXLAN, when enabling
aksbyocni. As discussed in
cilium#36226 (comment) this
seems to be a leftover from when there was just one flag called "tunnel"
that combined routing-mode and tunnel-protocol.

This patch makes it possible to configure any tunnel-protocol with
aksbyocni.enabled: true.

Related: cilium#24561

Signed-off-by: Jonas Badstübner <jonas@jb.software>
@jonasbadstuebner
Copy link
Contributor Author

Should I add the change for allowing all tunnelProtocols now to the release-notes as well?

@giorio94
Copy link
Member

/test

@jonasbadstuebner
helm: Fix duplicate tunnel-protocol key in cilium-config configmap
00f5ede 
Previously it was only possible to enable geneve with tunnel routing
mode by explicitly setting both as helm values.
Otherwise it would fail like this:

line 132: mapping key "tunnel-protocol" already defined at line 131

Fixes cilium#33456

Signed-off-by: Jonas Badstübner <jonas@jb.software>
@jonasbadstuebner
helm: Remove tunnelProtocol limitation to vxlan for aksbyocni
f681254 
Previously the tunnel-protocol was limited to VXLAN, when enabling
aksbyocni. As discussed in
cilium#36226 (comment) this
seems to be a leftover from when there was just one flag called "tunnel"
that combined routing-mode and tunnel-protocol.

This patch makes it possible to configure any tunnel-protocol with
aksbyocni.enabled: true.

Related: cilium#24561

Signed-off-by: Jonas Badstübner <jonas@jb.software>
@jonasbadstuebner jonasbadstuebner force-pushed the 33456-fix-tunnel-protocol-configuration branch from b02f6e5 to f681254 Compare November 28, 2024 13:41
@jonasbadstuebner
Copy link
Contributor Author

Sorry for aborting the tests, probably should have waited for them to finish.

@gandro
Copy link
Member

gandro commented Nov 28, 2024

/test

@gandro gandro removed the request for review from youngnick November 28, 2024 13:49
@gandro
Copy link
Member

gandro commented Nov 28, 2024

Restarted failed CI (failures are common flakes unrelated to the PR)

@gandro gandro enabled auto-merge November 28, 2024 14:34
@gandro gandro added this pull request to the merge queue Nov 28, 2024
@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 Nov 28, 2024
Merged via the queue into cilium:main with commit f281de5 Nov 28, 2024
64 checks passed
@jonasbadstuebner jonasbadstuebner deleted the 33456-fix-tunnel-protocol-configuration branch November 28, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@giorio94 giorio94 giorio94 left review comments

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience kind/community-contribution This was a contribution made by a community member. 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.

Helm can't enable geneve protocol: line 132: mapping key "tunnel-protocol" already defined at line 131
3 participants
@jonasbadstuebner @giorio94 @gandro