Fix replacements errors in helm charts and templates #52459

fjglira · 2024-08-01T14:56:30Z

Update and fix helm errors

After the merge of the PR's #52395 and #52036 some of the replacements using the profile are being done wrong because of the move of the fields on those PR's. More information in the issue #52458 and we can discuss there about the move of the fields

Fixes #52458

Signed-off-by: frherrer <frherrer@redhat.com> Update and fix helm errors Signed-off-by: frherrer <frherrer@redhat.com>

istio-testing · 2024-08-01T14:56:41Z

Hi @fjglira. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

howardjohn

Thanks! that was a sloppy find and replace, sorry..

manifests/charts/istio-cni/templates/configmap-cni.yaml

Co-authored-by: John Howard <john.howard@solo.io>

Signed-off-by: frherrer <frherrer@redhat.com>

manifests/charts/istio-cni/templates/configmap-cni.yaml

manifests/charts/istio-cni/templates/daemonset.yaml

howardjohn · 2024-08-01T16:00:25Z

manifests/helm-profiles/openshift-ambient.yaml

  cniBinDir: /var/lib/cni/bin
  cniConfDir: /etc/cni/multus/net.d
  chained: false
  cniConfFileName: "istio-cni.conf"
-  logLevel: info
-  provider: "multus"
-pilot:


I don't think this should move. Technically i think it will work its just a bit confusing.

The problem is we currently share the same profile between all charts, which is a bit wonky but technically works.

We then allow flattening pilot and cni.

These values are meant to be passed to the pilot chart, they just (somewhat confusingly) are passed as pilot.cni. So because we unwrap that, we happen to get cni which happens to conflict with the CNI chart.

A more clean approach would be to have a profile-per-chart, but its a pain to maintain.

cc @bleggett

I don't think this should move. Technically i think it will work its just a bit confusing.

The problem is we currently share the same profile between all charts, which is a bit wonky but technically works.

We then allow flattening pilot and cni.

These values are meant to be passed to the pilot chart, they just (somewhat confusingly) are passed as pilot.cni. So because we unwrap that, we happen to get cni which happens to conflict with the CNI chart.

A more clean approach would be to have a profile-per-chart, but its a pain to maintain.

In the case of the openshift profile, I think there is no override of any pilot value.That is why I deleted those in this case. Also, checking in the repo I only find the use of pilot.cni* in some testdata yaml but instead .Values.cni.enabled and .Values.cni.provider are used across all charts now.

Yeah - these are pilot values, and used nowhere else. They should be scoped exclusively to pilot.

helm install pilot --set cni.enabled

helm install <thing-that-composes-pilot-by-name> --set pilot.cni.enabled

helm install <thing-that-composes-pilot-and-cni-by-name> --set pilot.cni.enabled

should all work just fine in normative helm and inherently be correctly scoped. but since we have/are slowly undoing our own scoping conventions it's not that simple.

As I mentioned in
#52036 (comment)
if pilot.cni is uniquely a problem because we are eagerly unwrapping pilot, I think we can just manually rescope/munge pilot.cni first, then unwrap what's left of pilot as we do currently.

enabled and provider are the only two keys under pilot.cni, they can be munged to cniEnabled and cniProvider under the hood in a sprig templating func if we want.

(if we used Helm composition and composed istiod and istio-cni in the same chart Helm would populate an equivalent of enabled for us anyway dynamically based on whether istio-cni was installed as a dependency or not, so IMO that one is a bit suspect already).

And cni.provider is used only by OpenShift anyway. It has one valid value (multus) - we should drop it, as whatever it does can and should just be done by the openshift profile/platform: openshift.

I'd just s/values.cni.provider=multus/values.platform=openshift/g and drop cni.provider

@bleggett Should I do this replacement of cni.provider in this PR or do it in another PR to not mix the things? I agree that if the only value used in all the charts is multus and is only used to set openshift specific configuration does not make sense to have it there and should set those values by using values.platform=openshift. Regarding enable will be only key remaining under pilot, can check as you metion to cniEnabled a leave it under pilot? wdyt?

@bleggett seems to be that I can't add /release-notes-none label

I'll add it - can you remove https://github.com/istio/istio/blob/49abfa448f43cfd61ed85ba00b640cdd3a47befd/manifests/helm-profiles/openshift.yaml and the resulting value moves, deferring those to followups where we change the scoping properly/drop some of them?

