3879 test acme issuer setup #3938

irbekrm · 2021-04-29T09:27:50Z

This PR adds unit tests for ACME issuer's Setup function.

This follows #3877 , see the comment about testing here .

Some notes:

This test is not a particularly good unit test- all the methods of Acme struct should be refactored to make them more testable. However, maybe it would be better if we merged (if we are going to at all) Remove /pkg/issuer in favour of individual controllers. #3695 first. Also maybe already having some unit test in place would make refactoring easier.
We usually unit test using test builder, which helps with mocking requests to kubernetes API server. It did not seem to make sense to use the test builder here because Setup does not have access to controller's context etc. This made it difficult to, for example, properly mock requests to create/list secrets. Again, refactoring Setup could help with this.

This PR slightly refactors the Setup function:

pulls out some calls to external dependencies, so they can be mocked in tests
turns a bunch of string literals into constants:
moves the updating of issuer's Ready condition to a deferred function here. As a side-effect of this change issuer's observed generation is updated every time the issuer gets synced- at the moment it ends up in incorrect state when the Setup function is called due to issuer's spec having changed and we hit this code path. (I have seen this when modifying issuer.spec.acme.externalAccountBinding for an already valid issuer). Perhaps this should have been a separate PR?

I think that the easiest way to code-review this PR might be to read through the test cases top to bottom and look at the Setup function's flow in parallel as the test cases are mostly ordered to follow the function's flow (/cc @wallrj ). The test does not cover all the code paths- I think that perhaps more thorough testing could be done after merging this PR when we are in a better position to refactor this function?

/kind cleanup

irbekrm · 2021-04-29T09:45:54Z

/release-note-none

irbekrm · 2021-04-29T09:59:11Z

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

Allow the functionality to set up a new ACME client and to retrieve and decode ACME account's key to be stubbed in tests Signed-off-by: irbekrm <irbekrm@gmail.com>

…erals This commit also ensures that issuer's observed generation is updated in cases where the issuer spec has changed, but the re-registration is skipped as the current registration seems already valid Signed-off-by: irbekrm <irbekrm@gmail.com>

….Registry Signed-off-by: irbekrm <irbekrm@gmail.com>

A simpler implementation than https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/fake/fake_secret.go and more suited for unit tests that don't spin up a controller Signed-off-by: irbekrm <irbekrm@gmail.com>

So they can be generated in tests with less lines of code Signed-off-by: irbekrm <irbekrm@gmail.com>

Signed-off-by: irbekrm <irbekrm@gmail.com>

wallrj

Thanks @irbekrm and sorry for delay.
Here's a partial review....more to follow.

pkg/issuer/acme/setup.go

wallrj · 2021-05-12T08:52:32Z

pkg/issuer/acme/setup.go

-		apiutil.SetIssuerCondition(a.issuer, a.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, "InvalidConfig",
-			fmt.Sprintf("Your ACME server URL is set to a v1 endpoint (%s). "+
-				"You should update the spec.acme.server field to %q", a.issuer.GetSpec().ACME.Server, newURL))
+		reason = errorInvalidConfig
+		msg = fmt.Sprintf(messageTemplateUpdateToV2, a.issuer.GetSpec().ACME.Server, newURL)


Idea: Should we / do we have API validation to prevent people using these v1 endpoints? If not, perhaps we should.

pkg/issuer/acme/setup_test.go

wallrj

Thanks again @irbekrm

Here are some further remarks and suggestions which you can address or answer as you please.

I note there are still one or two areas of untested code in the Setup function, but the new coverage is great.

acme Go Coverage Report.pdf

pkg/issuer/acme/setup.go

pkg/issuer/acme/setup_test.go

irbekrm · 2021-05-13T10:04:19Z

/test pull-cert-manager-e2e-v1-20-feature-issuers-venafi-tpp

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm · 2021-05-14T11:53:55Z

Thank you very much for the code review @wallrj !

I have tried to address all the comments, in some cases by creating new issues.

Thank you for the coverage, this is very useful. I have added one new test case and created #4006 for testing the functionality for updating ACME account email.
The other sections that aren't covered by tests are namespace checking (I think perhaps the namespace should be set on Acme in caller), removal of ACME URI from Issuer status (this seems to be an intermittent step, not sure if there is a value in testing it) and some functionality in the helper functions (I think there needs to be a refactor of Setup which may change how helpers behave, not sure if it's worth testing them in depth in this test).

