Helm: Clean up deprecated values #24214

qmonnet · 2023-03-07T11:52:36Z

helm: Mention deprecated encryption.<attr> values in upgrade notes
helm: Remove deprecated value hubble.peerService.enabled
helm: Remove deprecated hubble.tls.ca and sub-values
⚠️ I'm not sure I got the template changes in this commit right, @sayboras please take a look
helm: Remove deprecated value hubble.ui.securityContext.enabled
helm: Remove deprecated values ipam.operator.clusterPoolIPv{4,6}PodCIDR

These commits result from a simple search for deprecated in values.yaml.tmpl.

qmonnet · 2023-03-07T12:06:39Z

Forgot to update the Helm reference in the docs at each commit, I'll fix this now
[EDIT: Fixed]

Documentation/operations/upgrade.rst

kaworu

Thanks @qmonnet 🙏 !

zacharysarah

@qmonnet 👋🏻 Looks good! Mostly nits, with one advisory about choosing between "to" or "will" to indicate lesser or greater likelihood of removal, respectively.

I also left a question for you about whether it's possible to avoid future tense in one specific instance while still remaining technically accurate.

Otherwise LGTM!

Documentation/operations/upgrade.rst

install/kubernetes/cilium/README.md

install/kubernetes/cilium/values.yaml.tmpl

qmonnet · 2023-03-10T12:16:36Z

@qmonnet 👋🏻 Looks good! Mostly nits, with one advisory about choosing between "to" or "will" to indicate lesser or greater likelihood of removal, respectively.

Thanks a lot for the review!

Thinking of the two terms, I decided to stick with the “to” construct. We do intend to remove these values, but after all, this PR cleans up values that we intended to remove in 1.13, and prepares removal for some that we intended to see disappear in 1.11. So they should go in 1.15, but there's always a risk we forget to actually remove them 🙂. Let me know if you strongly feels against it.

I also left a question for you about whether it's possible to avoid future tense in one specific instance while still remaining technically accurate.

Thanks as well, but this particular instance is unrelated to the current PR. The README file is generated automatically from the changes in values.yaml.tmpl (with make -C install/kubernetes), so I didn't edit it manually, and the comment from which it comes from is untouched. So I agree with your suggestion, but consider it outside of the scope.

Otherwise LGTM!

Thanks! I fixed the other nits and rebased.

qmonnet · 2023-03-10T12:17:18Z

