Adds acme CertificateRequest controller #1943

JoshVanL · 2019-07-30T17:42:15Z

Adds the ACME CertificateRequest controller to resolve CertificateRequests that have an ACME issuer references.

TODO:

e2e tests

/assign

Adds ACME CertificateRequest Controller.

jetstack-bot · 2019-07-30T17:42:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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:

~~OWNERS~~ [JoshVanL]

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

JoshVanL · 2019-08-12T17:14:55Z

Looking good
/unassign
/assign @munnerz

munnerz · 2019-08-12T18:18:47Z

pkg/controller/certificaterequests/acme/acme.go

+	controllerpkg.Register(CRControllerName, func(ctx *controllerpkg.Context) (controllerpkg.Interface, error) {
+		acme := NewACME(ctx)
+
+		orderInformer := ctx.SharedInformerFactory.Certmanager().V1alpha1().Orders().Informer()


Should we also have a 'watch' on Issuer resources? Or does the existing CertificateRequest controller shared implementation already handle this?

Indeed it does :)

pkg/controller/certificaterequests/acme/acme.go

pkg/controller/certificaterequests/acme/acme_test.go

munnerz · 2019-08-12T18:24:54Z

pkg/controller/certificaterequests/acme/acme_test.go

+			certificateRequest: testCR,
+			builder: &testpkg.Builder{
+				ExpectedEvents: []string{
+					`Normal ErrorGettingOrder Failed to get order resource default-unit-test-ns/test-cr-3958469914: this is a network error`,


Undecided whether network errors etc. should be bubbled up the events, as it could in turn place even more burden on the network. But let's think about that later 🙄

munnerz · 2019-08-12T18:25:22Z

pkg/controller/certificaterequests/acme/acme_test.go

+			certificateRequest: testCR,
+			builder: &testpkg.Builder{
+				ExpectedEvents: []string{
+					`Warning OrderFailed Failed to resolve order resource default-unit-test-ns/test-cr-3958469914: order is in failure state "invalid"`,


This message feels very mechanical for something that should be human-readable

pkg/controller/certificaterequests/acme/acme_test.go

pkg/controller/certificaterequests/controller.go

pkg/controller/certificaterequests/sync.go

JoshVanL · 2019-08-14T22:36:59Z

/hold

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL · 2019-08-15T16:23:31Z

@munnerz Good for another look
/unassign
/assign @munnerz
/hold cancel

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

munnerz · 2019-08-16T14:53:13Z

pkg/controller/certificaterequests/acme/acme.go

+		return nil, err
+	}
+
+	log = logf.WithResource(log, order)


This should be WithRelatedResource

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

munnerz · 2019-08-16T15:21:14Z

/lgtm

JoshVanL · 2019-08-16T17:33:19Z

/retest

munnerz · 2019-08-16T21:00:03Z

/test pull-cert-manager-e2e-v1-15

retest-bot · 2019-08-16T22:03:45Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 30, 2019

jetstack-bot assigned JoshVanL Jul 30, 2019

jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jul 30, 2019

jetstack-bot requested a review from munnerz July 30, 2019 17:42

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019

JoshVanL changed the title ~~Adds acme CertificateRequest controller~~ WIP: Adds acme CertificateRequest controller Jul 30, 2019

JoshVanL force-pushed the cr-acme branch from 6e00c8f to 618b36d Compare August 1, 2019 16:45

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2019

JoshVanL force-pushed the cr-acme branch from 618b36d to dfffe1e Compare August 7, 2019 15:39

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 7, 2019

JoshVanL force-pushed the cr-acme branch 3 times, most recently from ee1928f to f4a6f2f Compare August 12, 2019 13:18

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2019

jetstack-bot assigned munnerz and unassigned JoshVanL Aug 12, 2019

JoshVanL changed the title ~~WIP: Adds acme CertificateRequest controller~~ Adds acme CertificateRequest controller Aug 12, 2019

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2019

munnerz reviewed Aug 12, 2019

View reviewed changes

jetstack-bot assigned munnerz Aug 13, 2019

JoshVanL force-pushed the cr-acme branch 2 times, most recently from 4430543 to 460f271 Compare August 14, 2019 10:29

JoshVanL added 11 commits August 15, 2019 17:18

Adds acme CertificateRequest controller with unit tests

e2b9b21

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Addes dns e2e test for acme CertificateRequest

b28d444

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Updates acme to use new reporter

eabdf04

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Add extra informers to cr controller struct on New

b918887

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds CR acme e2e tests for http

06d02a7

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Move fake order lister to ./test/unit/listers

0f08f40

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Remove Failed reason from invalid acme URL

393f7c2

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds CertificateRequest to condition matcher in e2e tests

277dc7a

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Improve event wording and some code nesting

64b2dde

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Update the ACME CR controller to use the new reporter signature

1040261

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Fix test package name and acme tests

7869f99

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the cr-acme branch from 460f271 to 97059b7 Compare August 15, 2019 16:19

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2019

Updates acme CR controller tests to include controller sync

5ef9700

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the cr-acme branch from 97059b7 to 5ef9700 Compare August 15, 2019 17:24

munnerz reviewed Aug 16, 2019

View reviewed changes

Changes WithResource -> WithRelatedResource

eb6e38a

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2019

jetstack-bot merged commit 8ab4dc1 into cert-manager:master Aug 16, 2019

jetstack-bot added this to the v0.10 milestone Aug 16, 2019

munnerz added this to Done in v0.10 Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds acme CertificateRequest controller #1943

Adds acme CertificateRequest controller #1943

JoshVanL commented Jul 30, 2019 •

edited

Loading

jetstack-bot commented Jul 30, 2019

JoshVanL commented Aug 12, 2019

munnerz Aug 12, 2019

JoshVanL Aug 13, 2019

munnerz Aug 12, 2019

munnerz Aug 12, 2019

JoshVanL commented Aug 14, 2019

JoshVanL commented Aug 15, 2019

munnerz Aug 16, 2019

munnerz commented Aug 16, 2019

JoshVanL commented Aug 16, 2019

munnerz commented Aug 16, 2019

retest-bot commented Aug 16, 2019

Adds acme CertificateRequest controller #1943

Adds acme CertificateRequest controller #1943

Conversation

JoshVanL commented Jul 30, 2019 • edited Loading

jetstack-bot commented Jul 30, 2019

JoshVanL commented Aug 12, 2019

munnerz Aug 12, 2019

Choose a reason for hiding this comment

JoshVanL Aug 13, 2019

Choose a reason for hiding this comment

munnerz Aug 12, 2019

Choose a reason for hiding this comment

munnerz Aug 12, 2019

Choose a reason for hiding this comment

JoshVanL commented Aug 14, 2019

JoshVanL commented Aug 15, 2019

munnerz Aug 16, 2019

Choose a reason for hiding this comment

munnerz commented Aug 16, 2019

JoshVanL commented Aug 16, 2019

munnerz commented Aug 16, 2019

retest-bot commented Aug 16, 2019

JoshVanL commented Jul 30, 2019 •

edited

Loading