add certificate rotation e2e test. #425
Conversation
Signed-off-by: morvencao <[email protected]>
WalkthroughThe changes fix typos in MQTT parameter names (MQTT_CLENT_* → MQTT_CLIENT_*) across template files, remove redundant Service definitions from MQTT TLS configuration, introduce a new e2e test suite for certificate rotation with cryptographic operations, and update setup scripts to support certificate rotation testing with new Kubernetes configurations and certificate secrets. Changes
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant K8s as Kubernetes API
participant Secret as Secrets Store
participant Broker as MQTT/gRPC Broker
participant Agent as Agent
Test->>K8s: Retrieve original certs from Secrets
K8s->>Secret: Fetch MQTT & gRPC broker certs
Secret-->>Test: Return cert & key data
Test->>Test: Parse & save original certificates
Note over Test: Certificate Rotation Loop
Test->>Test: Generate new RSA key
Test->>Test: Create cert template & sign with CA
Test->>Test: PEM encode new cert & key
Test->>K8s: Update Secrets with rotated certs
K8s->>Secret: Store new certificates
Note over Agent: Reconnection phase
Broker->>Agent: TLS session invalidated
Agent->>Broker: Reconnect with new cert
Broker->>Agent: TLS handshake with rotated cert
Agent-->>Broker: Connected
Test->>K8s: Update deployment replicas (trigger sync)
Agent->>Broker: Send agent communication
Test->>K8s: Query work application status
K8s-->>Test: Confirm synchronized status
Test->>K8s: Restore original certificates
K8s->>Secret: Revert to saved certs
Test->>Test: Teardown complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
test/e2e/pkg/cert_rotation_test.go (2)
74-77: Consider replacing fixed sleep with polling.The 20-second sleep for certificate reload could introduce flakiness. Consider using
Eventuallyto poll for agent readiness instead.- // wait for certificate reload and agent reconnection - time.Sleep(20 * time.Second) + // wait for certificate reload and agent reconnection + Eventually(func() error { + pods, err := agentTestOpts.kubeClientSet.CoreV1().Pods(agentTestOpts.agentNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: "app=maestro-agent", + }) + if err != nil { + return err + } + if len(pods.Items) == 0 || pods.Items[0].Status.Phase != corev1.PodRunning { + return fmt.Errorf("agent pod not ready") + } + return nil + }, 30*time.Second, 2*time.Second).ShouldNot(HaveOccurred())
340-352: Consider using cryptographically random serial numbers.Using
time.Now().Unix()for the serial number could cause collisions if multiple certificates are generated within the same second. For e2e tests this is unlikely to be an issue, but usingcrypto/randwould be more robust.+ // Generate cryptographically random serial number + serialNumber, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate serial number: %w", err) + } + // create certificate template clientCertTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(time.Now().Unix()), + SerialNumber: serialNumber, Subject: pkix.Name{ CommonName: "test-client", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
templates/agent-template.yml(2 hunks)templates/agent-tls-template.yml(2 hunks)templates/mqtt-tls-template.yml(1 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/e2e/setup/e2e_setup.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (4)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/api/openapi/model_list.go (1)
List(21-26)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(179-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: migration
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (11)
templates/mqtt-tls-template.yml (1)
44-44: LGTM!Minor whitespace cleanup. The MQTT broker TLS configuration looks correct with required certificates and TLSv1.2.
templates/agent-template.yml (2)
66-70: LGTM - Typo fix.Good catch fixing
MQTT_CLENT_*→MQTT_CLIENT_*in the parameter definitions.
340-341: LGTM!References correctly updated to match the fixed parameter names.
templates/agent-tls-template.yml (2)
66-70: LGTM - Typo fix.Parameter names correctly fixed from
MQTT_CLENT_*toMQTT_CLIENT_*.
354-355: LGTM!Secret configuration references correctly updated to use the fixed parameter names.
test/e2e/setup/e2e_setup.sh (3)
74-79: LGTM - KubeletConfiguration for faster secret detection.The
syncFrequency: 1sandWatchstrategy for configMaps/secrets enables timely certificate rotation detection during e2e tests.
130-138: LGTM - MQTT certificate generation with rotation support.The separate
maestro-mqtt-casecret containing the CA key is appropriately scoped to the agent namespace for rotation tests.
165-178: LGTM - gRPC broker certificate generation with rotation support.The
maestro-grpc-broker-casecret with CA keys enables the rotation test to sign new client certificates dynamically.test/e2e/pkg/cert_rotation_test.go (3)
33-54: LGTM - Proper certificate backup.Good practice to deep-copy the secret data before saving to avoid reference issues during restoration.
213-253: LGTM - Well-structured certificate rotation logic.The function correctly:
- Retrieves CA credentials from dedicated secrets
- Parses and validates CA cert/key
- Signs new client certificates
- Updates the appropriate secrets
Good error handling with descriptive wrapped errors.
305-330: LGTM - Robust private key parsing.Good approach handling both PKCS1 and PKCS8 formats, which covers keys generated by different tools.
| By("verifying the deployment is not updated to 2 replicas durating agent certificate expiration...") | ||
| Consistently(func() error { | ||
| deployment, err := agentTestOpts.kubeClientSet.AppsV1().Deployments("default").Get(ctx, deployName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if deployment.Spec.Replicas != nil && *deployment.Spec.Replicas == 2 { | ||
| return fmt.Errorf("expecte replicas not equal to 2, got %d", *deployment.Spec.Replicas) | ||
| } | ||
| return nil | ||
| }, 30*time.Second, 2*time.Second).Should(BeNil()) |
There was a problem hiding this comment.
Fix typos in error messages.
Line 164: "durating" → "during"
Line 171: "expecte" → "expected"
- By("verifying the deployment is not updated to 2 replicas durating agent certificate expiration...")
+ By("verifying the deployment is not updated to 2 replicas during agent certificate expiration...")
Consistently(func() error {
deployment, err := agentTestOpts.kubeClientSet.AppsV1().Deployments("default").Get(ctx, deployName, metav1.GetOptions{})
if err != nil {
return err
}
if deployment.Spec.Replicas != nil && *deployment.Spec.Replicas == 2 {
- return fmt.Errorf("expecte replicas not equal to 2, got %d", *deployment.Spec.Replicas)
+ return fmt.Errorf("expected replicas not equal to 2, got %d", *deployment.Spec.Replicas)
}
return nil
}, 30*time.Second, 2*time.Second).Should(BeNil())🤖 Prompt for AI Agents
In test/e2e/pkg/cert_rotation_test.go around lines 164 to 174, fix the typos in
the test messages: change "durating" to "during" in the By(...) description and
change "expecte" to "expected" in the fmt.Errorf error message so the strings
read correctly; update the two literal strings accordingly without altering
logic or formatting.
1 participant
ref: https://issues.redhat.com/browse/ACM-27141