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 'solvers' configuration to ACME Issuer #1450

Merged
merged 19 commits into from
May 1, 2019

Conversation

munnerz
Copy link
Member

@munnerz munnerz commented Mar 7, 2019

What this PR does / why we need it:

This is a very WIP first pass at moving the ACME certificate config over to the Issuer resource in a backwards compatible manner.

We'll remove support for the old syntax at {some point} (either pre-1.0, or for 1.0).

Mostly pushing this up to see if the e2e tests (which have not changed) will pass 😄

A (very verbose) example of an Issuer using a mixture of new and old configuration, and also including an example of how the 'webhook' code in #1443 can be adapted for HTTP01:

apiVersion: certmanager.k8s.io/v1alpha1
kind: Issuer
metadata:
  name: letsencrypt-staging
spec:
  acme:
    server: ...
    privateKeySecretRef:
      name: le-pk
    solvers:
    # use dns01 by default
    - selector: {} # match all certificates
      dns01:
        # TODO: should we take this opp. to remove support for 'core' solvers?
        webhook:
          groupName: solvers.acme.cert-manager.io
          solverName: cloudflare
          config:
            email: james@munnelly.eu
            apiKeySecretRef:
              name: cloudflare-api-key
              key: api-key
    - selector:
        matchLabels:
          "use-acme-http01-nginx": "true"
      http01:
        webhook:
          groupName: solvers.acme.cert-manager.io
          solverName: ingress
          config:
            ingressClass: nginx
    - selector:
        matchLabels:
          "use-acme-http01-contour": "true"
      http01:
        webhook:
          groupName: solvers.acme.cert-manager.io
          solverName: contour
          config:
            # config required for contour http01 solver,
            # e.g. name of IngressRoute
            ingressRouteName: my-root-ingress-route
    # Example showing backwards compatibility with non-webhook types
    # TODO: do we want to ever support the old format here?
    - selector:
        matchLabels:
          "use-acme-http01-nginx-non-webhook-format": "true"
      http01:
        ingressClass: "nginx"
    dns01:
      providers:
      - name: "old-legacy-format-provider"
        cloudflare:
          email: james@munnelly.eu
          apiKeySecretRef:
            name: cloudflare-api-key
            key: api-key
    http01:
      serviceType: NodePort

Which issue this PR fixes: fixes #1342

Special notes for your reviewer:

Release note:

TODO

/cc @DanielMorsing

@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2019
@jetstack-bot jetstack-bot added area/acme Indicates a PR directly modifies the ACME Issuer code dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme/dns01 Indicates a PR modifies ACME DNS01 provider code release-note Denotes a PR that will be considered when it comes time to generate release notes. area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory kind/documentation Categorizes issue or PR as related to documentation. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 7, 2019
@jetstack-bot jetstack-bot added the area/deploy Indicates a PR modifies deployment configuration label Mar 7, 2019
@munnerz munnerz force-pushed the acme-config-on-issuer branch 3 times, most recently from 1e711b5 to a74b005 Compare March 7, 2019 16:53
@munnerz munnerz added this to the v0.8 milestone Mar 7, 2019
@munnerz munnerz added this to In progress in v0.8 Mar 7, 2019
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2019
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 27, 2019
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Apr 29, 2019
// List of DNSNames that can be used to further refine the domains that
// this solver applies to.
// +optional
DNSNames []string `json:"dnsNames,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @Evesy this field currently only supports exact matches, FYI

IngressClass *string `json:"ingressClass,omitempty"`

// +optional
IngressName string `json:"ingressName,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @kragniz should these two fields be Class and Name to avoid the stutter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to Name and Class 😄

@munnerz munnerz force-pushed the acme-config-on-issuer branch 2 times, most recently from d11aae2 to 70f0d8c Compare April 30, 2019 11:24
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
Copy link
Contributor

@kragniz kragniz left a comment

Choose a reason for hiding this comment

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

One question and a couple of suggestions, happy to merge

docs/tasks/acme/configuring-dns01/index.rst Outdated Show resolved Hide resolved
docs/tasks/acme/configuring-http01.rst Outdated Show resolved Hide resolved
pkg/controller/acmeorders/sync.go Outdated Show resolved Hide resolved
pkg/controller/acmeorders/sync.go Outdated Show resolved Hide resolved
Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: James Munnelly <james@munnelly.eu>
v0.8 automation moved this from In progress to Reviewer approved May 1, 2019
Copy link
Contributor

@kragniz kragniz left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kragniz, munnerz

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

The pull request process is described 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 merged commit e0474fb into cert-manager:master May 1, 2019
v0.8 automation moved this from Reviewer approved to Done May 1, 2019
@munnerz munnerz deleted the acme-config-on-issuer branch May 1, 2019 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/acme/dns01 Indicates a PR modifies ACME DNS01 provider code area/acme/http01 Indicates a PR modifies ACME HTTP01 provider code area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing 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. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
No open projects
v0.8
  
Done
Development

Successfully merging this pull request may close these issues.

Moving Certificate's ACME config to the Issuer resource
3 participants