Skip to content

Add Venafi custom field support to cert-shim#6146

Closed
k0da wants to merge 1 commit into
cert-manager:masterfrom
k0da:venafi
Closed

Add Venafi custom field support to cert-shim#6146
k0da wants to merge 1 commit into
cert-manager:masterfrom
k0da:venafi

Conversation

@k0da

@k0da k0da commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

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

NONE

@k0da
Add Venafi custom field support to cert-shim
fb8c3c0 
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>
@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 13, 2023
@jetstack-bot

jetstack-bot commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

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 /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.

Details

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.

@jetstack-bot jetstack-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 13, 2023
@k0da

k0da commented Jun 16, 2023

Copy link
Copy Markdown
Contributor Author

/cc @inteon
HI, @inteon could you please review?

@jetstack-bot jetstack-bot requested a review from inteon June 16, 2023 09:14
@k0da

k0da commented Jun 16, 2023

Copy link
Copy Markdown
Contributor Author

/cc @wallrj

@jetstack-bot jetstack-bot requested a review from wallrj June 16, 2023 09:15
@inteon

inteon commented Jun 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

/hold
I don't think we should be reading issuer-specific annotations for the certificate shim, what do you think @wallrj.
Ideally we have a more general solution for this.

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2023
@k0da

k0da commented Jun 16, 2023

Copy link
Copy Markdown
Contributor Author

@inteon I thought first to get all annotations as is. But then decided to narrow down a change to a specific usecase.

@inteon

inteon commented Jun 22, 2023

Copy link
Copy Markdown
Member

@inteon I thought first to get all annotations as is. But then decided to narrow down a change to a specific use case.

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).

wallrj
wallrj requested changes Jun 23, 2023
View reviewed changes

@wallrj wallrj left a comment

Copy link
Copy Markdown
Member

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:

  1. 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.
  2. However I can see two arguments in favour of this change:
    1. 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.
    2. 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.
    3. 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.

@k0da

k0da commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

@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.

@inteon

inteon commented Jul 14, 2023

Copy link
Copy Markdown
Member

@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.

@jetstack-bot

jetstack-bot commented Oct 12, 2023

Copy link
Copy Markdown
Contributor

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.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 12, 2023
@jetstack-bot

jetstack-bot commented Nov 11, 2023

Copy link
Copy Markdown
Contributor

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.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

@jetstack-bot jetstack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 11, 2023
@jetstack-bot

jetstack-bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

@jetstack-bot

jetstack-bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

@jetstack-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

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.

@inteon

inteon commented Dec 11, 2023

Copy link
Copy Markdown
Member

/remove-lifecycle rotten
/reopen

@jetstack-bot jetstack-bot reopened this Dec 11, 2023
@jetstack-bot

jetstack-bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

@inteon: Reopened this PR.

Details

In response to this:

/remove-lifecycle rotten
/reopen

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.

@jetstack-bot jetstack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 11, 2023
@jetstack-bot

jetstack-bot commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

[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 irbekrm for approval. For more information see the Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@jetstack-bot

jetstack-bot commented Mar 10, 2024

Copy link
Copy Markdown
Contributor

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

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 10, 2024
@hawksight

hawksight commented Mar 11, 2024

Copy link
Copy Markdown
Member

/remove-lifecycle stale

@jetstack-bot jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 11, 2024
@jetstack-bot

jetstack-bot commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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.

@k0da k0da closed this Jun 9, 2024
@k0da k0da mentioned this pull request Jun 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wallrj wallrj wallrj requested changes

@inteon inteon Awaiting requested review from inteon

Assignees

No one assigned

Labels

dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@k0da @jetstack-bot @inteon @hawksight @wallrj