Fix issue #1668 about auto renew certificat on mutating / validatinf#1669
Fix issue #1668 about auto renew certificat on mutating / validatinf#1669disaster37 wants to merge 2 commits into
Conversation
… validation deployment
disaster37
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[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 API-group detection is stored by 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 Cluster as Cluster detection
participant Infrastructure as Infrastructure state
participant ConfigBuilders as Webhook configuration builders
participant Annotations as getWebhookAnnotations
Cluster->>Infrastructure: detect cert-manager API group
Infrastructure->>Infrastructure: store certManagerDetected
ConfigBuilders->>Annotations: generate annotations(namespace)
Annotations->>Infrastructure: read environment state
Infrastructure-->>Annotations: return detection result
Annotations-->>ConfigBuilders: return CA injection annotations
ConfigBuilders->>ConfigBuilders: set ObjectMeta annotations
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
2 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