I'll do a separate PR to mark the README file as auto-generated in GitHub diff interface.
[EDIT: #24295]

sayboras

Sorry for late reply. I have one comment as part below, the rest looks good to me.

install/kubernetes/cilium/templates/cilium-ca-secret.yaml

qmonnet · 2023-03-20T11:08:06Z

@zacharysarah Could you have another look at this PR, please?

qmonnet · 2023-04-14T14:46:35Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.001s.

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/1784/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 30.000s.

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/1837/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

qmonnet · 2023-04-14T15:35:16Z

Good thing we added the failsafe, it seems

Let's fix test/helpers/kubectl.go.

qmonnet · 2023-04-14T15:43:45Z

/test

qmonnet · 2023-04-17T09:02:55Z

/test

gandro

Thanks a lot for taking care of all of this, especially the added check around the clusterpool pod CIDR!

install/kubernetes/cilium/templates/cilium-configmap.yaml

Some Helm values related to encryption have been deprecated in the charts since Cilium version 1.10, in commit b52e210 ("helm: move IPSec options under encryption.ipsec"), but we have never logged the change in the upgrade instructions. Let's do it now, so we can remove these values safely in 1.15. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Helm value hubble.peerService.enabled has been deprecated in commit 66ea2f9 ("hubble: deprecate relay peer-svc through unix domain socket") and, for the upgrade notes, 1.13-backport commit 8ebc2ba ("hubble: deprecate relay peer-svc through unix domain socket"). We can remove this value from the Charts. We also need to adjust some of the templates now that this value is gone. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

These values were deprecated in Cilium 1.12 with commit 0c55c8e ("helm: Support using same CA with helm method"). We can now remove them from the Helm charts and templates. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Value hubble.ui.securityContext.enabled was removed in Cilium 1.12, with commit 7a457ec ("Expose hubble-ui security context in helm chart"). We can now remove the option from the Helm charts. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

These values were deprecated in Cilium 1.11, with commit 8a7c37f ("fix(cluster-pools): fix parameter"), but have never been removed. Let's remove them from the chart and configmap template. To preserve the behaviour when users don't change the default values, the default CIDRs are moved from the removed options to their list counterparts. To avoid surprises resulting from the change, in particular if users have set up a value for clusterPoolIPv{4,6}PodCIDR and do not expect it to be overwritten by the list versions, make the Helm templating fail if the removed value is still present. This failsafe mechanism can be removed in a few minor versions, or if Helm starts one day to warn on unused values [0]. [0] helm/helm#6422 Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet · 2023-04-18T10:11:40Z

/test

Deprecated Cilium Helm Chart values have been removed in the PR cilium/cilium#24214. Installing Cilium (`cilium install`) & enabling hubble (`cilium hubble enable`) will break the connectivity from the hubble relay to the peer service (`Failed to create peer client for peers synchronization...`). This will consequently also fail the connectivity tests with the following error: `Timeout waiting for flow listener to become ready`. We need to set `tls.ca.cert` & `tls.ca.key` during `cilium hubble enable` to use the same CA cert. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Deprecated Cilium Helm Chart values have been removed in the PR cilium#24214. Installing Cilium (`cilium install`) & enabling hubble (`cilium hubble enable`) will break the connectivity from the hubble relay to the peer service (`Failed to create peer client for peers synchronization...`). This will consequently also fail the connectivity tests with the following error: `Timeout waiting for flow listener to become ready`. We need to set `tls.ca.cert` & `tls.ca.key` during `cilium hubble enable` to use the same CA cert. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

qmonnet added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact. area/helm Impacts helm charts and user deployment experience labels Mar 7, 2023

qmonnet requested review from jibi, kaworu and sayboras March 7, 2023 11:52

qmonnet requested review from a team as code owners March 7, 2023 11:52

qmonnet changed the title ~~Helm: Clean-up deprecated values~~ Helm: Clean up deprecated values Mar 7, 2023

qmonnet force-pushed the pr/values-cleanup branch from 3383d7f to 2643d45 Compare March 7, 2023 12:08

kaworu reviewed Mar 7, 2023

View reviewed changes

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved

qmonnet force-pushed the pr/values-cleanup branch from 2643d45 to de71bf7 Compare March 7, 2023 14:16

qmonnet requested a review from kaworu March 7, 2023 14:18

qmonnet force-pushed the pr/values-cleanup branch from de71bf7 to eb00a5f Compare March 7, 2023 14:18

kaworu approved these changes Mar 7, 2023

View reviewed changes

jibi approved these changes Mar 7, 2023

View reviewed changes

sayboras added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 8, 2023

zacharysarah requested changes Mar 8, 2023

View reviewed changes

qmonnet force-pushed the pr/values-cleanup branch from eb00a5f to ebd207a Compare March 10, 2023 12:05

qmonnet removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 10, 2023

qmonnet requested a review from zacharysarah March 10, 2023 12:16

sayboras reviewed Mar 18, 2023

View reviewed changes

install/kubernetes/cilium/templates/cilium-ca-secret.yaml Outdated Show resolved Hide resolved

qmonnet force-pushed the pr/values-cleanup branch from ebd207a to df5c31b Compare March 20, 2023 11:05

qmonnet requested a review from sayboras March 20, 2023 11:06

qmonnet marked this pull request as ready for review April 14, 2023 14:46

qmonnet requested a review from gandro April 14, 2023 14:46

qmonnet force-pushed the pr/values-cleanup branch from 95b20ec to 9c86ba1 Compare April 14, 2023 15:42

qmonnet requested a review from a team as a code owner April 14, 2023 15:42

qmonnet force-pushed the pr/values-cleanup branch from 9c86ba1 to c5d3ff7 Compare April 17, 2023 08:56

gandro approved these changes Apr 17, 2023

View reviewed changes

qmonnet added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Apr 17, 2023

joestringer reviewed Apr 17, 2023

View reviewed changes

install/kubernetes/cilium/templates/cilium-configmap.yaml Outdated Show resolved Hide resolved

install/kubernetes/cilium/templates/cilium-configmap.yaml Outdated Show resolved Hide resolved

install/kubernetes/cilium/templates/cilium-configmap.yaml Outdated Show resolved Hide resolved

qmonnet added 5 commits April 18, 2023 10:40

helm: Remove deprecated hubble.tls.ca and sub-values

0fd3945

These values were deprecated in Cilium 1.12 with commit 0c55c8e ("helm: Support using same CA with helm method"). We can now remove them from the Helm charts and templates. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet force-pushed the pr/values-cleanup branch from c5d3ff7 to 8d59aae Compare April 18, 2023 09:42

qmonnet requested a review from joestringer April 18, 2023 09:46

joestringer approved these changes Apr 18, 2023

View reviewed changes

joestringer merged commit 1eef5ac into cilium:main Apr 18, 2023
57 checks passed

qmonnet deleted the pr/values-cleanup branch April 18, 2023 23:33

mhofstetter mentioned this pull request Apr 19, 2023

cli: adapt helm chart cleanup of deprecated hubble values cilium/cilium-cli#1520

Merged

sayboras mentioned this pull request Feb 3, 2024

helm: Remove deprecated values encryption.* #30613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm: Clean up deprecated values #24214

Helm: Clean up deprecated values #24214

qmonnet commented Mar 7, 2023 •

edited

qmonnet commented Mar 7, 2023 •

edited

kaworu left a comment

zacharysarah left a comment

qmonnet commented Mar 10, 2023

qmonnet commented Mar 10, 2023 •

edited

sayboras left a comment

qmonnet commented Mar 20, 2023

qmonnet commented Apr 14, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

qmonnet commented Apr 14, 2023 •

edited

qmonnet commented Apr 14, 2023

qmonnet commented Apr 17, 2023

gandro left a comment

qmonnet commented Apr 18, 2023

Helm: Clean up deprecated values #24214

Helm: Clean up deprecated values #24214

Conversation

qmonnet commented Mar 7, 2023 • edited

qmonnet commented Mar 7, 2023 • edited

kaworu left a comment

Choose a reason for hiding this comment

zacharysarah left a comment

Choose a reason for hiding this comment

qmonnet commented Mar 10, 2023

qmonnet commented Mar 10, 2023 • edited

sayboras left a comment

Choose a reason for hiding this comment

qmonnet commented Mar 20, 2023

qmonnet commented Apr 14, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

qmonnet commented Apr 14, 2023 • edited

qmonnet commented Apr 14, 2023

qmonnet commented Apr 17, 2023

gandro left a comment

Choose a reason for hiding this comment

qmonnet commented Apr 18, 2023

qmonnet commented Mar 7, 2023 •

edited

qmonnet commented Mar 7, 2023 •

edited

qmonnet commented Mar 10, 2023 •

edited

qmonnet commented Apr 14, 2023 •

edited by maintainer-s-little-helper bot

qmonnet commented Apr 14, 2023 •

edited