Custom labels added for istio-cni deployment and pods in helm chart

[nischay30]

This PR adds the capability in helm chart for adding custom labels to istio-cni deployments ands pods and aims at this Issue - #50372

[linux-foundation-easycla]

• ✅ login: nischay30 / name: Nischay Goyal (83a321b, e5c5396, a7b9fee, 8175e37, d6dadb5, 99cd62f, 7b54ca3, b16c562, 85f993a, 7e31af3)
• ✅ login: dgn / name: Daniel Grimm (85f993a)
• ❌ The email address for the commit (7e31af3) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

[istio-policy-bot]

😊 Welcome @nischay30! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @nischay30. 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/test-infra repository.

[howardjohn]

This isn't a Deployment so the naming is kind of weird?

[nischay30]

That was a typo. Fixed it

[costinm]

I was about to open a PR - can you add the "app: istio-cni" label as well ?

It is very annoying that all other charts have this label - but this one uses k8s-app instead ( and I don't know what "-node" suffix means - all other charts use the "app: chart_name")

[costinm]

And it may be useful to provide an example of 'when you need them' - by the criteria that you may need something we would replicate the entire Pod spec in values.yaml...

I know there is a pattern of redundant docs in most of the helm charts, but at least for new stuff it may be nice to have at least an example use case.

[nischay30]

I was about to open a PR - can you add the "app: istio-cni" label as well ?

It is very annoying that all other charts have this label - but this one uses k8s-app instead ( and I don't know what "-node" suffix means - all other charts use the "app: chart_name")

The k8s-app label is present in spec.selector.matchLabels, which is immutable field. By replacing, it will become a breaking change, where first it needs to be uninstalled.

[nischay30]

And it may be useful to provide an example of 'when you need them' - by the criteria that you may need something we would replicate the entire Pod spec in values.yaml...

I know there is a pattern of redundant docs in most of the helm charts, but at least for new stuff it may be nice to have at least an example use case.

Do you have any example? any format? where we can add? because custom labels isn't a big thing

[costinm]

Not change - add. There is no harm in adding an extra label.

…

On Mon, Apr 29, 2024, 03:45 Nischay Goyal ***@***.***> wrote: I was about to open a PR - can you add the "app: istio-cni" label as well ? It is very annoying that all other charts have this label - but this one uses k8s-app instead ( and I don't know what "-node" suffix means - all other charts use the "app: chart_name") The k8s-app label is present in spec.selector.matchLabels, which is immutable field. By replacing, it will become a breaking change, where first it needs to be uninstalled. — Reply to this email directly, view it on GitHub <#50694 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2XRRTNNQ7TX6YIOYJLY7YQDDAVCNFSM6AAAAABG2QGFCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGQYDCNBZGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

[costinm]

Don't worry about 'app', I can open a separate PR. For comments - just some example like what label you are planning to use, I assume this PR is based on some use case you have.

…

On Mon, Apr 29, 2024, 15:50 Costin Manolache ***@***.***> wrote: Not change - add. There is no harm in adding an extra label. On Mon, Apr 29, 2024, 03:45 Nischay Goyal ***@***.***> wrote: > I was about to open a PR - can you add the "app: istio-cni" label as well > ? > > It is very annoying that all other charts have this label - but this one > uses k8s-app instead ( and I don't know what "-node" suffix means - all > other charts use the "app: chart_name") > > The k8s-app label is present in spec.selector.matchLabels, which is > immutable field. By replacing, it will become a breaking change, where > first it needs to be uninstalled. > > — > Reply to this email directly, view it on GitHub > <#50694 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2XRRTNNQ7TX6YIOYJLY7YQDDAVCNFSM6AAAAABG2QGFCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBSGQYDCNBZGY> > . > You are receiving this because your review was requested.Message ID: > ***@***.***> >

[linsun]

Can you add a release note file?

Also explain why use daemonset vs pod level label for istio-cni?

cc @bleggett

[bleggett]

Can you add a release note file?

Also explain why use daemonset vs pod level label for istio-cni?

cc @bleggett

It's good practice to have both, like we already do in the ztunnel daemonset chart.

My biggest beef with the istio-cni chart is the use of the cni prefix for all the values, which is an artifact of istioctl and very non-helm.

[costinm]

On Thu, May 2, 2024 at 9:02 AM Ben Leggett ***@***.***> wrote: Can you add a release note file? Also explain why use daemonset vs pod level label for istio-cni? cc @bleggett <https://github.com/bleggett> It's good practice to have both, like we already do in the ztunnel daemonset chart. My biggest beef with the istio-cni chart is the use of the cni prefix for all the values, which is an artifact of istioctl and very non-helm

+1 There is an opportunity to cleanup a bit - add new things at top level, add "public / stable" things to top level, etc.

[nischay30]

Can you add a release note file?

Also explain why use daemonset vs pod level label for istio-cni?

cc @bleggett

I have changed to labels as discussed above. Also, how to add release-notes file?

[bleggett]

Can you add a release note file?
Also explain why use daemonset vs pod level label for istio-cni?
cc @bleggett

I have changed to labels as discussed above. Also, how to add release-notes file?

Check istio/releasenotes/README.md

[dgn]

generally lgtm, just one small comment regarding the release note

[costinm]

> + # Custom labels on Daemonset level, for eg - FinOps/teams labels + labels: {} + # team-owner: finops + + # Custom labels on Pod level, for eg - FinOps/teams labels + podLabels: {} + # team-owner: finops + Minor nit - don't need specific examples.

I asked for examples and use cases - but I agree 'team-owner' is not the right example for CNI or ztunnel, it's not a regular pod. I still don't understand the use case, but since all other helm charts we have contain similar customisation I don't mind this, was just hoping to understand.

[bleggett]

• Custom labels on Daemonset level, for eg - FinOps/teams labels + labels: {} + # team-owner: finops + + # Custom labels on Pod level, for eg - FinOps/teams labels + podLabels: {} + # team-owner: finops + Minor nit - don't need specific examples.

I asked for examples and use cases - but I agree 'team-owner' is not the right example for CNI or ztunnel, it's not a regular pod. I still don't understand the use case, but since all other helm charts we have contain similar customisation I don't mind this, was just hoping to understand.

I'll leave it to you then, I missed you had asked for it, it just looks weird since we don't do that in other spots and people might not know what finops is.

[nischay30]

What is blocking to get this merged?

[costinm]

Not sure.

[hanxiaop]

Can you rebase?

/ok-to-test

[istio-testing]

@nischay30: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
unit-tests-arm64_istio	b16c562	link	true	/test unit-tests-arm64
gencheck_istio	b16c562	link	true	/test gencheck
unit-tests_istio	b16c562	link	true	/test unit-tests

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. I understand the commands that are listed here.