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

Change routing-mode and tunnel-protocol based on .Values.tunnel and .Values.routingMode #27841

Merged
merged 2 commits into from Sep 13, 2023
Merged

Change routing-mode and tunnel-protocol based on .Values.tunnel and .Values.routingMode #27841

merged 2 commits into from Sep 13, 2023

Conversation

macmiranda
Copy link
Contributor

@macmiranda macmiranda commented Aug 31, 2023 •
edited

Fixes #27756

@macmiranda macmiranda requested review from a team as code owners August 31, 2023 07:59
@maintainer-s-little-helper
Copy link

Commit 8636098a7e5ec3b8b2a67a607c41124b0e900966 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 Aug 31, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 31, 2023
@macmiranda macmiranda changed the title Change routing-mode and tunnel-protocol based on .Values.tunnel and .Values.routingMode, default to tunnel and vxlan. Change routing-mode and tunnel-protocol based on .Values.tunnel and .Values.routingMode Aug 31, 2023
@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 Aug 31, 2023
gandro
gandro requested changes Sep 4, 2023
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! Overall looks good, and most combinations work, but I've found that specifying both a routing mode and a tunnel protocol did not work as expected, e.g. --set tunnel=geneve --set routingMode=tunnel did not set the tunnel-protocol correctly, which I think it should also do.

Also, the commit subject is overly long, thus causing checkpatch.pl to fail in CI. Could you shorten it a bit and instead put the details in the commit message description? Thanks

@gandro gandro added area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium. labels Sep 4, 2023
@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 Sep 4, 2023
@gandro gandro added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Sep 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Sep 4, 2023
@gandro gandro requested a review from pchaigno September 4, 2023 11:59
@gandro
Copy link
Member

gandro commented Sep 4, 2023

@pchaigno Would you mind giving this a quick look too if the flag combinations are still working as intended?

@macmiranda
Copy link
Contributor Author

The way I see it, tunnel should only be taken into consideration if routingMode isn't set, wouldn't you agree?

What if someone sets tunnel and tunnelProtocol to different things? That would unnecessarily complicate things.

Since tunnel is deprecated, it should be ignored when (the new) routingMode and tunnelProtocol are set.

The only case in which tunnel should be honored is if it were already set and none of the new values were explicitly given. (This would only happen if you upgraded Cilium without reading the release notes or ignored the deprecation notice).

gandro
gandro approved these changes Sep 5, 2023
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.

Since tunnel is deprecated, it should be ignored when (the new) routingMode and tunnelProtocol are set.

Yeah I think you are right, I missed the fact that tunnel has been replaced by tunnelProtocol in Helm too.

I wonder if we should introduce some additional validations that disallow tunnelProtocol without a routing mode, or tunnel with routingMode.

At the moment, if I specify --set tunnelProtocol=geneve without routingMode, it generates the following for some reason:

  # Encapsulation mode for communication between nodes
  # Possible values:
  #   - disabled
  #   - vxlan (default)
  #   - geneve
  # Default case
  routing-mode: "tunnel"
  tunnel-protocol: "vxlan"
  tunnel-protocol: "geneve"

@macmiranda
Copy link
Contributor Author

Yes, agreed. I'll add the validation you mentioned.

@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@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 Sep 5, 2023
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@gandro
Copy link
Member

gandro commented Sep 6, 2023

Great! Could you remove the validation commits instead of reverting them clean history on the main branch? Thanks!

@macmiranda
Copy link
Contributor Author

@gandro anything else?

@michi-covalent michi-covalent added this to Needs backport from main in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.2 Sep 9, 2023
@gandro
Copy link
Member

gandro commented Sep 11, 2023

@gandro anything else?

No, this looks good to me know! Thanks again, let's wait for the other CODEOWNERs to review.

@gandro gandro removed request for a team September 11, 2023 11:21
@gandro gandro removed the request for review from pchaigno September 12, 2023 09:02
@gandro
Copy link
Member

gandro commented Sep 12, 2023 •
edited

/test

Edit:

