Skip to content
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 support for specifying ACME HTTP01 ingress name as an annotation on Certificates #1880

Closed
wants to merge 3 commits into from

Conversation

cheukwing
Copy link
Contributor

What this PR does / why we need it:
Adds support for an annotation on Certificates http01.acme.certmanager.k8s.io/ingress-to-edit to manually specify the ingress to edit, to overcome the need to have access to adding a specific solver entry in the Issuer.
A new field AllowManuallySpecifiedIngress is available for Issuer solvers to enable this annotation to be used, then if the annotation is set the solver will be prioritized.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1666

Special notes for your reviewer:
Re-do of #1762 after the large changes in the order controller

Release note:

Adds support for Certificate annotation `http01.acme.certmanager.k8s.io/ingress-to-edit` to manually specify the Ingress to edit, when the option is enabled under `AllowManuallySpecifiedIngress` in one of the Issuer's solvers.

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme Indicates a PR directly modifies the ACME Issuer code labels Jul 10, 2019
@jetstack-bot jetstack-bot added the area/api Indicates a PR directly modifies the 'pkg/apis' directory label Jul 10, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cheukwing
To complete the pull request process, please assign munnerz
You can assign the PR to them by writing /assign @munnerz in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added kind/documentation Categorizes issue or PR as related to documentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2019

if ingress.AllowManuallySpecifiedIngress {
if numIngressSpecifiers > 0 {
el = append(el, field.Forbidden(fldPath.Child("allowManuallySpecifiedIngress"), "may not specify more than one of class, name, or allowManuallySpecifiedIngress"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this in the original PR but I'm not sure if we require this, e.g. we could put a fallback Name or Class if this solver is selected but we do not have the annotation?

selectSolver()
break
}

Copy link
Contributor Author

@cheukwing cheukwing Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this point on wards, we continue as before but this could lead to problems if the solver which has AllowManuallySelectedIngress is the most specific solver and we have not set the annotation, since we would end up with no final ingress - I'm not sure if this is something we should handle (i.e. prevent choosing this solver if annotation not set) or something the user has to handle themselves (maybe some more validation or some additional docs?).

The previous review comment also stands here.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
…tion set, with tests

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019
@munnerz munnerz added this to the v0.10 milestone Jul 23, 2019
@munnerz munnerz modified the milestones: v0.10, v0.11 Sep 10, 2019
@munnerz
Copy link
Member

munnerz commented Sep 10, 2019

Closing in favour of #2061

@munnerz munnerz closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/documentation Categorizes issue or PR as related to documentation. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support specifying ACME HTTP01 ingress name as an annotation on Certificates
3 participants