Webhook solver conformance bugfix #5736

irbekrm · 2023-01-18T17:46:28Z

This PR fixes DNS webhook solver conformance test bug introduced in #5691 - in that PR I changed a webhook Solver interface implentation for RFC2136 to not start a Secrets informer factory and made some associated changes to integration tests which are a bit clunky for this particular Solver implementation.
The tests are being used as conformance tests by external webhook implementations and the changes broke their tests.

This PR:

reverts changes to tests that would affect third party solver implementations using them
makes some changes to the RFC2136 Solver implementation to make it work in the integration test setup (which does not generally match how it would normally run, but I think this is acceptable)
adds a bunch of comments to make it clearer which parts of the webhook solver code are going to be consumed by external webhook implementations

To reproduce the bug:

checkout example-webhook code from this PR that bumps cert-manager dependency to latest https://github.com/irbekrm/webhook-example/tree/bump_deps
run TEST_ZONE_NAME=example.com. make test that runs the conformance tests from cert-manager against the example webhook implementation
observe an error

To observe the fix:

checkout example-webhook code from this PR that bumps cert-manager dependency to latest https://github.com/irbekrm/webhook-example/tree/bump_deps
Add replace github.com/cert-manager/cert-manager => github.com/irbekrm/cert-manager webhook_solver_conformance_bugfix to go.mod to pull in cert-manager from this PR
Run go mod tidy
run TEST_ZONE_NAME=example.com. make test that runs the conformance tests from cert-manager against the example webhook implementation
Observe no errors

NONE

/kind bug

With the goal of making folks working on these parts of code be aware that this is the one bit that will be imported in external projects Signed-off-by: irbekrm <irbekrm@gmail.com>

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

The way the tests run (a new kube apiserver with a different client created for the same initialized solver) is not how this solver would actually run Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm · 2023-01-18T17:47:28Z

/kind bug

irbekrm · 2023-01-18T17:48:01Z

/test

jetstack-bot · 2023-01-18T17:48:05Z

@irbekrm: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cert-manager-master-chart
/test pull-cert-manager-master-e2e-v1-26
/test pull-cert-manager-master-e2e-v1-26-upgrade
/test pull-cert-manager-master-make-test

The following commands are available to trigger optional jobs:

/test pull-cert-manager-master-e2e-v1-21
/test pull-cert-manager-master-e2e-v1-22
/test pull-cert-manager-master-e2e-v1-23
/test pull-cert-manager-master-e2e-v1-24
/test pull-cert-manager-master-e2e-v1-25
/test pull-cert-manager-master-e2e-v1-26-feature-gates-disabled
/test pull-cert-manager-master-e2e-v1-26-issuers-venafi-cloud
/test pull-cert-manager-master-e2e-v1-26-issuers-venafi-tpp
/test pull-cert-manager-master-license

Use /test all to run the following jobs that were automatically triggered:

pull-cert-manager-master-chart
pull-cert-manager-master-e2e-v1-26
pull-cert-manager-master-e2e-v1-26-upgrade
pull-cert-manager-master-make-test

In response to this:

/test

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.

irbekrm · 2023-01-18T17:48:39Z

/test pull-cert-manager-master-e2e-v1-26-feature-gates-disabled
/test pull-cert-manager-master-e2e-v1-21
/test pull-cert-manager-master-e2e-v1-22

irbekrm · 2023-01-18T18:13:05Z

Seems to be an unrelated flake to do with venafi addons setup

/test pull-cert-manager-master-e2e-v1-26-feature-gates-disabled

irbekrm · 2023-01-18T18:26:54Z

network issues as far as I can tell

/test pull-cert-manager-master-e2e-v1-21

irbekrm · 2023-01-18T19:03:42Z

/test pull-cert-manager-master-e2e-v1-22
/test pull-cert-manager-master-e2e-v1-26

irbekrm · 2023-01-19T07:04:57Z

Some flake related to the skipped Venafi tests, unrelated, but probably worth looking into at some point https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/cert-manager_cert-manager/5736/pull-cert-manager-master-e2e-v1-26/1615787040345952256

/test pull-cert-manager-master-e2e-v1-26

SgtCoDFish

One nitpick about import ordering - sorry! This looks good to go though!