That should reduce the delta here.

sure, ill try to do that this weekend if not Monday morning will be done

sure, ill try to do that this weekend if not Monday morning will be done

Don't do it over the weekend - if you do I'll be tempted to look at it :P

OK, changes are done. To summarise the changes on this PR and what is pending for the next one:
Fix:

.Values.cni errors in helm charts

Fix incorrect replace in apiVersion: k8s.cni.cncf.io/v1

Fix .Values.pilot.cni.enabled and .Values.pilot.cni.provider incorrect replace that affect login in helm charts

Pending for the next PR:

Replace the use of cni.provider because the only use is to set values applicable to openshift. Replace by the use of platform=openshift

Replace pilot.cni.enabled for pilot. cniEnabled to avoid warp cni inside pilot

I'll check if there is another helm value that can be simplified and check in the docs to see if we need to update some of the docs also

Co-authored-by: John Howard <john.howard@solo.io>

manifests/charts/istio-cni/templates/configmap-cni.yaml

Co-authored-by: John Howard <john.howard@solo.io>

howardjohn · 2024-08-01T20:00:15Z

/ok-to-test

howardjohn · 2024-08-01T20:00:19Z

ty for the fixes!

bleggett · 2024-08-01T21:02:00Z

manifests/charts/istio-cni/templates/configmap-cni.yaml

@@ -14,8 +14,8 @@ data:
  AMBIENT_ENABLED: {{ .Values.ambient.enabled | quote }}
  AMBIENT_DNS_CAPTURE: {{ .Values.ambient.dnsCapture | default "false" | quote  }}
  AMBIENT_IPV6: {{ .Values.ambient.ipv6 | default "false" | quote }}
-  {{- if .ValuesConfFileName }} # K8S < 1.24 doesn't like empty values


I shoulda caught this in review, that's on me.

bleggett · 2024-08-01T21:15:17Z

manifests/helm-profiles/openshift-ambient.yaml

  cniBinDir: /var/lib/cni/bin
  cniConfDir: /etc/cni/multus/net.d
  chained: false
  cniConfFileName: "istio-cni.conf"
-  logLevel: info
-  provider: "multus"
-pilot:


Yeah - these are pilot values, and used nowhere else. They should be scoped exclusively to pilot.

helm install pilot --set cni.enabled

helm install <thing-that-composes-pilot-by-name> --set pilot.cni.enabled

helm install <thing-that-composes-pilot-and-cni-by-name> --set pilot.cni.enabled

should all work just fine in normative helm and inherently be correctly scoped. but since we have/are slowly undoing our own scoping conventions it's not that simple.

As I mentioned in
#52036 (comment)
if pilot.cni is uniquely a problem because we are eagerly unwrapping pilot, I think we can just manually rescope/munge pilot.cni first, then unwrap what's left of pilot as we do currently.

enabled and provider are the only two keys under pilot.cni, they can be munged to cniEnabled and cniProvider under the hood in a sprig templating func if we want.

(if we used Helm composition and composed istiod and istio-cni in the same chart Helm would populate an equivalent of enabled for us anyway dynamically based on whether istio-cni was installed as a dependency or not, so IMO that one is a bit suspect already).

And cni.provider is used only by OpenShift anyway. It has one valid value (multus) - we should drop it, as whatever it does can and should just be done by the openshift profile/platform: openshift.

I'd just s/values.cni.provider=multus/values.platform=openshift/g and drop cni.provider

fjglira · 2024-08-02T14:22:42Z

/release-notes-none

fjglira · 2024-08-02T14:28:53Z

/release-note-none

Signed-off-by: frherrer <frherrer@redhat.com>

