Fix issue #1668 about auto renew certificate on mutating / validating - #1669
Fix issue #1668 about auto renew certificate on mutating / validating#1669disaster37 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: disaster37 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @disaster37. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
📝 WalkthroughWalkthroughAdds environment-specific annotations to mutating and validating webhook configurations. Cert-manager detection is stored during infrastructure initialization, and annotation generation selects cert-manager injection, OpenShift CA injection, or no annotations. ChangesEnvironment-aware webhook annotations
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterDetection
participant InfrastructureState
participant ConfigBuilders
participant AnnotationGenerator
ClusterDetection->>InfrastructureState: detect cert-manager API group
InfrastructureState->>InfrastructureState: store detection state
ConfigBuilders->>AnnotationGenerator: getWebhookAnnotations(namespace)
AnnotationGenerator->>InfrastructureState: read CertManagerDetected()
InfrastructureState-->>AnnotationGenerator: return detection result
AnnotationGenerator-->>ConfigBuilders: return environment annotations
ConfigBuilders->>ConfigBuilders: set ObjectMeta annotations
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webhook/workspace/mutating_cfg.go (1)
160-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the annotation to a shared constant.
The
cert-manager.io/inject-ca-fromkey anddevworkspace-controller-serving-certname are duplicated verbatim invalidating_cfg.go(Lines 41-43). Centralizing this in a shared constant/helper (e.g., alongsideserver.WebhookServerAppLabels()) would prevent the two copies from drifting if the secret/certificate name ever changes.♻️ Proposed refactor
+// in a shared location, e.g. webhook/workspace/server package +const CertManagerServingCertName = "devworkspace-controller-serving-cert" + +func CertManagerInjectCAAnnotation(namespace string) map[string]string { + return map[string]string{ + "cert-manager.io/inject-ca-from": fmt.Sprintf("%s/%s", namespace, CertManagerServingCertName), + } +}- Annotations: map[string]string{ - "cert-manager.io/inject-ca-from": fmt.Sprintf("%s/devworkspace-controller-serving-cert", namespace), - }, + Annotations: server.CertManagerInjectCAAnnotation(namespace),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webhook/workspace/mutating_cfg.go` around lines 160 - 162, The webhook CA injection annotation and serving cert name are duplicated in mutating_cfg.go and validating_cfg.go, so extract them into a shared constant or helper near the webhook/server label helpers (for example alongside server.WebhookServerAppLabels) and use that in both places. Update the annotation map construction in the webhook config methods to reference the shared symbol instead of hardcoding cert-manager.io/inject-ca-from and devworkspace-controller-serving-cert, so both configs stay in sync if the name changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webhook/workspace/mutating_cfg.go`:
- Around line 160-162: The webhook CA injection annotation and serving cert name
are duplicated in mutating_cfg.go and validating_cfg.go, so extract them into a
shared constant or helper near the webhook/server label helpers (for example
alongside server.WebhookServerAppLabels) and use that in both places. Update the
annotation map construction in the webhook config methods to reference the
shared symbol instead of hardcoding cert-manager.io/inject-ca-from and
devworkspace-controller-serving-cert, so both configs stay in sync if the name
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 300da1dc-77c7-49ef-b60d-15c9b809d86d
📒 Files selected for processing (2)
webhook/workspace/mutating_cfg.gowebhook/workspace/validating_cfg.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1669 +/- ##
==========================================
+ Coverage 37.17% 40.25% +3.07%
==========================================
Files 168 171 +3
Lines 14761 15613 +852
==========================================
+ Hits 5488 6285 +797
- Misses 8921 8942 +21
- Partials 352 386 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: MutateWebhookCfgName, | ||
| Labels: server.WebhookServerAppLabels(), | ||
| Annotations: map[string]string{ |
There was a problem hiding this comment.
I think it fixes issue with Kubernetes infrastructure.
For OpenShift it has to be service.beta.openshift.io/inject-cabundle: "true"
Please correct me, if I am wrong.
There was a problem hiding this comment.
You are right when OpenShift's Native Service CA Operator is used. I have modified this PR to:
- Check is OCP or regulare Kubernetes
- If OCP, check if used OpenShift's Native Service CA Operator or cert-manager
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webhook/workspace/annotations.go (1)
24-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for the cert-manager/OpenShift annotation branches. Cover
getWebhookAnnotationsfor cert-manager, OpenShift, and default behavior, plusInitializeForTestingWithCertManager.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webhook/workspace/annotations.go` around lines 24 - 32, Add tests covering getWebhookAnnotations for cert-manager, OpenShift, and default environments, asserting the exact annotation maps for each branch; also add coverage for InitializeForTestingWithCertManager, using mocks or test setup to control infrastructure detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webhook/workspace/annotations.go`:
- Around line 24-32: Add tests covering getWebhookAnnotations for cert-manager,
OpenShift, and default environments, asserting the exact annotation maps for
each branch; also add coverage for InitializeForTestingWithCertManager, using
mocks or test setup to control infrastructure detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ebe3e7d-1671-45ec-9838-19f3ddec4eda
📒 Files selected for processing (4)
pkg/infrastructure/cluster.gowebhook/workspace/annotations.gowebhook/workspace/mutating_cfg.gowebhook/workspace/validating_cfg.go
|
@disaster37 : Thank you for your PR! Could you please amend your commit with |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webhook/workspace/annotations_test.go (1)
18-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate project-local and third-party imports.
Per coding guidelines, imports should be organized into three groups: standard library, third-party/Kubernetes, and project-local. The project-local import (
github.com/devfile/devworkspace-operator/pkg/infrastructure) and the third-party import (github.com/stretchr/testify/assert) are currently in the same group.As per coding guidelines: "Organize imports into three groups separated by blank lines: standard library, third-party/Kubernetes, and project-local imports."
♻️ Proposed fix
import ( "testing" + "github.com/stretchr/testify/assert" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" - "github.com/stretchr/testify/assert" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webhook/workspace/annotations_test.go` around lines 18 - 23, Reorganize the imports in the annotations test so the third-party assert import and project-local infrastructure import are separated by a blank line, while preserving the standard-library group and existing imports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@webhook/workspace/annotations_test.go`:
- Around line 18-23: Reorganize the imports in the annotations test so the
third-party assert import and project-local infrastructure import are separated
by a blank line, while preserving the standard-library group and existing
imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd698839-cf3c-4ed2-addd-6417d1ceb17d
📒 Files selected for processing (2)
pkg/infrastructure/cluster_test.gowebhook/workspace/annotations_test.go
|
It's done (sign commit + test) |
|
DCO |
|
@disaster37 : Could you please squash your commits into one? I see you've added signed-off only in latest commit. DCO check is failing due to earlier two commits not aligned with commit policy |
…/ validation deployment Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
1311e75 to
b6faf73
Compare
|
It's done |
|
/che-ai-assistant ok-pr-review Task completed. |
1 similar comment
|
/che-ai-assistant ok-pr-review Task completed. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Thanks for addressing the stale caBundle problem. The annotation selection logic is well-structured and the test coverage is good. A few things worth discussing:
PR title has typos. The title reads "Fix issue #1668 about auto renew certificat on mutating / validatinf" - "certificat" should be "certificate" and "validatinf" should be "validating". Consider updating before merging, as it will appear in the merge commit message.
SetupWebhooks TLS path inconsistency (pkg/webhook/create.go). With CertManagerDetected() now in place, there are four infrastructure states but SetupWebhooks in pkg/webhook/create.go still branches only on IsOpenShift(). On an OpenShift cluster with cert-manager installed, the service gets annotated with service.beta.openshift.io/serving-cert-secret-name (OpenShift Service CA provisions the TLS secret), while the webhook gets the cert-manager.io/inject-ca-from annotation pointing at a cert-manager Certificate. These use different CAs - OpenShift Service CA vs. cert-manager's own Issuer - so the CA bundle cert-manager injects will not match the TLS certificate the server presents. Would it be possible to incorporate CertManagerDetected() into SetupWebhooks so that when cert-manager is present the service is not annotated for the OpenShift Service CA?
| ) | ||
|
|
||
| func getWebhookAnnotations(namespace string) map[string]string { | ||
| annotations := map[string]string{} |
There was a problem hiding this comment.
Consider adding a brief comment here explaining why cert-manager takes priority over the OpenShift Service CA annotation. The ordering is intentional and correct, but without a comment a future maintainer may read the else if as arbitrary ordering rather than a deliberate policy decision. Something like: // cert-manager takes precedence when installed on OpenShift; mixing both annotations can cause conflicts.
| annotations := map[string]string{} | ||
| if infrastructure.CertManagerDetected() { | ||
| annotations["cert-manager.io/inject-ca-from"] = fmt.Sprintf("%s/devworkspace-controller-serving-cert", namespace) | ||
| } else if infrastructure.IsOpenShift() { |
There was a problem hiding this comment.
Consider extracting devworkspace-controller-serving-cert to a named constant, similar to how MutateWebhookCfgName and ValidateWebhookCfgName are defined. This string is the post-kustomize composed name (from namePrefix: devworkspace-controller- applied to base name serving-cert in deploy/templates/components/cert-manager/self-signed-certificates.yaml). If the namePrefix changes, the kustomize-generated CRD webhook patches update automatically but this code silently breaks - cert-manager cainjector would find no Certificate and log errors without injecting the CA bundle. A constant (or at minimum a comment documenting the coupling and the kustomize file that must stay in sync) makes the contract explicit.
| Labels: server.WebhookServerAppLabels(), | ||
| Name: MutateWebhookCfgName, | ||
| Labels: server.WebhookServerAppLabels(), | ||
| Annotations: getWebhookAnnotations(namespace), |
There was a problem hiding this comment.
When cert-manager or the OpenShift Service CA operator is active, the webhook config will have both an explicit CABundle (written from the cert file on disk by the caller) and a CA-injection annotation. During a cert rotation the operator may write a stale CABundle and the external controller will overwrite it - creating a brief window with a mismatched CA. Would it be possible to omit CABundle from WebhookClientConfig when CertManagerDetected() is true (or when using the OpenShift annotation), letting the external controller be the sole authority? The cert-manager CRD patch in deploy/templates/cert-manager/crd_webhooks_patch.yaml already follows this pattern, with a comment "caBundle will be filled by cert-manager on creation".
|
|
||
| func TestCertManagerDetectedPanicsWhenNotInitialized(t *testing.T) { | ||
| initialized = false | ||
| defer func() { |
There was a problem hiding this comment.
Consider using t.Cleanup() to restore the global state after this test instead of (or in addition to) the defer. The defer resets to Kubernetes after the panic test, but if the test panics in an unexpected way before the deferred function runs, subsequent tests in the same package may see initialized = false. A t.Cleanup or a helper that captures and restores all three package-level variables (current, certManagerDetected, initialized) would make the test order-independent. Also worth noting in the file or test that these tests cannot use t.Parallel() due to shared mutable package state.
| @@ -85,6 +102,7 @@ func detect() (Type, error) { | |||
| if err != nil { | |||
There was a problem hiding this comment.
Does it make sense to move the certManagerDetected assignment into Initialize() alongside the detect() call, rather than inside detect() as a side effect? The function's name and (Type, error) return type suggest it only determines the cluster type, so the hidden side effect is a bit surprising. It also makes detect() harder to test in isolation and is architecturally inconsistent with how OpenShift detection works (returned as a value, not a side effect). One option: return a richer struct from detect() (e.g., DetectionResult{InfraType, CertManagerAvailable}) or call a separate function from Initialize().
disaster37
changed the title
3 participants
What does this PR do?
Add cert-manager annotations on mutating and validating depoloyment to auto renew certificate and auto restart pod when it's done.
What issues does this PR fix or reference?
Fix issue #1668
Is it tested? How?
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit