Add helm values for annotations

[thesuperzapper]

closes #175

This PR adds two new values to the helm chart:

• app.deploymentAnnotations: annotations to be applied to the trust-manager Deployment resource
• app.certificateAnnotations: annotations to be applied to the Certificate and Issuer resources

This is primarily so that users can create a combined chart with cert-manager, and use ArgoCD to prevent race conditions.

For example, here are the values I am using in deployKF for this purpose:

app:
  certificateAnnotations:
    ## we must sync cert-manager resources after cert-manager itself
    argocd.argoproj.io/sync-wave: "10"

  deploymentAnnotations:
    ## we must sync the trust-manager deployment after its certificate is ready
    argocd.argoproj.io/sync-wave: "20"

[jetstack-bot]

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

[thesuperzapper]

@inteon I would appreciate a quick look at this small change to the helm chart, as it will let me stop forking the trust-manager helm chart for use in deployKF.

[erikgb]

@thesuperzapper Have you seen #157? Being able to cut the dependency to cert-manager from trust-manager makes a lot of sense.

We should probably include the feature of adding annotations anyway, but do you really need to annotate the trust-manager deployment to solve your use case? Since the deployment mount the certificate secret as a volume mount, I don't think the pods will be scheduled before the secret is created by cert-manager.

I am also a bit torn regarding the proposed app.certificateAnnotations value. The certificate is more related to the webhook IMO, and we have a webhook hierarchy in the Helm chart (mess) also. WDYT?

/ok-to-test
/lgtm
/cc @inteon @SgtCoDFish

[erikgb]

Please run 'make -s update-helm-docs'

To fix CI

[SgtCoDFish]

Could you provide a little more detail on why you want this? We discussed it a bit and we guessed that with a combined chart you're running into issues with the CRDs not being installed when the trust-manager Certificate is created?

[jetstack-bot]

New changes are detected. LGTM label has been removed.

[jetstack-bot]

[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 wallrj for approval. For more information see the Kubernetes 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

[thesuperzapper]

Could you provide a little more detail on why you want this? We discussed it a bit and we guessed that with a combined chart you're running into issues with the CRDs not being installed when the trust-manager Certificate is created?

@SgtCoDFish the issue is not about the CRDs themselves being installed, it's about race conditions between the four "phases" of a combined cert-manager + trust-manager chart:

1. cert-manager
2. Cert Manager Certificates (depend on cert-manager webhook)
3. trust-manager (mounts certificate secrets, for its webhook)
4. Trust Manager Bundles (depend on trust-manager webhook)

ArgoCD supports the idea of "sync waves" which allows us to resolve this race condition by appropriately annotating the resources that depend on each other into separate waves.

[SgtCoDFish]

Thanks for explaining! I've gotta say I'm kinda uneasy about merging this for what feels like such a specific Argo thing - but I think that's probably because I'm just generally kinda uneasy about Helm 😂

I've left a comment - do you think you could make that change?

(Note I'm finishing for the day now, so I might not be able to re-review until tomorrow)

[SgtCoDFish]

nitpick: Looking to the future I'd like to remove the dependency on cert-manager that trust-manager currently has. Having app.certificateAnnotations doesn't really fit with a future chart where we donn't have any certificates or issuers.

Could we move this under app.webhook.tls.certManager.annotations? That way it's clear that it's for all certManager resources (certs + issuers) and it's moved to a more specific area of the chart.

[jetstack-bot]

PR needs rebase.

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.

[cert-manager-prow]

@thesuperzapper: 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-trust-manager-test	ddfc6a6	link	true	/test pull-trust-manager-test
pull-trust-manager-integration	ddfc6a6	link	true	/test pull-trust-manager-integration

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.

[cert-manager-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

[cert-manager-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
/lifecycle rotten
/remove-lifecycle stale

[thesuperzapper]

I think this issue is still important, and is causing me to run a forked version of the chart.