@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 Sep 13, 2023
@julianwiedmann julianwiedmann merged commit fdeb6f3 into cilium:main Sep 13, 2023
62 checks passed
@aanm aanm added the kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. label Sep 15, 2023
@giorio94 giorio94 mentioned this pull request Sep 26, 2023
Merged
22 tasks
@giorio94 giorio94 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Sep 26, 2023
gandro added a commit to cilium/cilium-cli that referenced this pull request Sep 28, 2023
@gandro
install: Avoid using deprecated "tunnel" flag
25781a5 
The tunnel option is deprecated and will be removed in Cilium v1.15.
This commit fixes the remaining uses I have found where the Cilium CLI
still set the old `tunnel` flag unconditionally, which will lead to
issues once the flag is no longer accepted [1]. The Cilium CLI now only
uses the deprecated `tunnel` flag for Cilium versions 1.13 and older.

When reading the ConfigMap (such as in the clustermesh code), we attempt
to first parse the new values, before falling back on the old ones.

[1] cilium/cilium#27841 (comment)

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro mentioned this pull request Sep 28, 2023
Merged
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 29, 2023
gandro added a commit to cilium/cilium-cli that referenced this pull request Oct 2, 2023
@gandro
install: Avoid using deprecated "tunnel" flag
1d86d69 
The tunnel option is deprecated and will be removed in Cilium v1.15.
This commit fixes the remaining uses I have found where the Cilium CLI
still set the old `tunnel` flag unconditionally, which will lead to
issues once the flag is no longer accepted [1]. The Cilium CLI now only
uses the deprecated `tunnel` flag for Cilium versions 1.13 and older.

When reading the ConfigMap (such as in the clustermesh code), we attempt
to first parse the new values, before falling back on the old ones.

[1] cilium/cilium#27841 (comment)

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro added a commit to cilium/cilium-cli that referenced this pull request Oct 2, 2023
@gandro
install: Avoid using deprecated "tunnel" flag
6b84802 
The tunnel option is deprecated and will be removed in Cilium v1.15.
This commit fixes the remaining uses I have found where the Cilium CLI
still set the old `tunnel` flag unconditionally, which will lead to
issues once the flag is no longer accepted [1]. The Cilium CLI now only
uses the deprecated `tunnel` flag for Cilium versions 1.13 and older.

When reading the ConfigMap (such as in the clustermesh code), we attempt
to first parse the new values, before falling back on the old ones.

[1] cilium/cilium#27841 (comment)

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@jrajahalme jrajahalme moved this from Needs backport from main to Backport done to v1.14 in 1.14.3 Oct 18, 2023
@jrajahalme jrajahalme mentioned this pull request Oct 18, 2023
Merged
michi-covalent pushed a commit to cilium/cilium-cli that referenced this pull request Nov 18, 2023
@renovate @michi-covalent
chore(deps): update dependency cilium/cilium to v1.14.4
7cb6425 
cilium/cilium#27841 changed how the routing mode gets set for GKE, and
now it always gets set to "native". Use --datapath-mode flag to force
the tunnel mode for the external workload test since that's the only
configuration that's known to work [^1].

Fixes: #2070

[^1]: https://docs.cilium.io/en/latest/network/external-workloads/

Signed-off-by: renovate[bot] <bot@renovateapp.com>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent mentioned this pull request Nov 18, 2023
Merged
1 task
michi-covalent pushed a commit to cilium/cilium-cli that referenced this pull request Nov 20, 2023
@renovate @michi-covalent
chore(deps): update dependency cilium/cilium to v1.14.4
ce305f7 
cilium/cilium#27841 changed how the routing mode gets set for GKE, and
now it always gets set to "native". Use --datapath-mode flag to force
the tunnel mode for the external workload test since that's the only
configuration that's known to work [^1].

Fixes: #2070

[^1]: https://docs.cilium.io/en/latest/network/external-workloads/

Signed-off-by: renovate[bot] <bot@renovateapp.com>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@giorio94 giorio94 mentioned this pull request Dec 6, 2023
Merged
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

@michi-covalent michi-covalent michi-covalent left review comments

@giorio94 giorio94 giorio94 left review comments

@nathanjsweet nathanjsweet nathanjsweet approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/community-contribution This was a contribution made by a community member. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.14.3
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

helm: apparent incompatible values when using routingMode with tunnel unset
7 participants
@macmiranda @gandro @nathanjsweet @michi-covalent @giorio94 @aanm @julianwiedmann