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

Improve E2E Test Coverage for Webhooks (Defaulting, Validation, Conversion) and Cert-Manager CA Injection #4202

Closed
camilamacedo86 opened this issue Oct 2, 2024 · 4 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. testing

Comments

@camilamacedo86
Copy link
Member

What do you want to happen?

The current e2e tests for webhooks in the Kubebuilder repository cover basic webhook creation and validation. However, several important scenarios remain untested. To ensure comprehensive coverage, we propose expanding the tests to include:

  • Defaulting Webhooks: Ensure defaulting webhooks apply resource defaults as expected.
  • Validation Webhooks: Confirm that invalid resources are rejected by validation webhooks and validation logic is enforced.
  • Conversion Webhooks: Verify conversion webhooks work when converting resources between different versions of CRDs.
  • Cert-Manager CA Injection: Ensure cert-manager correctly injects CA certificates into all webhook types (defaulting, validation, and conversion), and verify that these webhooks function properly in a cluster.

Context:
There are existing tests for webhooks, such as in:

These tests can be added under a new directory in e2e tests or, if more convenient, integrated into the existing go/v4 test suite.

Extra Labels

No response

@camilamacedo86 camilamacedo86 added kind/feature Categorizes issue or PR as related to a new feature. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. testing lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Oct 2, 2024
@camilamacedo86 camilamacedo86 added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Oct 11, 2024
@awgrass
Copy link

awgrass commented Oct 12, 2024

If this is an easy one, I am happy to take it over. It would be my first contribution.

@Bharadwajshivam28
Copy link
Member

@awgrass are you working on this?

@camilamacedo86
Copy link
Member Author

Hi @awgrass and @Bharadwajshivam28,

I apologize; that was totally my fault.

As you can see, we’re already validating the injection here:

By("validating that the mutating|validating webhooks have the CA injected")
verifyCAInjection := func() error {
mwhOutput, err := kbc.Kubectl.Get(
false,
"mutatingwebhookconfigurations.admissionregistration.k8s.io",
fmt.Sprintf("e2e-%s-mutating-webhook-configuration", kbc.TestSuffix),
"-o", "go-template={{ range .webhooks }}{{ .clientConfig.caBundle }}{{ end }}")
ExpectWithOffset(2, err).NotTo(HaveOccurred())
// check that ca should be long enough, because there may be a place holder "\n"
ExpectWithOffset(2, len(mwhOutput)).To(BeNumerically(">", 10))
vwhOutput, err := kbc.Kubectl.Get(
false,
"validatingwebhookconfigurations.admissionregistration.k8s.io",
fmt.Sprintf("e2e-%s-validating-webhook-configuration", kbc.TestSuffix),
"-o", "go-template={{ range .webhooks }}{{ .clientConfig.caBundle }}{{ end }}")
ExpectWithOffset(2, err).NotTo(HaveOccurred())
// check that ca should be long enough, because there may be a place holder "\n"
ExpectWithOffset(2, len(vwhOutput)).To(BeNumerically(">", 10))
return nil
}

So, this step isn't necessary. What we need instead is an e2e test to validate the conversion from one version to another using the webhook. We can implement the necessary code in the sample, and potentially add it here:

https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/book/src/multiversion-tutorial/testdata/project/test/e2e/e2e_test.go

I’ll open a new issue to provide more details, and your assistance would be greatly appreciated!

I'll go ahead and close this one as it’s already complete / not required.

@camilamacedo86
Copy link
Member Author

Hi @awgrass and @Bharadwajshivam28,

Please take a look at the issue here: #4255.

Your assistance would be greatly appreciated, and feel free to collaborate if that works best for both of you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. testing
Projects
None yet
Development

No branches or pull requests

3 participants