feat: add validating webhook for run-levels #1451
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: SequeI <[email protected]>
Reviewer's GuideThis PR implements a validating admission webhook for Securesign CRs to enforce namespace run-level policies, integrating it into the operator controller, adding the necessary webhook configuration, service, cert-manager resources and kustomize overlays, enhancing the e2e tests to deploy and validate the webhook (including CA bundle injection), updating the Makefile and CI to support overlay selection and cert-manager installation, and adding unit tests for the new validator logic. Sequence diagram for Securesign CR validation via webhooksequenceDiagram
participant User as actor User
participant K8sAPI as Kubernetes API Server
participant Webhook as Validating Webhook
participant Operator as Operator Controller
User->>K8sAPI: Create/Update Securesign CR
K8sAPI->>Webhook: AdmissionReview (Securesign CR)
Webhook->>Operator: Validate namespace policy
Operator-->>Webhook: Validation result
Webhook-->>K8sAPI: AdmissionResponse (allow/deny)
K8sAPI-->>User: Resource created/updated or error
ER diagram for webhook and certificate resourceserDiagram
"ValidatingWebhookConfiguration" {
string name
string path
string failurePolicy
string[] admissionReviewVersions
string[] operations
string[] resources
string[] apiGroups
string[] apiVersions
string sideEffects
}
"Service" {
string name
string namespace
string[] ports
string selector
}
"Certificate" {
string name
string namespace
string secretName
string issuerRef
string[] dnsNames
}
"Issuer" {
string name
string namespace
string selfSigned
}
ValidatingWebhookConfiguration ||--|{ Service : "uses"
Certificate ||--|{ Issuer : "issued by"
ValidatingWebhookConfiguration ||--|{ Certificate : "secured by"
Service ||--|{ Certificate : "serves cert"
Certificate ||--|{ Service : "for service"
Class diagram for SecureSignValidator and validation logicclassDiagram
class SecureSignValidator {
+Client: client.Client
+ValidateCreate(ctx, obj)
+ValidateUpdate(ctx, oldObj, newObj)
+ValidateDelete(ctx, obj)
-validateNamespacePolicy(ctx, operandCR)
}
SecureSignValidator --|> admission.CustomValidator
class reservedRunLevels {
+"0": true
+"1": true
+"9": true
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new polling helpers in suite_test always swallow client.Get errors (treating all errors as ‘not ready’); consider distinguishing NotFound from other errors and failing fast on real errors to avoid masking issues.
- suite_test.go has grown very large and is doing a lot of raw YAML/unstructured object manipulation; consider extracting common YAML loading and patching logic into reusable test helpers or using typed API objects for clarity and maintainability.
- The hardcoded relative paths for cert-manager and webhook YAML in tests can easily break when moving files—consider using Go’s embed FS or a configurable base path to make file resolution more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new polling helpers in suite_test always swallow client.Get errors (treating all errors as ‘not ready’); consider distinguishing NotFound from other errors and failing fast on real errors to avoid masking issues.
- suite_test.go has grown very large and is doing a lot of raw YAML/unstructured object manipulation; consider extracting common YAML loading and patching logic into reusable test helpers or using typed API objects for clarity and maintainability.
- The hardcoded relative paths for cert-manager and webhook YAML in tests can easily break when moving files—consider using Go’s embed FS or a configurable base path to make file resolution more robust.
## Individual Comments
### Comment 1
<location> `config/overlays/openshift/serving_cert_annotation_patch.yaml:7` </location>
<code_context>
+ name: controller-manager-webhook-service
+ namespace: openshift-rhtas-operator
+ labels:
+ control-plane: controller-manager
+ annotations:
+ service.beta.openshift.io/serving-cert-secret-name: webhook-server-tls
</code_context>
<issue_to_address>
**issue (bug_risk):** Label value differs from base service; may cause selector issues.
The label should match the base service to ensure selectors and resource matching work correctly.
</issue_to_address>
### Comment 2
<location> `config/overlays/kubernetes/cert_resources.yaml:20-21` </location>
<code_context>
+ name: selfsigned-issuer
+ kind: Issuer
+ dnsNames:
+ - rhtas-controller-manager-webhook-service.openshift-rhtas-operator.svc
+ - rhtas-controller-manager-webhook-service.openshift-rhtas-operator.svc.cluster.local
</code_context>
<issue_to_address>
**issue (bug_risk):** DNS names for certificate may not match service name in base manifest.
The DNS names use 'rhtas-controller-manager-webhook-service', but the actual service is 'controller-manager-webhook-service'. This mismatch may lead to TLS failures. Please update the DNS names to match the service name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a validating webhook to the operator for Securesign CRs to enforce RHTAS namespace policies, integrate cert-manager for TLS, update kustomize and Makefile for overlay support, and extend both unit and e2e tests as well as CI workflows to cover the new webhook functionality
New Features:
Enhancements:
CI:
Tests: