feat(acme): Add default feature gate for ACME HTTP01 Ingress pathType Exact #7795
Conversation
|
@sspreitzer: The label(s) In response to this:
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-sigs/prow repository. |
|
Hi @sspreitzer. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
/kind feature |
903746d to
ba9be37
Compare
ba9be37 to
efecd30
Compare
c31df7f to
7a09b82
Compare
|
@wallrj Please have a look. There could be some parts missing, eg. tests and e2e tests. If they should be part of this PR. |
|
/ok-to-test @sspreitzer I expect the E2E tests will fail since #7792. I'll figure out how to work around that. |
|
@wallrj added code tests |
|
I think what we need to do is --set config.strict-path-validation=false here: cert-manager/make/e2e-setup.mk Lines 369 to 388 in 12cad36 And then, ideally, figure out how we can set that to true if the new feature gate is disabled. cert-manager/make/e2e-setup.mk Line 228 in 12cad36
If you want to try running some of the E2E tests rather than the whole suite, you can do this: If you want to test the updated code on a real cluster (with cilium installed), you can publish the images to a public registry and deploy to the current cluster in your kubeconfig, as follows:
|
Signed-off-by: Sascha Spreitzer <sascha@spreitzer.ch>
ec4ceca to
965c1b4
Compare
|
/test pull-cert-manager-master-e2e-v1-33-upgrade |
| @@ -382,6 +382,7 @@ e2e-setup-ingressnginx: $(call image-tar,ingressnginx) load-$(call image-tar,ing | |||
| --set controller.service.clusterIP=${SERVICE_IP_PREFIX}.15 \ | |||
| --set controller.service.type=ClusterIP \ | |||
| --set controller.config.no-tls-redirect-locations= \ | |||
| --set-string controller.config.strict-validate-path-type=false \ | |||
There was a problem hiding this comment.
I added an explanatory note, in #7809
|
@wallrj I believe we are good to go. Regarding the end to end testing idea of testing
Regarding adding cilium ingress as an end-to-test scenario, I would prefer creating an other PR for that implementation. Insofar, I believe we are ready to merge this contribution. Is the release note sufficient or shall we add some more context? |
|
Due to this change, NGINX Ingress Controller v1.12.3 admission validation web hook is throwing the below error which is preventing the ingress creation: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: ingress contains invalid paths: path /.well-known/acme-challenge/TOKEN cannot be used with pathType Exact |
|
@sspreitzer thank you for sharing. It is clear that this bug needs to be resolved by ingress Nginx maintainers. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a feature gate (ACMEHTTP01IngressPathTypeExact) to control whether ACME HTTP01 Ingress challenge solvers default to pathType Exact or ImplementationSpecific. It introduces new unit tests in the ingress_test.go file, updates ingress creation logic in ingress.go, tweaks e2e test configuration in make/e2e-setup.mk, and adds the new feature flag in the internal/controller/feature/features.go file.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/issuer/acme/http/ingress_test.go | Added tests to validate Ingress pathType behavior based on the feature gate |
| pkg/issuer/acme/http/ingress.go | Updated Ingress creation logic to select the correct pathType |
| make/e2e-setup.mk | Added configuration to disable strict path type validation for e2e tests |
| internal/controller/feature/features.go | Added and documented the new ACMEHTTP01IngressPathTypeExact feature flag |
Comments suppressed due to low confidence (1)
internal/controller/feature/features.go:181
- There appears to be an extra backtick in the documentation comment. Consider changing
pathType`` topathType` for clarity and consistency.
// `ACMEHTTP01IngressPathTypeExact` changes the default `pathType`` for ACME
There was a problem hiding this comment.
Thanks @sspreitzer
This looks good enough to me.
Let's merge it and tomorrow
I didn't mean that you should add automated cilium tests. I just meant that you should do a manual E2E test to verity that cert-manager HTTP01 can now be used with the cilium ingress.
Similarly, I'll go through the https://cert-manager.io/docs/tutorials/acme/nginx-ingress/ tutorial to see for myself that disabling the feature gate allows me to use ingress-nginx 4.12....we'll need to add a note to that tutorial.
There's a tiny typo in the feature gate docs which we can address in an another PR.
And I want to update this section in the helm chart before we release 1.18.1, with this and another new feature gate.
cert-manager/deploy/charts/cert-manager/values.yaml
Lines 237 to 253 in 12cad36
And I'll need to update the cert-manager documentation with this new featuregate.
/approve
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.18 |
|
@wallrj: #7795 failed to apply on top of branch "release-1.18": In response to this:
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-sigs/prow repository. |
|
/cherry-pick release-1.18 |
|
@wallrj: new pull request created: #7810 In response to this:
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-sigs/prow repository. |
TestingI temporarily re-enabled the ingress-nginx strict-validate-path feature Observed the E2E tests fail:
Verified that I can disable the acmesolver:
image:
repository: cert-manager-acmesolver-amd64
tag: v1.18.0-17-g379f43e3de2237
cainjector:
extraArgs:
- --feature-gates=ServerSideApply=true
image:
repository: cert-manager-cainjector-amd64
tag: v1.18.0-17-g379f43e3de2237
crds:
enabled: true
dns01RecursiveNameservers: 10.0.0.16:53
dns01RecursiveNameserversOnly: true
extraArgs:
- --kube-api-qps=9000
- --kube-api-burst=9000
- --concurrent-workers=200
- --enable-gateway-api
featureGates: ExperimentalCertificateSigningRequestControllers=true,ExperimentalGatewayAPISupport=true,ServerSideApply=true,LiteralCertificateSubject=true,UseCertificateRequestBasicConstraints=true,NameConstraints=true,OtherNames=true,ACMEHTTP01IngressPathTypeExact=false
image:
repository: cert-manager-controller-amd64
tag: v1.18.0-17-g379f43e3de2237
startupapicheck:
image:
repository: cert-manager-startupapicheck-amd64
tag: v1.18.0-17-g379f43e3de2237
webhook:
featureGates: LiteralCertificateSubject=true,NameConstraints=true,OtherNames=true
image:
repository: cert-manager-webhook-amd64
tag: v1.18.0-17-g379f43e3de2237
Now the E2E tests pass: |
|
Also checked that I can disable the feature via the config file |
| @@ -196,6 +212,7 @@ var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.Feature | |||
| OtherNames: {Default: false, PreRelease: featuregate.Alpha}, | |||
| UseDomainQualifiedFinalizer: {Default: true, PreRelease: featuregate.GA}, | |||
| DefaultPrivateKeyRotationPolicyAlways: {Default: true, PreRelease: featuregate.Beta}, | |||
| ACMEHTTP01IngressPathTypeExact: {Default: true, PreRelease: featuregate.GA}, | |||
There was a problem hiding this comment.
This should be featuregate.Beta, like DefaultPrivateKeyRotationPolicyAlways above it.
It'll cause it to be turned on by default, but won't show a warning in the logs if you disable it.
There was a problem hiding this comment.
Pull Request Motivation
With the release of cert-manager version
1.18.1the pathType changes fromImplementationSpecifictoExact. This PR implements the feature gateACMEHTTP01IngressPathTypeExact.ACMEHTTP01IngressPathTypeExactchanges the defaultpathTypefor ACME HTTP01 Ingress based challenges toExact. This security feature ensures that the challenge path (which is an exact path) is not misinterpreted as a regular expression or some other Ingress specific (ImplementationSpecific) parsing. This allows HTTP01 challenges to be solved when using standards compliant Ingress controllers such as Cilium. The old defaultImplementationSpecificcan be reinstated by disabling this feature gate. You may need to disable the feature for compatibility withingress-nginx. See version 1.18 release notes.Kind
/kind feature
Release Note