pkg/acme/webhook/cmd/cmd.go

test/acme/dns/options.go

SgtCoDFish · 2023-01-19T11:56:48Z

/cherry-pick release-1.11

jetstack-bot · 2023-01-19T11:56:50Z

@SgtCoDFish: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.11

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.

SgtCoDFish · 2023-01-19T11:56:56Z

/cherry-pick release-1.10

jetstack-bot · 2023-01-19T11:56:58Z

@SgtCoDFish: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.10

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.

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

SgtCoDFish

/lgtm
/approve

Thanks for this 👍

Question about backporting: Do people run the conformance tests against tagged versions or could they run against a branch? It seems a shame to do a patch release just for this testing change 🤔

jetstack-bot · 2023-01-19T12:17:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

irbekrm · 2023-01-19T12:24:56Z

Question about backporting: Do people run the conformance tests against tagged versions or could they run against a branch? It seems a shame to do a patch release just for this testing change

I agree with you, it does not make sense to cut a release just so as to get a tag.
I think for now it'll be okay to ask for folks to import from a branch, once the backport PRs merge I will update the example webhook deps and add a branch import so there's an example of doing that.

irbekrm · 2023-01-19T13:02:51Z

/test pull-cert-manager-master-e2e-v1-26

jetstack-bot · 2023-01-19T13:44:57Z

@SgtCoDFish: new pull request created: #5738

In response to this:

/cherry-pick release-1.11

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 · 2023-01-19T13:45:34Z

@SgtCoDFish: new pull request created: #5739

In response to this:

/cherry-pick release-1.10

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.

irbekrm added 3 commits January 18, 2023 17:41

A bunch of comments on webhook solver functionality

1834afa

With the goal of making folks working on these parts of code be aware that this is the one bit that will be imported in external projects Signed-off-by: irbekrm <irbekrm@gmail.com>

RFC2136 solver has an init option to reset secrets lister

216b60e

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

Resets secrets lister in RFC2136 conformance tests

644a46c

The way the tests run (a new kube apiserver with a different client created for the same initialized solver) is not how this solver would actually run Signed-off-by: irbekrm <irbekrm@gmail.com>

jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jan 18, 2023

SgtCoDFish reviewed Jan 19, 2023

View reviewed changes

pkg/acme/webhook/cmd/cmd.go Show resolved Hide resolved

test/acme/dns/options.go Outdated Show resolved Hide resolved

Code review feedback: fix imports

438c79d

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

SgtCoDFish approved these changes Jan 19, 2023

View reviewed changes

jetstack-bot assigned SgtCoDFish Jan 19, 2023

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2023

jetstack-bot merged commit c08b337 into cert-manager:master Jan 19, 2023

jetstack-bot added this to the v1.12 milestone Jan 19, 2023

jetstack-bot mentioned this pull request Jan 19, 2023

[release-1.11] Webhook solver conformance bugfix #5738

Merged

jetstack-bot mentioned this pull request Jan 19, 2023

[release-1.10] Webhook solver conformance bugfix #5739

Merged

This was referenced Jan 19, 2023

Bump deps cert-manager/webhook-example#45

Merged

ACME webhook conformance tests crashing with cert-manager 1.11.0 and 1.10.2 #5725

Closed

irbekrm deleted the webhook_solver_conformance_bugfix branch March 9, 2023 15:18

Webhook solver conformance bugfix #5736

Webhook solver conformance bugfix #5736

Uh oh!

Conversation

irbekrm commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

jetstack-bot commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 18, 2023

Uh oh!

irbekrm commented Jan 19, 2023

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SgtCoDFish commented Jan 19, 2023

Uh oh!

jetstack-bot commented Jan 19, 2023

Uh oh!

SgtCoDFish commented Jan 19, 2023

Uh oh!

jetstack-bot commented Jan 19, 2023

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Jan 19, 2023

Uh oh!

irbekrm commented Jan 19, 2023

Uh oh!

irbekrm commented Jan 19, 2023

Uh oh!

jetstack-bot commented Jan 19, 2023

Uh oh!

jetstack-bot commented Jan 19, 2023

Uh oh!

Uh oh!

irbekrm commented Jan 18, 2023 •

edited

Loading