* upstream/master: (67 commits) Add release notes for dual-stack support promotion (istio#52524) Automator: update proxy@master in istio/istio@master (istio#52515) add name (istio#52501) Allow setting resources to null in gateway chart (istio#52514) Fix replacements errors in helm charts and templates (istio#52459) hand written IstioEndpoint deepcopy (istio#52485) operator: remove `profile` commands (istio#52468) Automator: update proxy@master in istio/istio@master (istio#52506) Automator: update proxy@master in istio/istio@master (istio#52505) Automator: update proxy@master in istio/istio@master (istio#52504) Automator: update ztunnel@master in istio/istio@master (istio#52502) Fix typo of InsertDataToConfigMap func description (istio#52491) Fix stale GVK in benchmark test (istio#52482) Bug/stale cert expiration logs (istio#52466) Automator: update ztunnel@master in istio/istio@master (istio#52496) Automator: update proxy@master in istio/istio@master (istio#52495) operator: move proto from api to this repo (istio#52472) remove unneeded func isAllowedKubernetesAudience (istio#52489) operator: misc code cleanup (istio#52467) ambient: do not allow service waypoint to have a waypoint (istio#52480) ...

Initial changes to fix cni errors from replace flat values PR

b656387

Signed-off-by: frherrer <frherrer@redhat.com> Update and fix helm errors Signed-off-by: frherrer <frherrer@redhat.com>

fjglira requested review from a team as code owners August 1, 2024 14:56

istio-testing added size/M needs-ok-to-test labels Aug 1, 2024

howardjohn reviewed Aug 1, 2024

View reviewed changes

manifests/charts/istio-cni/templates/configmap-cni.yaml Outdated Show resolved Hide resolved

fjglira and others added 2 commits August 1, 2024 17:06

Update manifests/charts/istio-cni/templates/configmap-cni.yaml

2e328f8

Co-authored-by: John Howard <john.howard@solo.io>

Move again cni paramst under cni in helm charts templates

a2a63d7

Signed-off-by: frherrer <frherrer@redhat.com>

fjglira requested a review from howardjohn August 1, 2024 15:53

howardjohn reviewed Aug 1, 2024

View reviewed changes

Apply suggestions from code review

20d5779

Co-authored-by: John Howard <john.howard@solo.io>

howardjohn reviewed Aug 1, 2024

View reviewed changes

manifests/charts/istio-cni/templates/configmap-cni.yaml Outdated Show resolved Hide resolved

Update manifests/charts/istio-cni/templates/configmap-cni.yaml

ac7b9ba

Co-authored-by: John Howard <john.howard@solo.io>

howardjohn approved these changes Aug 1, 2024

View reviewed changes

istio-testing added ok-to-test and removed needs-ok-to-test labels Aug 1, 2024

Merge branch 'master' into fix-cni-flat-values

49abfa4

bleggett reviewed Aug 1, 2024

View reviewed changes

bleggett added the release-notes-none label Aug 2, 2024

Rollback to move pilot.cni outside pilot values in helm charts

2f5688e

Signed-off-by: frherrer <frherrer@redhat.com>

istio-testing merged commit 6fbfc2b into istio:master Aug 5, 2024
27 checks passed

fjglira mentioned this pull request Aug 5, 2024

Continue improving helm charts values #52520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix replacements errors in helm charts and templates #52459

Fix replacements errors in helm charts and templates #52459

fjglira commented Aug 1, 2024

istio-testing commented Aug 1, 2024

howardjohn left a comment

howardjohn Aug 1, 2024

howardjohn Aug 1, 2024

fjglira Aug 1, 2024

bleggett Aug 1, 2024 •

edited

Loading

fjglira Aug 2, 2024 •

edited

Loading

fjglira Aug 2, 2024

bleggett Aug 2, 2024 •

edited

Loading

fjglira Aug 2, 2024

bleggett Aug 2, 2024

fjglira Aug 5, 2024

howardjohn commented Aug 1, 2024

howardjohn commented Aug 1, 2024

bleggett Aug 1, 2024

bleggett Aug 1, 2024 •

edited

Loading

fjglira commented Aug 2, 2024

fjglira commented Aug 2, 2024

Fix replacements errors in helm charts and templates #52459

Fix replacements errors in helm charts and templates #52459

Conversation

fjglira commented Aug 1, 2024

istio-testing commented Aug 1, 2024

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

fjglira Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Aug 1, 2024

howardjohn commented Aug 1, 2024

Choose a reason for hiding this comment

bleggett Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

fjglira commented Aug 2, 2024

fjglira commented Aug 2, 2024

bleggett Aug 1, 2024 •

edited

Loading

fjglira Aug 2, 2024 •

edited

Loading

bleggett Aug 2, 2024 •

edited

Loading

bleggett Aug 1, 2024 •

edited

Loading