-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Venafi custom field support to cert-shim #6146
Conversation
Some setups have a requirement to have custom-field annotation set on Certificate object. This commit adds support for scraping Venafi custom-fields annotation from ingressLike objects and pass them to Certificate object. Signed-off-by: Dinar Valeev <k0da@opensuse.org>
Hi @k0da. 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 Once the patch is verified, the new status will be reflected by the 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. |
/cc @wallrj |
/hold |
@inteon I thought first to get all annotations as is. But then decided to narrow down a change to a specific usecase. |
I think this is the best way forward, we would still have to filter the annotations (eg. only copy annotations with a specific prefix). It would be great if we could create something that aligns with our existing annotation-copying solutions (haven't had the time to investigate this yet). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this PR in our Wednesday standup and my contributions to the discussion were:
- I was initially opposed to adding more annotations to ingress-shim because we will end up having to create an annotation for every field of the Certificate, which is a maintenance burden. 1. Instead users with special Certificate requirements should not use ingress-shim and should pre-provision a Certificate resource with their required settings.
- However I can see two arguments in favour of this change:
- Some platform engineers prefer Ingress and ingress-shim because they don't want application engineers to have to learn about and use the cert-manager custom resources directly.
- By configuring Ingress certificates using only annotations, the application manifests do not have a dependency on the cert-manager CRDs so applications and cert-manager can be installed in any order. Although @hawksight argued that cert-manager is necessarily part of the cluster platform and will usually be deployed in an earlier phase than the applications.
- I was conflating this PR with previous PRs which have introduced Ingress annotations for Certificate.Spec fields. This PR is about copying Certificate annotations, so I guess it seems reasonable that all the supported Certificate annotations can also be added to an Ingress and copied to the generated Certificate by ingress-shim.
@k0da Please also create a cert-manager website PR, documenting how this new feature will work and link it to this PR.
@wallrj Thanks for the review. Will do website change. I have an implementation question though, should we take all annotations as is, like we do for Labels given Certificate is created out of IngLike object I feel natural to take them as is. |
I'm not sure I understand your question. |
Issues go stale after 90d of inactivity. |
Stale issues rot after 30d of inactivity. |
Rotten issues close after 30d of inactivity. |
@jetstack-bot: Closed this PR. In response to this:
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. |
/remove-lifecycle rotten |
@inteon: Reopened this PR. In response to this:
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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issues go stale after 90d of inactivity. |
/remove-lifecycle stale |
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. |
Some setups have a requirement to have custom-field annotation set on Certificate object.
This commit adds support for scraping Venafi custom-fields annotation from ingressLike objects and pass them to Certificate object.
Pull Request Motivation
This PR adds support for Venafi custom-field annotation from ingLike resources
Kind
Feature
Release Note