wallrj

Thanks for all those followup issues and extra comments etc

/lgtm

jetstack-bot · 2021-05-14T12:35:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, wallrj

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~~ [irbekrm,wallrj]

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

jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 29, 2021

irbekrm requested a review from wallrj April 29, 2021 13:13

irbekrm assigned JoshVanL and unassigned JoshVanL Apr 29, 2021

irbekrm requested a review from JoshVanL April 29, 2021 13:14

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2021

irbekrm added 7 commits May 10, 2021 09:51

Remove direct calls to external deps from Setup function

d8367cb

Allow the functionality to set up a new ACME client and to retrieve and decode ACME account's key to be stubbed in tests Signed-off-by: irbekrm <irbekrm@gmail.com>

Fix FakeRegistry.AddClient + ensure that FakeACME implements accounts…

c97b14a

….Registry Signed-off-by: irbekrm <irbekrm@gmail.com>

Adds a fake Secrets client

6318de5

A simpler implementation than https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/fake/fake_secret.go and more suited for unit tests that don't spin up a controller Signed-off-by: irbekrm <irbekrm@gmail.com>

Adds functionality to generate issuer conditions to gen

0c751f5

So they can be generated in tests with less lines of code Signed-off-by: irbekrm <irbekrm@gmail.com>

Adds a few ACME-specific functions to issuer gen

284de09

Signed-off-by: irbekrm <irbekrm@gmail.com>

Adds a unit test for ACME issuer Setup function

e82ea35

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the 3879_test_acme_issuer_setup branch from e0613c3 to e82ea35 Compare May 10, 2021 08:54

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2021

wallrj self-assigned this May 12, 2021

wallrj reviewed May 12, 2021

View reviewed changes

wallrj requested changes May 12, 2021

View reviewed changes

wallrj removed their assignment May 12, 2021

wallrj mentioned this pull request May 12, 2021

2686 fixes stuck Orders issue #3805

Merged

irbekrm mentioned this pull request May 13, 2021

Re-enable e2e tests that connect to Venafi TPP. #4000

Merged

irbekrm force-pushed the 3879_test_acme_issuer_setup branch from 3e4ac47 to c55f81b Compare May 14, 2021 09:23

irbekrm mentioned this pull request May 14, 2021

Manage ACME accounts registry better #4005

Closed

Implement feedback from code review

9ecf896

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the 3879_test_acme_issuer_setup branch from c55f81b to 9ecf896 Compare May 14, 2021 11:41

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 May 14, 2021

irbekrm mentioned this pull request May 14, 2021

Test ACME account email update #4006

Closed

wallrj approved these changes May 14, 2021

View reviewed changes

jetstack-bot assigned wallrj May 14, 2021

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2021

jetstack-bot merged commit e941307 into cert-manager:master May 14, 2021

jetstack-bot added this to the v1.4 milestone May 14, 2021

irbekrm deleted the 3879_test_acme_issuer_setup branch May 14, 2021 12:49

irbekrm mentioned this pull request May 19, 2021

Unit test for ACME Issuer setup #3879

Closed

maelvls mentioned this pull request Mar 6, 2022

Retry on conflict for the end-to-end test "added an additional dnsName" #4924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3879 test acme issuer setup #3938

3879 test acme issuer setup #3938

irbekrm commented Apr 29, 2021 •

edited

Loading

irbekrm commented Apr 29, 2021

irbekrm commented Apr 29, 2021

wallrj left a comment

wallrj May 12, 2021

wallrj left a comment

irbekrm commented May 13, 2021

irbekrm commented May 14, 2021

wallrj left a comment

jetstack-bot commented May 14, 2021

3879 test acme issuer setup #3938

3879 test acme issuer setup #3938

Conversation

irbekrm commented Apr 29, 2021 • edited Loading

irbekrm commented Apr 29, 2021

irbekrm commented Apr 29, 2021

wallrj left a comment

Choose a reason for hiding this comment

wallrj May 12, 2021

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

irbekrm commented May 13, 2021

irbekrm commented May 14, 2021

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented May 14, 2021

irbekrm commented Apr 29, 2021 •

edited

Loading