feat: add unified admission webhook for Group validation - #261
Conversation
- Register validating webhook and TLS in manager - Add Group webhook handlers and tests - Add cert-manager, webhook service, and kustomize patches - Extend RBAC and vendor deps for webhook/testing Signed-off-by: Sunando Bhattacharya <subhatta@redhat.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust admission webhook mechanism for the operator. By implementing a unified validator, the system can now perform cross-resource validation, specifically ensuring that critical dependencies like ServiceAccounts are present when certain backend configurations are requested. The changes include the necessary infrastructure for secure communication via cert-manager and provide a flexible configuration to toggle webhook functionality. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The webhook waits for groups to change, To check the backends in their range. If 'rover' type is found in sight, It checks the SA to keep things right. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validating admission webhook for the Group resource to ensure that any backend of type rover has a corresponding usernaut-prod ServiceAccount in the watched namespace. It implements a unified webhook architecture that decodes raw API requests and delegates validation to specific resource types. Feedback includes suggestions to improve the decoding safety by providing GVK, centralizing the watched namespace logic to avoid duplication, refining error messages for better troubleshooting, and using named interfaces to enhance code maintainability.
| } | ||
|
|
||
| gvk := obj.GetObjectKind().GroupVersionKind() | ||
| decoded, _, err := v.Decoder.Decode(unknown.Raw, nil, nil) |
There was a problem hiding this comment.
When decoding a runtime.Unknown object, it is generally safer to provide the expected GroupVersionKind if it is known from the admission request, rather than passing nil as the second argument to Decode.
| func getWatchedNamespace() string { | ||
| if ns := os.Getenv("WATCHED_NAMESPACE"); ns != "" { | ||
| return ns | ||
| } | ||
| return defaultNamespace | ||
| } |
There was a problem hiding this comment.
The logic to determine the watched namespace is duplicated here and in cmd/main.go. Consider centralizing this in a utility package or passing the namespace as a configuration parameter to the webhook handler to ensure consistency across the application.
|
|
||
| c := ClientFromContext(ctx) | ||
| if c == nil { | ||
| return fmt.Errorf("internal error: webhook client not available") |
There was a problem hiding this comment.
The error message 'internal error: webhook client not available' is somewhat generic for an end-user. Consider a more descriptive message such as 'failed to retrieve Kubernetes client from context' to aid in troubleshooting if the context injection fails.
| return fmt.Errorf("internal error: webhook client not available") | |
| return fmt.Errorf("failed to retrieve Kubernetes client from context") |
| if validator, ok := obj.(interface { | ||
| ValidateCreate(ctx context.Context) (admission.Warnings, error) | ||
| }); ok { |
There was a problem hiding this comment.
The anonymous interface for ValidateCreate is repeated for ValidateUpdate and ValidateDelete. Defining a named interface (e.g., ResourceValidator) would improve code readability and maintainability.
1 participant
Changes
📝 Description
What changed?
Why is this change needed?
Admission hooks are needed.
Dependencies
🧪 Testing
Test Coverage
Performance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
⚙️ Configuration Changes
✅ Developer Checklist