test: add Helm upgrade verification for Gateway #631
Conversation
WalkthroughAdds a GitHub Actions step that triggers and verifies a Gateway Helm upgrade via a ConfigMap values.yaml annotation change, and introduces backoff handling in the RestApiReconciler to delay processing when NextRetryTime is in the future. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant CLI as kubectl/helm
participant K8s as Kubernetes API
participant Operator as Gateway Operator
participant Gateway as Gateway Deployments
note right of GH: "Test Gateway Helm Upgrade" step
GH->>CLI: modify test-gateway `values.yaml` annotations\nkubectl apply -f ...
CLI->>K8s: apply ConfigMap
K8s->>Operator: ConfigMap annotation change event
Operator->>Operator: detect annotation -> trigger Helm upgrade
Operator->>K8s: update/upgrade Gateway deployments
K8s->>Gateway: rollout updated deployments
Gateway->>Operator: update deployment annotations/status
Operator->>GH: deployment annotations show upgrade detected
GH->>CLI: wait for rollouts, port-forward controller, curl /health
CLI->>GH: return health status (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/operator-integration-test.yml (2)
1103-1103: Consider a more targeted annotation update.The sed command with the
/gflag replaces ALL occurrences ofannotations: {}throughout the values.yaml file, affecting ConfigMap annotations (line 590), service annotations (lines 602, 732, 789), and deployment annotations (lines 689, 746, 801). While this works for the test, a more surgical approach would modify only the specific field needed to trigger the Helm upgrade.🔎 More targeted approaches
Option 1: Target specific line with sed addressing
-sed -i 's/annotations: {}/annotations: {helm-upgrade-test: "true"}/g' values.yaml +# Only replace annotations under gateway.router.deployment (around line 746 in the original) +sed -i '/gateway:/{:a;N;/deployment:/!ba;s/annotations: {}/annotations: {helm-upgrade-test: "true"}/}' values.yamlOption 2: Use yq for structured YAML manipulation (more robust)
-sed -i 's/annotations: {}/annotations: {helm-upgrade-test: "true"}/g' values.yaml +# Install yq if not available and use it for precise YAML path modification +yq eval '.gateway.router.deployment.annotations.helm-upgrade-test = "true"' -i values.yaml
1105-1109: Add verification that the ConfigMap update succeeded.The test applies the ConfigMap update but doesn't verify it succeeded before waiting for the operator to detect changes. Adding a verification step improves test reliability and makes failures easier to diagnose.
🔎 Suggested verification step
kubectl create configmap test-gateway-config --from-file=values.yaml=values.yaml --dry-run=client -o yaml | kubectl apply -f - +# Verify ConfigMap was updated with the new annotation +kubectl get configmap test-gateway-config -o jsonpath='{.data.values\.yaml}' | grep -q 'helm-upgrade-test: "true"' || { + echo "ERROR: ConfigMap update failed - annotation not found" + exit 1 +} + echo "Waiting for deployment update..."
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
kubernetes/gateway-operator/internal/controller/restapi_controller.go (1)
299-309: Consider adding unit tests for time-based backoff logic.Time-based logic is error-prone and should be covered by unit tests to verify:
- Behavior when
NextRetryTimeis zero (unset)- Behavior when
NextRetryTimeis in the past- Behavior when
NextRetryTimeis in the future- Correct calculation of
RequeueAfterduration
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/operator-integration-test.ymlkubernetes/gateway-operator/internal/controller/restapi_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/operator-integration-test.yml
🔇 Additional comments (2)
kubernetes/gateway-operator/internal/controller/restapi_controller.go (2)
299-309: Excellent defensive guard for backoff enforcement.This check prevents early retry attempts when reconciliation is triggered by other events (e.g., Gateway status changes) before the backoff period expires. The logic correctly handles zero (unset) and past times, ensuring backoff is respected across reconciliation triggers.
299-309: The original review comment is based on an incorrect assumption. The actual commit title is "fix: enhance Helm upgrade verification and add health check for Gateway Controller," not "test: add Helm upgrade verification for Gateway." The "fix:" prefix is correct for production code changes, and the workflow file.github/workflows/operator-integration-test.ymlexists in the repository. No action is required.Likely an incorrect or invalid review comment.
1 participant
Purpose
Summary by CodeRabbit
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.