fix: make handling of additionalAnnotations more robust

[jrhoward]

…[email protected]

The helm chart interprets more complex additionalAnnotation incorrectly and will not render charts.

See linked issue for more detail:

#534

In summary this PR ensures annotation values passed to the csr agent are safe and ensures bootstrap Certificate annotation values are interpreted as a string.

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thatsmrtalbot for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cert-manager-prow]

Hi @jrhoward. Thanks for your PR.

I'm waiting for a cert-manager 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.

[jrhoward]

would some one be able to look at this please? it's a simple change to the helm charts @Alkemic @james-w @marcingy @munnerz

[sitaramkm]

@jrhoward
The better model to support your use-case would be configure istio-csr with a CA issuer so certs for mesh workloads can be issued locally.
Injecting custom-field annotations into CertificateRequest implies that you are looking at having Venafi issue all certificates for mesh workloads. Managing & maintaining an inventory for large number of workload certs is not of much value. As you probably already know, we do not create a Certificate or a Secret resource when istio-csr signs incoming mesh workloads. In fact, our recommendation is to turn off preserveCertificateRequests for the mesh as it's typically cert with a short duration and are very often renewed.

I agree that the PR itself is simple but the supporting usecase is not something I would recommend (that is assuming I am understanding the underlying usecase correctly)
My recommendation if you plan to use open-source components

Create a CA issuer with Venafi as your issuer. As you will create this with a Certificate resource you can have all the annotations that Venafi expects.
Configure this CA issuer in istio-csr. All mesh workloads will anchor back to the above CA issuer and it's root.
You can also use policy-approver and create a few CertificateRequestPolicy resources if you want to constrain them to specific issuer/namespace, etc.

[jrhoward]

Thanks @sitaramkm , have taken your advice on board and will be looking to change our strategy, would still be good to get this merged as it makes the passing of cli arguments to the CSR agent safer.

[jrhoward]

Hi folks, would it be possible to get this reviewed ?

[erikgb]

/ok-to-test

[Copilot]

Pull Request Overview

This PR fixes issues with handling complex additionalAnnotations in the istio-csr Helm chart by ensuring annotation values are properly quoted and formatted to prevent chart rendering failures.

• Updates command-line argument formatting to use proper quoting for annotation values
• Changes certificate annotation value handling to use YAML literal block format instead of inline values

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
deploy/charts/istio-csr/templates/deployment.yaml	Updates command-line argument formatting to properly quote annotation values using printf and quote functions
deploy/charts/istio-csr/templates/certificate.yaml	Changes annotation value rendering from inline to YAML literal block format with proper indentation

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The literal block scalar format with | may not be appropriate for all annotation values. Consider using {{ $annotation.value | toYaml }} instead, which will properly handle strings, numbers, booleans, and complex values while maintaining YAML compliance.

Suggested change

	{{ $annotation.name }}: |
	{{ $annotation.value | indent 6 }}
	{{ $annotation.name }}: {{ $annotation.value | toYaml | nindent 6 }}

[cert-manager-prow]

@jrhoward: 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
pull-cert-manager-istio-csr-istio-v1-24	503370f	link	true	/test pull-cert-manager-istio-csr-istio-v1-24
pull-cert-manager-istio-csr-istio-ambient	503370f	link	true	/test pull-cert-manager-istio-csr-istio-ambient
pull-cert-manager-istio-csr-istio-v1-23	503370f	link	true	/test pull-cert-manager-istio-csr-istio-v1-23
pull-cert-manager-istio-csr-istio-v1-21	503370f	link	true	/test pull-cert-manager-istio-csr-istio-v1-21

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.