Conversation
Two bugs in the RDS Valkey admission webhook: 1. Webhook annotations referenced the wrong API group (rds.inst.buf.red instead of rds.valkey.buf.red), causing both the mutating and validating webhooks to never fire for Valkey CR operations. This meant the sentinel access serviceType was never propagated from spec.access.serviceType to spec.sentinel.access.serviceType, resulting in sentinel services always being ClusterIP even when NodePort was requested. 2. For ValkeyFailover arch, spec.replicas.shards was only defaulted to 1 when spec.replicas was nil. If the user provided a Replicas object with only ReplicasOfShard set, Shards defaulted to 0 and the validating webhook would reject the CR. Fixed by always enforcing Shards=1 for failover, matching the existing ValkeyReplica behavior.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the RDS Valkey webhook configuration and CRD manifests to align with the rds.valkey.buf.red API group and updated webhook endpoints, and tweaks defaulting behavior for failover instances.
Changes:
- Renames the Valkey mutating/validating webhook paths and API group from the old
rds.inst.buf.rednaming tords.valkey.buf.red(code + manifests). - Forces
spec.replicas.shards = 1in the failover defaulter path. - Updates several CRD schema description strings (notably pod affinity weighting text).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook/rds/v1alpha1/valkey_webhook.go | Updates webhook markers and changes defaulting behavior for failover shards. |
| config/webhook/manifests.yaml | Updates webhook configuration paths and apiGroups to match the Valkey RDS API group. |
| config/crd/bases/valkey.buf.red_sentinels.yaml | CRD schema description text updates (includes pod affinity weighting wording). |
| config/crd/bases/valkey.buf.red_failovers.yaml | CRD schema description text updates (includes pod affinity weighting wording in multiple places). |
| config/crd/bases/valkey.buf.red_clusters.yaml | CRD schema description text updates (includes pod affinity weighting wording). |
| config/crd/bases/rds.valkey.buf.red_valkeys.yaml | CRD schema description text updates (includes pod affinity weighting wording in multiple places). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
118
to
123
| ReplicasOfShard: 2, | ||
| } | ||
| } | ||
| inst.Spec.Replicas.Shards = 1 | ||
| if inst.Spec.Sentinel == nil { | ||
| inst.Spec.Sentinel = &v1alpha1.SentinelSettings{} |
Comment on lines
+686
to
+687
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
Comment on lines
+638
to
+639
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
Comment on lines
+681
to
+682
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
Comment on lines
+2274
to
+2275
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
Comment on lines
+703
to
+704
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
Comment on lines
+2310
to
+2311
| compute a sum by iterating through the elements of this field and subtracting | ||
| "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two bugs in the RDS Valkey admission webhook:
Webhook annotations referenced the wrong API group (rds.inst.buf.red instead of rds.valkey.buf.red), causing both the mutating and validating webhooks to never fire for Valkey CR operations. This meant the sentinel access serviceType was never propagated from spec.access.serviceType to spec.sentinel.access.serviceType, resulting in sentinel services always being ClusterIP even when NodePort was requested.
For ValkeyFailover arch, spec.replicas.shards was only defaulted to 1 when spec.replicas was nil. If the user provided a Replicas object with only ReplicasOfShard set, Shards defaulted to 0 and the validating webhook would reject the CR. Fixed by always enforcing Shards=1 for failover, matching the existing ValkeyReplica behavior.