feat: initialize admission webhook #234
Conversation
WalkthroughThis pull request introduces webhook configurations for Changes
Sequence DiagramsequenceDiagram
participant Webhook as Admission Webhook
participant Cluster as GreptimeDBCluster/Standalone
participant Manager as Controller Manager
Cluster ->> Webhook: Create/Update/Delete Request
Webhook ->> Webhook: Validate Request
alt Validation Passes
Webhook -->> Manager: Allow Operation
else Validation Fails
Webhook -->> Manager: Deny Operation
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/e2e/webhook_suite_test.go (3)
125-132: Specify an explicit timeout forEventuallyto ensure consistent test durationCurrently, the
Eventuallyblock relies on the default timeout and polling interval. Specifying explicit values can help prevent flaky tests and ensure consistent behavior across different environments.Apply this diff to set explicit timeout and interval:
}).Should(Succeed()) + }, "10s", "100ms").Should(Succeed())Adjust the timeout (
"10s") and polling interval ("100ms") as appropriate for your test environment.
64-67: Consider settingErrorIfCRDPathMissingtotrueto catch missing CRDsSetting
ErrorIfCRDPathMissingtotruehelps detect missing or misconfigured CRD paths early in the test setup, preventing silent failures.Apply this diff:
testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "resources")}, - ErrorIfCRDPathMissing: false, + ErrorIfCRDPathMissing: true, WebhookInstallOptions: envtest.WebhookInstallOptions{
79-85: Remove unnecessary addition ofadmissionv1beta1schemeThe admission
v1beta1API has been deprecated in favor ofv1. Since you are usingadmissionReviewVersions=v1, addingadmissionv1beta1to the scheme may be unnecessary.Apply this diff to remove the deprecated API:
scheme := runtime.NewScheme() err = v1alpha1.AddToScheme(scheme) Expect(err).NotTo(HaveOccurred()) - err = admissionv1beta1.AddToScheme(scheme) - Expect(err).NotTo(HaveOccurred()) //+kubebuilder:scaffold:schemeapis/v1alpha1/greptimedbcluster_webhook.go (1)
58-63: Consider enabling delete validation or removing theValidateDeletemethodThe
ValidateDeletemethod includes a TODO but the webhook is not registered for delete operations (verbs). If deletion validation is required, update the webhook configuration to include thedeleteverb. Otherwise, consider removing the unused method to keep the codebase clean.Apply this diff to update the webhook configuration:
//+kubebuilder:webhook:path=/validate-greptime-io-v1alpha1-greptimedbcluster,mutating=false,failurePolicy=fail,sideEffects=None,groups=greptime.io,resources=greptimedbclusters,verbs=create;update,versions=v1alpha1,name=vgreptimedbcluster.kb.io,admissionReviewVersions=v1 +// TODO: change verbs to "verbs=create;update;delete" if you want to enable deletion validation.apis/v1alpha1/greptimedbstandalone_webhook.go (1)
58-63: Decide on delete validation for the webhookThe
ValidateDeletemethod includes a TODO comment, but the webhook does not handle delete operations due to the missingdeleteverb in the configuration. If delete validation is needed, update the webhook verbs accordingly. If not, consider removing the method.Apply this diff to include delete validation:
//+kubebuilder:webhook:path=/validate-greptime-io-v1alpha1-greptimedbstandalone,mutating=false,failurePolicy=fail,sideEffects=None,groups=greptime.io,resources=greptimedbstandalones,verbs=create;update,versions=v1alpha1,name=vgreptimedbstandalone.kb.io,admissionReviewVersions=v1 +// TODO: change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
config/certmanager/certificate.yamlis excluded by!config/**config/certmanager/kustomization.yamlis excluded by!config/**config/certmanager/kustomizeconfig.yamlis excluded by!config/**config/default/manager_webhook_patch.yamlis excluded by!config/**config/default/webhookcainjection_patch.yamlis excluded by!config/**config/webhook/kustomization.yamlis excluded by!config/**config/webhook/kustomizeconfig.yamlis excluded by!config/**config/webhook/manifests.yamlis excluded by!config/**config/webhook/service.yamlis excluded by!config/**
📒 Files selected for processing (5)
PROJECT(2 hunks)apis/v1alpha1/greptimedbcluster_webhook.go(1 hunks)apis/v1alpha1/greptimedbstandalone_webhook.go(1 hunks)apis/v1alpha1/zz_generated.deepcopy.go(1 hunks)tests/e2e/webhook_suite_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- PROJECT
- apis/v1alpha1/zz_generated.deepcopy.go
🔇 Additional comments (1)
tests/e2e/webhook_suite_test.go (1)
136-141: LGTM
The AfterSuite function correctly handles cleanup by canceling the context and stopping the test environment.
related issue: #153
related docs: https://kubebuilder.io/cronjob-tutorial/webhook-implementation
Summary by CodeRabbit
New Features
GreptimeDBClusterandGreptimeDBStandaloneresources, enabling validation.Bug Fixes
GreptimeDBStandalonefor consistency.Documentation