add certificate rotation e2e test. - #425
Conversation
WalkthroughRenamed two misspelled MQTT template parameters, added BROKER_CLIENT_CERT_REFRESH_DURATION and propagated it as CERT_CALLBACK_REFRESH_DURATION into the agent Deployment; removed two MQTT Service objects; added E2E cert-rotation tests and switched test cert generation to RSA; updated test scripts, Makefile, and README. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
1da7211 to
33f4a2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
164-172: Fix remaining typos in test messages.The earlier review note about these strings still applies; suggest correcting them for clarity:
- 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...") @@ - if deployment.Spec.Replicas != nil && *deployment.Spec.Replicas == 2 { - return fmt.Errorf("expecte replicas not equal to 2, got %d", *deployment.Spec.Replicas) + if deployment.Spec.Replicas != nil && *deployment.Spec.Replicas == 2 { + return fmt.Errorf("expected replicas not equal to 2, got %d", *deployment.Spec.Replicas)
📜 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 (8)
README.md(0 hunks)templates/agent-template.yml(2 hunks)templates/agent-tls-template.yml(2 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(0 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (3)
- test/setup/deploy_agent.sh
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/agent-tls-template.yml
🧰 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(182-226)
🪛 Shellcheck (0.11.0)
test/setup/env_setup.sh
[warning] 156-156: mqttCertDir is referenced but not assigned.
(SC2154)
⏰ 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: upgrade
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
🔇 Additional comments (6)
templates/agent-template.yml (1)
66-71: MQTT client cert/key rename is consistent across parameters and config.The
MQTT_CLIENT_CERT/MQTT_CLIENT_KEYparameters and their use inclientCertFile/clientKeyFileare aligned and self‑consistent; no issues spotted here.Also applies to: 340-341
test/setup/env_setup.sh (2)
88-93: Kubelet config patch for faster secret/config reload looks appropriate.Adding
syncFrequency: "1s"withconfigMapAndSecretChangeDetectionStrategy: "Watch"should help the cluster react quickly to cert/secret changes during rotation tests.
144-147: Explicit RSA key type keeps step-generated certs compatible with test helpers.Switching MQTT and gRPC (broker) CA/server/client cert generation to
--kty RSAaligns with the Go rotation helpers that assume RSA keys, avoiding parse failures on ECDSA keys.Also applies to: 174-175, 183-183
test/setup/deploy_server.sh (1)
59-60: RSA key selection for gRPC service certs matches rotation test expectations.Adding
--kty RSAformaestro-grpc-ca,maestro-grpc-server, andmaestro-grpc-clientbrings these certs in line with the test-side RSA parsing logic and avoids mixed key types.Also applies to: 68-68
test/e2e/pkg/cert_rotation_test.go (2)
26-129: End-to-end cert rotation flow and helper implementations look solid.The ordered Ginkgo flow (save original secrets → exercise current certs → short‑lived rotation → long‑lived rotation → restore) is coherent, and the helper functions for parsing CAs, generating RSA client certs, and patching secrets are straightforward and defensive (good error propagation on all secret/parse/update steps).
Also applies to: 213-365
213-294: Verify that the gRPC CA secretmaestro-grpc-broker-cais created for the agent namespace.
rotateCertificatesassumes the presence of amaestro-grpc-broker-casecret (withca.crtandca.key) inagentTestOpts.agentNamespace. In the snippets shown fortest/setup/env_setup.shandtest/setup/deploy_server.sh, only MQTT CA (maestro-mqtt-ca) and gRPC cert bundle (maestro-grpc-broker-cert/maestro-grpc-cert) secrets are clearly created.If no other setup script creates
maestro-grpc-broker-cain the agent namespace, this test will fail when callingSecrets(...).Get(ctx, "maestro-grpc-broker-ca", ...).Please double‑check the e2e setup path and, if needed, add a matching secret creation for the broker CA so this helper can succeed.
33f4a2c to
62bf7d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
164-171: Fix typos in test messages.Two typos in the test:
- Line 164: "durating" → "during"
- Line 171: "expecte" → "expected"
Apply this diff:
- 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())
🧹 Nitpick comments (3)
test/setup/env_setup.sh (1)
155-156: Consider adding kubectl delete for idempotency.The secret creation will fail if the secret already exists on subsequent runs. Consider preceding this with a delete command for consistency with the pattern used on lines 149-151.
Apply this diff to make the script re-runnable:
kubectl create secret generic maestro-agent-certs -n "${agent_namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=client.crt=${mqtt_cert_dir}/agent-client.crt --from-file=client.key=${mqtt_cert_dir}/agent-client.key + kubectl delete secret maestro-mqtt-ca -n "${agent_namespace}" --ignore-not-found # create a separate secret for MQTT CA keys used in certificate rotation tests kubectl create secret generic maestro-mqtt-ca -n ${agent_namespace} --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.keyNote: The past review comment about
mqttCertDirbeing undefined has been resolved—the current code correctly uses${mqtt_cert_dir}.test/e2e/pkg/cert_rotation_test.go (2)
76-76: Consider polling for agent readiness instead of fixed sleep.The 20-second sleep assumes certificate reload and agent reconnection complete within that time. Consider replacing with a polling mechanism that checks agent pod status or connection health for more robust cleanup.
Example polling approach:
// wait for agent pod to be ready after certificate restoration 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 { return fmt.Errorf("no agent pods found") } pod := pods.Items[0] if pod.Status.Phase != corev1.PodRunning { return fmt.Errorf("agent pod not running") } for _, condition := range pod.Status.Conditions { if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { return nil } } return fmt.Errorf("agent pod not ready") }, 1*time.Minute, 2*time.Second).ShouldNot(HaveOccurred())
137-137: Consider more precise timing for certificate expiration tests.The 20-second sleeps (lines 137 and 182) assume certificates expire and reload within that window. For the 5-second expiration test, a more precise wait (e.g., 10-15 seconds) might reduce test time while maintaining reliability.
Example timing adjustment:
It("should rotate client certificate with short expiration for maestro agent", func() { err := rotateCertificates(ctx, 5*time.Second) Expect(err).ShouldNot(HaveOccurred()) By("waiting for certificate expiration...") - time.Sleep(20 * time.Second) + time.Sleep(15 * time.Second) // 5s expiration + 10s buffer for cert reload })Also applies to: 182-182
📜 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 (8)
README.md(0 hunks)templates/agent-template.yml(2 hunks)templates/agent-tls-template.yml(2 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_server.sh
- templates/agent-tls-template.yml
🧰 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(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: upgrade
- GitHub Check: e2e
🔇 Additional comments (10)
test/setup/deploy_agent.sh (1)
44-48: LGTM! MQTT configuration exports are appropriate for certificate-based authentication.The exported variables configure MQTT client authentication using TLS certificates without username/password (hence
mqtt_password_file="/dev/null"), which aligns with the certificate rotation testing objectives of this PR.templates/agent-template.yml (1)
66-66: LGTM! Typo corrections are applied consistently.The parameter names are corrected from
MQTT_CLENT_CERTandMQTT_CLENT_KEYtoMQTT_CLIENT_CERTandMQTT_CLIENT_KEY, and all references are updated accordingly.Also applies to: 69-69, 340-341
test/setup/env_setup.sh (2)
88-93: LGTM! Kubelet configuration optimized for certificate rotation testing.The
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"settings enable rapid detection of secret changes, which is essential for testing certificate rotation. This configuration is appropriate for the test environment.
144-147: LGTM! RSA key type standardization aligns with test requirements.Explicitly specifying
--kty RSAensures consistent key type generation and aligns with the certificate rotation test code intest/e2e/pkg/cert_rotation_test.go, which expects RSA private keys.test/e2e/pkg/cert_rotation_test.go (6)
33-54: LGTM! BeforeAll hook properly preserves original certificate state.The setup correctly saves original MQTT and gRPC broker certificates using deep copies, ensuring the test can restore the original state after completion.
98-130: LGTM! Initial connectivity test properly validates agent functionality.The test correctly verifies both the deployment of resources on the agent cluster and the status reporting back to the server, establishing baseline connectivity before certificate rotation.
177-209: LGTM! Long expiration test validates successful agent reconnection.The test correctly verifies that after rotating to long-lived certificates (3600 seconds), the agent successfully reconnects and processes work updates.
214-294: LGTM! Certificate rotation function properly handles both MQTT and gRPC certificates.The function correctly retrieves CA secrets, parses certificates/keys, signs new client certificates, and updates agent secrets with comprehensive error handling.
296-330: LGTM! Certificate and key parsing functions are robust.The
parseCertificateandparsePrivateKeyfunctions properly handle PEM decoding withparsePrivateKeysupporting both PKCS1 and PKCS8 formats, which provides good compatibility.
332-365: LGTM! Client certificate signing implementation is secure and appropriate for testing.The function generates proper RSA 2048-bit keys, creates certificates with appropriate key usage for client authentication, and returns PEM-encoded output. The timestamp-based serial number is acceptable for test environments.
62bf7d6 to
0c83606
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/setup/env_setup.sh (1)
156-156: Variable quoting inconsistency in kubectl secret creation.Line 156 uses unquoted
${agent_namespace}while line 189 uses quoted"$agent_namespace". For shell robustness and consistency with otherkubectlinvocations in the script (e.g., lines 149–154), both should use quoted variables.Apply this diff for consistency:
- kubectl create secret generic maestro-mqtt-ca -n ${agent_namespace} --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.key + kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" --from-file=ca.key="${mqtt_cert_dir}/ca.key"Also applies to: 189-189
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_agent.sh
- test/e2e/pkg/cert_rotation_test.go
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
🔇 Additional comments (9)
test/setup/deploy_server.sh (1)
59-68: Consistent RSA key type specification across gRPC certificate setup.The
--kty RSAflag additions align well with the broader certificate generation changes inenv_setup.shanddeploy_server.sh. All SANs, templates, and other flags are preserved correctly.Makefile (1)
111-112: New broker certificate refresh parameter properly introduced.The variable declaration and template parameter propagation follow existing patterns. Default value of
5mis reasonable for certificate rotation testing.Also applies to: 335-335
test/setup/env_setup.sh (2)
88-93: KubeletConfiguration patch enables fast secret detection for rotation tests.The
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"settings are essential for the kubelet to detect certificate updates quickly during rotation testing. This is correctly implemented.
144-147: RSA certificate generation consistently applied across MQTT and gRPC setups.All certificate creation commands properly specify
--kty RSA. SANs and certificate names are correctly configured for their respective brokers. The template references are preserved.Also applies to: 174-175, 183-183
templates/agent-template.yml (2)
66-70: MQTT parameter name typo fixed: MQTT_CLENT → MQTT_CLIENT.Parameter names are corrected throughout the template. All references in the MQTT config (lines 346–347) use the corrected names consistently.
Also applies to: 346-347
72-74: New broker certificate refresh parameter properly integrated.The
BROKER_CLIENT_CERT_REFRESH_DURATIONparameter is defined with a sensible default (5m) and injected into the Maestro agent container asCERT_CALLBACK_REFRESH_DURATIONenvironment variable. This allows the application to control certificate refresh frequency via deployment configuration.Also applies to: 310-311
templates/agent-tls-template.yml (3)
66-70: MQTT parameter name typo fixed consistently across TLS template.Mirrors the same parameter name corrections as
agent-template.yml. All configuration references properly use the correctedMQTT_CLIENT_CERTandMQTT_CLIENT_KEYnames.Also applies to: 362-363
72-74: Broker certificate refresh parameter aligned with non-TLS template.The same parameter and environment variable injection pattern from
agent-template.ymlis consistently applied here.Also applies to: 310-311
324-327: Volume mounts for TLS certificate rotation properly configured.The additional volume mounts (
mqtt-certs,grpc-broker-cert) reference secrets created byenv_setup.sh(lines 156 and 189). Mount paths align with application expectations. Theoptional: trueflag gracefully handles cases where gRPC broker is not in use.Also applies to: 332-339
0c83606 to
6406c5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
templates/service-template.yml (1)
214-221: Re-evaluate making the MQTT secret volume optional.Marking the
maestro-mqttsecret volume asoptional: truewill let pods schedule even when the secret (and thus/secrets/${MESSAGE_DRIVER_TYPE}/config.yaml) is missing, shifting failure from admission time to runtime (likely crashloop). If the broker config is required wheneverMESSAGE_DRIVER_TYPE=mqtt, consider keeping this required for that mode or documenting that the optional flag is only to support non‑MQTT/test scenarios.test/setup/deploy_agent.sh (1)
44-50: Env wiring for MQTT certs and refresh duration looks consistent.The new mqtt_* exports and
broker_client_cert_refresh_duration=5salign with the updated agent TLS templates and Makefile plumbing for BROKER_CLIENT_CERT_REFRESH_DURATION and CERT_CALLBACK_REFRESH_DURATION; this is appropriate for the e2e rotation scenario. Just keep this script scoped to tests so the aggressive 5s refresh interval doesn’t accidentally bleed into other environments.test/setup/env_setup.sh (1)
76-95: Kind kubelet tuning, RSA cert generation, and CA secrets align with rotation tests.The kubeadmConfigPatches (1s syncFrequency + Watch change detection) plus switching MQTT/gRPC certs to RSA and introducing
maestro-mqtt-ca/maestro-grpc-broker-cain the agent namespace all line up with the new Go e2e helpers (which parse RSA keys and expectca.crt/ca.keyin those secrets). This should make certificate updates visible to the agent quickly enough for the rotation tests to be reliable.Also applies to: 138-157, 168-190
templates/agent-tls-template.yml (1)
66-75: MQTT client cert parameters and refresh duration wiring look consistent.Renaming to
MQTT_CLIENT_CERT/MQTT_CLIENT_KEY, introducingBROKER_CLIENT_CERT_REFRESH_DURATION, wiring it intoCERT_CALLBACK_REFRESH_DURATION, and updating the MQTT config to use the new parameters all look consistent with the deployment scripts and the cert-rotation behavior being tested.Also applies to: 308-312, 352-363
test/e2e/pkg/cert_rotation_test.go (1)
166-177: Clarify test intent and naming in the reconnection checks.The two reconnection tests share the same description (
"should verify agent reconnects with new certificates") but validate different behaviors (blocked update during short expiry vs successful update after long expiry). Consider giving them distinct descriptions or adding a brief comment to distinguish the short‑expiry “no update” case from the long‑expiry “update succeeds” case for clearer test output.Also applies to: 187-211
templates/agent-template.yml (1)
66-75: Agent template param and env updates are consistent with TLS variant and tests.The MQTT client cert/key param renames, new
BROKER_CLIENT_CERT_REFRESH_DURATIONdefault, and theCERT_CALLBACK_REFRESH_DURATIONenv var mirror the TLS template and align with the deploy scripts and rotation tests. MQTT config now correctly references${MQTT_CLIENT_CERT}/${MQTT_CLIENT_KEY}instead of the misspelled names.Also applies to: 308-312, 342-347
📜 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 (10)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)templates/service-template.yml(1 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_server.sh
- Makefile
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e
🔇 Additional comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
331-364: Cert-signing helper is appropriate for tests and matches RSA key material.
signClientCertificategenerating a fresh 2048‑bit RSA key and a short‑lived client certificate (clientAuth, digitalSignature+keyEncipherment) matches the RSA keys produced byenv_setup.shand is suitable for this e2e scenario. No functional issues here.
6406c5a to
90ac9c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/e2e/pkg/cert_rotation_test.go (3)
37-55: Harden error handling to avoid nil dereference on non-NotFound errors.The condition
if !errors.IsNotFound(err)passes whenerr == nil(correct) but also whenerris any other error (incorrect), in which casesecretmay be nil causing a panic on line 41.Apply this fix pattern:
By("saving original MQTT certificate") secret, err := agentTestOpts.kubeClientSet.CoreV1().Secrets(agentTestOpts.agentNamespace).Get(ctx, "maestro-agent-certs", metav1.GetOptions{}) - if !errors.IsNotFound(err) { + if err == nil { originalMQTTCerts = make(map[string][]byte) for key, value := range secret.Data { originalMQTTCerts[key] = make([]byte, len(value)) copy(originalMQTTCerts[key], value) } + } else if !errors.IsNotFound(err) { + Expect(err).ShouldNot(HaveOccurred()) } By("saving original gRPC broker certificate") grpcSecret, err := agentTestOpts.kubeClientSet.CoreV1().Secrets(agentTestOpts.agentNamespace).Get(ctx, "maestro-grpc-broker-cert", metav1.GetOptions{}) - if !errors.IsNotFound(err) { + if err == nil { originalGRPCBrokerCerts = make(map[string][]byte) for key, value := range grpcSecret.Data { originalGRPCBrokerCerts[key] = make([]byte, len(value)) copy(originalGRPCBrokerCerts[key], value) } + } else if !errors.IsNotFound(err) { + Expect(err).ShouldNot(HaveOccurred()) }
216-252: Fix error handling to prevent silent failures or panics.Same issue as in BeforeAll: the condition
if !errors.IsNotFound(err)will proceed into the block whenGetreturns an unexpected error (e.g., network timeout), potentially causing a nil dereference or silently skipping rotation.mqttCASecret, err := agentTestOpts.kubeClientSet.CoreV1().Secrets(agentTestOpts.agentNamespace).Get(ctx, "maestro-mqtt-ca", metav1.GetOptions{}) - if !errors.IsNotFound(err) { + if errors.IsNotFound(err) { + // MQTT rotation not applicable - CA secret not found + } else if err != nil { + return fmt.Errorf("failed to get maestro-mqtt-ca secret: %w", err) + } else { mqttCACertPEM := mqttCASecret.Data["ca.crt"] // ... rest of MQTT rotation logic }
254-290: Apply same error handling fix for gRPC broker secrets.The gRPC secret fetching has the same error handling issue.
gRPCBrokerCASecret, err := agentTestOpts.kubeClientSet.CoreV1().Secrets(agentTestOpts.agentNamespace).Get(ctx, "maestro-grpc-broker-ca", metav1.GetOptions{}) - if !errors.IsNotFound(err) { + if errors.IsNotFound(err) { + // gRPC rotation not applicable - CA secret not found + } else if err != nil { + return fmt.Errorf("failed to get maestro-grpc-broker-ca secret: %w", err) + } else { gRPCBrokerCACertPEM := gRPCBrokerCASecret.Data["ca.crt"] // ... rest of gRPC rotation logic }
🧹 Nitpick comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
142-177: Consider renaming to avoid duplicate test description.This test (line 142) has the same description "should verify agent reconnects with new certificates" as the test at line 187. While Ginkgo will run both, identical descriptions can make test reports confusing.
- It("should verify agent reconnects with new certificates", func() { + It("should verify agent fails to process updates with expired certificates", func() {
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/setup/deploy_server.sh
- Makefile
- templates/agent-tls-template.yml
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (2)
test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e
🔇 Additional comments (13)
test/setup/deploy_agent.sh (1)
44-50: LGTM! Environment variables for TLS-based agent deployment.The exported variables align with the template parameter renames (
MQTT_CLIENT_CERT,MQTT_CLIENT_KEY) and the shortbroker_client_cert_refresh_duration=5sis appropriate for exercising certificate rotation in tests.test/setup/env_setup.sh (4)
88-93: Good addition for faster secret detection in cert rotation tests.The KubeletConfiguration with
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"ensures the Kubelet detects secret updates quickly, which is essential for testing certificate rotation behavior.
144-147: LGTM! Consistent RSA key generation for all certificates.Using
--kty RSAconsistently ensures compatibility with the certificate rotation test'sparsePrivateKeyfunction, which parses RSA keys in both PKCS1 and PKCS8 formats.
155-156: LGTM! CA secrets for certificate rotation tests.Creating separate CA secrets (with both
ca.crtandca.key) enables the e2e test to sign new client certificates during rotation testing.
187-189: LGTM! gRPC broker certificate secrets properly provisioned.The split of secrets between namespaces and inclusion of the CA secret in
agent_namespacecorrectly supports the certificate rotation workflow.templates/agent-template.yml (3)
66-74: LGTM! Typo fixes and new refresh duration parameter.The parameter renames (
MQTT_CLENT_*→MQTT_CLIENT_*) fix the typos, andBROKER_CLIENT_CERT_REFRESH_DURATIONwith a sensible default of "5m" enables configurable certificate refresh intervals.
310-311: LGTM! Environment variable for certificate refresh duration.Propagating
BROKER_CLIENT_CERT_REFRESH_DURATIONasCERT_CALLBACK_REFRESH_DURATIONto the container enables the agent to pick up certificate changes based on the configured interval.
346-347: LGTM! Updated secret config to use corrected parameter names.The MQTT secret configuration now correctly references
${MQTT_CLIENT_CERT}and${MQTT_CLIENT_KEY}.test/e2e/pkg/cert_rotation_test.go (5)
1-24: LGTM! Clean imports and package setup for certificate rotation tests.The imports cover all necessary crypto primitives and Kubernetes client types for the test implementation.
58-98: LGTM! Proper cleanup and certificate restoration.The AfterAll block properly guards restoration with
len(original*Certs) > 0checks and includes reasonable cleanup of test resources.
100-132: LGTM! Initial connectivity verification test.The test properly creates a ManifestWork, verifies deployment creation, and checks work status conditions.
295-329: LGTM! Well-implemented certificate parsing helpers.The
parseCertificateandparsePrivateKeyfunctions properly handle PEM decoding and support both PKCS1 and PKCS8 key formats, ensuring compatibility with different key generation tools.
331-364: LGTM! Clean client certificate signing implementation.The
signClientCertificatefunction correctly generates a 2048-bit RSA key, creates a properly constrained client certificate withExtKeyUsageClientAuth, and encodes the output in PEM format.
e789268 to
6365119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/e2e/pkg/cert_rotation_test.go (3)
62-103: Make cleanup more resilient to partial environment teardown (avoid hard-failing on NotFound restores).
AfterAllcurrentlyExpect(err).ShouldNot(HaveOccurred())when re-GET’ing secrets to restore; if the agent namespace or secrets are already gone, cleanup will fail and mask the real test failure. Consider treating NotFound as a no-op for restores.
142-144: PreferEventuallyover fixedtime.Sleepfor certificate reload/reconnect waits.
The hard-coded 10s/30s sleeps can be flaky (slow clusters) and slow (fast clusters). If there’s a signal you can poll (e.g., agent pod ready condition, observed SecretresourceVersion, successful work status update), useEventuallyon that.Also applies to: 81-83
309-341: Use a stronger (unique) certificate serial number source thantime.Now().Unix().
Seconds-resolution serials can collide if certs are minted quickly (or in parallel). Preferrand.Int(rand.Reader, ...).@@ - clientCertTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(time.Now().Unix()), + serialLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialLimit) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate serial number: %w", err) + } + clientCertTemplate := &x509.Certificate{ + SerialNumber: serialNumber, Subject: pkix.Name{ CommonName: "test-client", },Makefile (1)
111-113: Make--paramformatting consistent (NAME=$(value)inside the quotes).
Current form likely works via shell token concatenation, but it’s easier to read and less error-prone if kept consistent with the other--param="NAME=VALUE"entries.@@ - --param="BROKER_CLIENT_CERT_REFRESH_DURATION"=$(broker_client_cert_refresh_duration) \ + --param="BROKER_CLIENT_CERT_REFRESH_DURATION=$(broker_client_cert_refresh_duration)" \Also applies to: 335-336
test/setup/env_setup.sh (1)
149-157: Makemaestro-mqtt-casecret creation re-runnable and quote variables.
Other secrets are deleted first; this one should match to avoid rerun failures underbash -e.@@ - # create a separate secret for MQTT CA keys used in certificate rotation tests - kubectl create secret generic maestro-mqtt-ca -n ${agent_namespace} --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.key + # create a separate secret for MQTT CA keys used in certificate rotation tests + kubectl delete secret maestro-mqtt-ca -n "${agent_namespace}" --ignore-not-found + kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" \ + --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" \ + --from-file=ca.key="${mqtt_cert_dir}/ca.key"
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_server.sh
- test/setup/deploy_agent.sh
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: upgrade
🔇 Additional comments (4)
test/setup/env_setup.sh (1)
88-94: Kubelet watch-based secret/config reload config is a good fit for rotation tests.
This should make certificate refresh behavior show up faster and more deterministically in KinD.templates/agent-template.yml (2)
66-75: Template param rename + refresh duration wiring looks consistent.
MQTT client cert/key references and the new BROKER_CLIENT_CERT_REFRESH_DURATION default align with the intended rotation test behavior.Also applies to: 310-312, 346-347
310-312: Verify the agent actually readsCERT_CALLBACK_REFRESH_DURATION— this environment variable is set in the templates but has no references in the maestro codebase.The variable appears to be dead wiring. No references to
CERT_CALLBACK_REFRESH_DURATIONorBROKER_CLIENT_CERT_REFRESH_DURATIONwere found in any Go code within the repository, including the agent code at./cmd/maestro/agent/cmd.go. If this configuration is intended for an external dependency (such as open-cluster-management.io/ocm libraries), that should be documented. Otherwise, this environment variable and its wiring should be removed.templates/agent-tls-template.yml (1)
66-75: TLS template mirrors the non-TLS template changes cleanly (good consistency).Also applies to: 310-312, 362-363
d7219a2 to
2a71cf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/e2e/pkg/cert_rotation_test.go (3)
63-83: Avoid clobbering concurrent Secret updates when restoring
secret.Data = original...overwrites the entiredatamap; if other keys are added/rotated during the test window, they’ll be dropped. Safer to restore only the keys you changed (or merge).- secret.Data = originalMQTTCerts + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + for k, v := range originalMQTTCerts { + secret.Data[k] = v + }
323-335: Harden cert issuance against clock skew + serial collisions
SerialNumber: big.NewInt(time.Now().Unix())can collide within the same second, andNotBefore: nowcan fail with small clock skew.- now := time.Now() + now := time.Now() + serialLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialLimit) + 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", }, - NotBefore: now.UTC(), + NotBefore: now.Add(-2 * time.Minute).UTC(), NotAfter: now.Add(duration).UTC(), KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }
171-179: Minor: error message grammar (“replicas”)- return fmt.Errorf("expected 2 replica, got %d", *deployment.Spec.Replicas) + return fmt.Errorf("expected 2 replicas, got %d", *deployment.Spec.Replicas)test/setup/env_setup.sh (1)
155-156: Makemaestro-mqtt-casecret creation rerunnable + quote namespace
Right nowkubectl create secret ...will fail on repeated runs; also quote${agent_namespace}for consistency.# create a separate secret for MQTT CA keys used in certificate rotation tests - kubectl create secret generic maestro-mqtt-ca -n ${agent_namespace} --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.key + kubectl delete secret maestro-mqtt-ca -n "${agent_namespace}" --ignore-not-found + kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" \ + --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" \ + --from-file=ca.key="${mqtt_cert_dir}/ca.key"
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_agent.sh
- Makefile
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (2)
test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: e2e-broadcast-subscription
- GitHub Check: e2e
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-grpc-broker
- GitHub Check: upgrade
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
🔇 Additional comments (4)
templates/agent-tls-template.yml (1)
66-75: Consistent with non-TLS template changes
Mirroring the MQTT param rename and refresh-duration wiring here looks consistent; once the repo-wide rename + env-var consumption is verified, this should be good.Also applies to: 310-312, 362-363
test/setup/env_setup.sh (1)
88-93: Kind explicitly supports KubeletConfiguration via kubeadmConfigPatches. The patch syntax in this code aligns with kind and kubeadm documentation, and configMapAndSecretChangeDetectionStrategy "Watch" is a valid KubeletConfiguration field. No verification issue here.test/setup/deploy_server.sh (1)
59-60: The--kty RSAflag is fully supported by the pinned Step CLI v0.26.2Step v0.26.2 is explicitly installed via
test/setup/env_setup.shand fully supports the--kty RSAflag (introduced in v0.11.0). The certificate generation changes are safe and correct.templates/agent-template.yml (1)
66-75: No action needed. All parameter names are correct and consistently used throughout the codebase. There is noMQTT_CLENT_*typo to fix—the parameters are already namedMQTT_CLIENT_CERTandMQTT_CLIENT_KEYconsistently across templates and the Makefile. TheBROKER_CLIENT_CERT_REFRESH_DURATIONparameter is properly wired in the template with a default value of "5m" and correctly maps to theCERT_CALLBACK_REFRESH_DURATIONenvironment variable for the agent.
2a71cf0 to
bbc474b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/setup/env_setup.sh (1)
138-157: Make secret creation idempotent even when the cert dir already exists (prevents e2e “rotation didn’t run” failures on reruns).
Right now, if${PWD}/test/_output/certs/mqttalready exists, you skip (re)creatingmaestro-mqtt-ca/maestro-agent-certsetc—even if those secrets were deleted or the cluster is new. That can make the new e2e test fail withrotated == false.- if [ ! -d "$mqtt_cert_dir" ]; then - # create certs - mkdir -p "$mqtt_cert_dir" + # create certs (only if missing) + if [ ! -d "$mqtt_cert_dir" ]; then + mkdir -p "$mqtt_cert_dir" step certificate create "maestro-mqtt-ca" ${mqtt_cert_dir}/ca.crt ${mqtt_cert_dir}/ca.key --kty RSA --profile root-ca --no-password --insecure step certificate create "maestro-mqtt-broker" ${mqtt_cert_dir}/server.crt ${mqtt_cert_dir}/server.key --kty RSA -san maestro-mqtt -san maestro-mqtt.maestro --profile leaf --ca ${mqtt_cert_dir}/ca.crt --ca-key ${mqtt_cert_dir}/ca.key --no-password --insecure step certificate create "maestro-server-client" ${mqtt_cert_dir}/server-client.crt ${mqtt_cert_dir}/server-client.key --kty RSA --profile leaf --ca ${mqtt_cert_dir}/ca.crt --ca-key ${mqtt_cert_dir}/ca.key --no-password --insecure step certificate create "maestro-agent-client" ${mqtt_cert_dir}/agent-client.crt ${mqtt_cert_dir}/agent-client.key --kty RSA --profile leaf --ca ${mqtt_cert_dir}/ca.crt --ca-key ${mqtt_cert_dir}/ca.key --no-password --insecure - # create secrets - kubectl delete secret maestro-mqtt-certs -n "${namespace}" --ignore-not-found - kubectl delete secret maestro-server-certs -n "${namespace}" --ignore-not-found - kubectl delete secret maestro-agent-certs -n "${agent_namespace}" --ignore-not-found - kubectl create secret generic maestro-mqtt-certs -n "${namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=server.crt=${mqtt_cert_dir}/server.crt --from-file=server.key=${mqtt_cert_dir}/server.key - kubectl create secret generic maestro-server-certs -n "${namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=client.crt=${mqtt_cert_dir}/server-client.crt --from-file=client.key=${mqtt_cert_dir}/server-client.key - kubectl create secret generic maestro-agent-certs -n "${agent_namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=client.crt=${mqtt_cert_dir}/agent-client.crt --from-file=client.key=${mqtt_cert_dir}/agent-client.key - # create a separate secret for MQTT CA keys used in certificate rotation tests - kubectl create secret generic maestro-mqtt-ca -n ${agent_namespace} --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.key - fi + fi + + # (re)create secrets from whatever is on disk + kubectl delete secret maestro-mqtt-certs -n "${namespace}" --ignore-not-found + kubectl delete secret maestro-server-certs -n "${namespace}" --ignore-not-found + kubectl delete secret maestro-agent-certs -n "${agent_namespace}" --ignore-not-found + kubectl delete secret maestro-mqtt-ca -n "${agent_namespace}" --ignore-not-found + kubectl create secret generic maestro-mqtt-certs -n "${namespace}" --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" --from-file=server.crt="${mqtt_cert_dir}/server.crt" --from-file=server.key="${mqtt_cert_dir}/server.key" + kubectl create secret generic maestro-server-certs -n "${namespace}" --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" --from-file=client.crt="${mqtt_cert_dir}/server-client.crt" --from-file=client.key="${mqtt_cert_dir}/server-client.key" + kubectl create secret generic maestro-agent-certs -n "${agent_namespace}" --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" --from-file=client.crt="${mqtt_cert_dir}/agent-client.crt" --from-file=client.key="${mqtt_cert_dir}/agent-client.key" + # secret for rotation tests (includes CA private key) + kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" --from-file=ca.key="${mqtt_cert_dir}/ca.key"
♻️ Duplicate comments (1)
test/setup/env_setup.sh (1)
155-156: Namespace quoting consistency (already flagged previously).
This is the same style fix as earlier reviews; keeping-n "${agent_namespace}"avoids edge cases.
🧹 Nitpick comments (4)
test/setup/env_setup.sh (1)
168-191: Apply the same “generate once, (re)create secrets always” pattern for gRPC broker CA + cert secrets.
Same rerun/flake risk as MQTT (dir exists ⇒ secrets might not).test/e2e/pkg/cert_rotation_test.go (3)
62-102: Prefer condition-based waits over fixed sleeps in e2e cleanup.
time.Sleep(30s)will be either too long (slow suite) or too short (flake) depending on environment.
186-283: Rotation logic is much safer now; consider retry-on-conflict for Secret updates.
Update(...)can fail with conflicts if something else touches the Secret; a small retry loop would reduce flakes.
321-354: Harden cert generation: avoid clock-skew “not yet valid” flakes + ensure unique serial numbers.
NotBefore=nowcan fail if the agent/node clock is slightly behind; serials based on Unix seconds can collide across fast test runs.func signClientCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, duration time.Duration) (certPEM, keyPEM []byte, err error) { @@ - now := time.Now() + now := time.Now() + serialLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialLimit) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate serial number: %w", err) + } @@ clientCertTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(time.Now().Unix()), + SerialNumber: serialNumber, Subject: pkix.Name{ CommonName: "test-client", }, - NotBefore: now.UTC(), + NotBefore: now.Add(-1 * time.Minute).UTC(), NotAfter: now.Add(duration).UTC(), KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- templates/mqtt-tls-template.yml
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/setup/deploy_server.sh
- test/setup/deploy_agent.sh
- Makefile
- templates/agent-tls-template.yml
🧰 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(182-226)
⏰ 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: e2e-with-istio
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
🔇 Additional comments (7)
templates/agent-template.yml (3)
341-347: MQTT config update is consistent with the renamed params.
clientCertFile/clientKeyFilenow matchMQTT_CLIENT_CERT/MQTT_CLIENT_KEY.
66-75: Parameter rename is complete and consistent across all call-sites. No references to the oldMQTT_CLENT_*names found; bothMQTT_CLIENT_CERTandMQTT_CLIENT_KEYare used correctly in the Makefile and all template files. The newBROKER_CLIENT_CERT_REFRESH_DURATIONparameter is also properly integrated in agent-template.yml and agent-tls-template.yml.
308-312: Verify the agent actually consumesCERT_CALLBACK_REFRESH_DURATION(name + semantics).The template maps
${BROKER_CLIENT_CERT_REFRESH_DURATION}to the environment variableCERT_CALLBACK_REFRESH_DURATION, but the agent is deployed from the upstream OCM library (open-cluster-management.io/ocm). No consumption ofCERT_CALLBACK_REFRESH_DURATIONexists in the maestro repository, and the env var name mismatch suggests a potential misconfiguration. Verify against the OCM library source code that the agent expects this exact environment variable name and interprets it as intended for broker certificate refresh timing.test/setup/env_setup.sh (1)
74-95: KinD kubelet patch is reasonable for fast secret/config propagation; verify kind/K8s accept these fields.
EspeciallyconfigMapAndSecretChangeDetectionStrategy: "Watch"with your KinD node image/K8s version.test/e2e/pkg/cert_rotation_test.go (3)
33-60: Original secret snapshotting is safe (deep-copies byte slices).
Good setup for restoring state after rotation.
104-136: Connectivity baseline test is solid.
Creates work, verifies applied workload, and checksApplied/Availableconditions.
285-319: PEM parsing helpers look fine for RSA-only test fixtures.
Clear errors and supports PKCS1 + PKCS8 RSA keys.
d277a3c to
508d955
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
321-354: Review the certificate expiration timing and key usage.The
signClientCertificatefunction generates certificates with:
- 2048-bit RSA keys (adequate for testing)
- Serial number based on
time.Now().Unix()(may collide if called multiple times in same second)KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEnciphermentExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}Considerations:
- Serial number collision: Using
time.Now().Unix()for serial numbers could cause collisions ifsignClientCertificateis called multiple times within the same second. Consider usingcrypto/randfor serial number generation or including nanoseconds.- Key size: 2048-bit RSA is acceptable for testing but consider 4096-bit for production. Since this is test-only code, 2048 is fine.
- Key usage: The combination of
DigitalSignature + KeyEnciphermentwithClientAuthis appropriate for TLS client certificates.Consider improving serial number generation:
- SerialNumber: big.NewInt(time.Now().Unix()), + SerialNumber: big.NewInt(time.Now().UnixNano()),Or for better uniqueness:
+ 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) + } clientCertTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(time.Now().Unix()), + SerialNumber: serialNumber,
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_agent.sh
- templates/agent-template.yml
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: upgrade
- GitHub Check: e2e-with-istio
🔇 Additional comments (18)
test/setup/deploy_server.sh (1)
59-60: LGTM!The addition of
--kty RSAto the certificate generation commands ensures RSA-based key generation, which aligns with the broader certificate infrastructure changes across the PR.Also applies to: 68-68
Makefile (2)
111-113: LGTM!The new
broker_client_cert_refresh_durationvariable with a default of 5 minutes is well-documented and provides a sensible production default for certificate refresh intervals.
335-335: LGTM!The parameter is correctly propagated to the template generation command, ensuring the refresh duration is available to the agent templates.
test/setup/env_setup.sh (3)
144-147: LGTM!The MQTT certificate generation now uses RSA keys consistently, and the maestro-mqtt-ca secret is correctly created for use in certificate rotation tests. The past issue with the undefined variable has been properly addressed.
Also applies to: 155-156
174-175: LGTM!The gRPC certificate generation is updated to use RSA keys, and the secrets are properly provisioned in both the main namespace and agent namespace. The addition of the maestro-grpc-broker-ca secret enables gRPC certificate rotation testing.
Also applies to: 183-183, 187-189
88-93: Consider documenting why aggressive sync settings are necessary for this test environment.The
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"settings are intentionally configured for the certificate rotation e2e test, which modifies Secrets to verify proper certificate reload behavior. While appropriate for this specific test purpose, add a comment inenv_setup.shexplaining that these aggressive settings support rapid Secret change detection. This helps future maintainers understand the rationale and ensures the settings remain aligned with test requirements.test/e2e/pkg/cert_rotation_test.go (8)
33-60: LGTM!The BeforeAll hook properly saves original certificates from both MQTT and gRPC secrets with appropriate error handling. The use of
errors.IsNotFoundcheck beforeExpectensures graceful handling when secrets don't exist.
62-102: LGTM!The AfterAll hook correctly restores original certificates and performs thorough cleanup. The 30-second sleep allows time for certificate reload and agent reconnection before cleanup, and the cleanup logic ensures test resources are removed.
104-136: LGTM!The initial connectivity verification test properly validates that the agent can communicate with the server using the current certificates before rotation is attempted. The use of Eventually with appropriate timeouts ensures flake-free assertions.
138-145: LGTM!The certificate rotation test now returns and asserts the
rotatedboolean, ensuring that rotation actually occurred rather than silently passing when CA secrets are missing. The 15-second sleep provides adequate time for the agent to detect and reload the new certificates.
147-182: Clarify the test's certificate rotation validation strategy.The test updates the deployment to 2 replicas 15 seconds after rotation and expects success within 30 seconds. This validates that the agent can process updates with the new certificate, but there's a timing consideration: the rotated certificate has a 30-second expiration, and we're verifying success before expiration.
Could you confirm whether:
- The agent's
CERT_CALLBACK_REFRESH_DURATION(default 5m from Makefile) means it polls/reloads every 5 minutes, OR- It reloads certificates immediately upon detecting secret changes (via the kubelet's Watch strategy)?
If it's polling-based with a 5m interval, the test might pass due to the original (long-lived) certificate still being valid rather than proving the new 30s certificate was actually loaded and used.
Based on learnings from past review discussions, I understand the certificate refresh mechanism ensures validity, but clarifying the reload trigger would strengthen confidence in what this test proves.
186-283: LGTM!The
rotateCertificatesfunction now has robust error handling:
- Returns a boolean indicating whether any rotation occurred
- Guards against nil
Secret.Datamaps before assignment (lines 225-227, 270-272)- Properly distinguishes between NotFound (skip rotation) and other errors (return error)
- Handles both MQTT and gRPC rotation paths independently
The fixes from previous review feedback have been properly addressed.
285-292: LGTM!The
parseCertificatehelper correctly decodes and parses PEM-encoded X.509 certificates with appropriate error handling.
294-319: LGTM!The
parsePrivateKeyhelper properly supports both PKCS1 and PKCS8 RSA private key formats, which is essential for compatibility with various certificate generation tools. The fallback logic and type assertion are correct.templates/agent-tls-template.yml (4)
66-70: LGTM!The parameter renames from
MQTT_CLENT_CERTandMQTT_CLENT_KEYtoMQTT_CLIENT_CERTandMQTT_CLIENT_KEYfix the typo and improve clarity.
72-74: LGTM!The new
BROKER_CLIENT_CERT_REFRESH_DURATIONparameter with a default value of "5m" is well-documented and provides a sensible default for certificate refresh behavior.
310-311: LGTM!The
CERT_CALLBACK_REFRESH_DURATIONenvironment variable correctly injects the broker client certificate refresh duration into the Maestro agent container, enabling configurable certificate reload behavior.
362-363: LGTM!The secret configuration now correctly references the renamed
MQTT_CLIENT_CERTandMQTT_CLIENT_KEYparameters, maintaining consistency with the parameter name fixes.
508d955 to
161b652
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/setup/deploy_agent.sh (1)
44-50: LGTM! Environment variables properly configured for testing.The exported variables align with the updated template parameters and the 5-second refresh duration is appropriate for accelerating certificate rotation in e2e tests.
Minor: The comment on line 49 says "crank the client certificate refresh interval" which typically means "increase," but the value is actually decreased from the production default (5m → 5s) to speed up testing. Consider: "# set a short client certificate refresh interval for cert rotation test"
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setup/deploy_server.sh
- Makefile
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: upgrade
🔇 Additional comments (15)
templates/agent-tls-template.yml (4)
66-70: LGTM! Typo correction improves parameter naming.The parameter names have been corrected from
MQTT_CLENT_CERTandMQTT_CLENT_KEYtoMQTT_CLIENT_CERTandMQTT_CLIENT_KEY, fixing the spelling error.
72-74: LGTM! New refresh duration parameter is well-defined.The new
BROKER_CLIENT_CERT_REFRESH_DURATIONparameter enables configurable certificate refresh intervals with a sensible default of 5 minutes.
310-311: LGTM! Environment variable properly wired to template parameter.The
CERT_CALLBACK_REFRESH_DURATIONenvironment variable correctly sources its value from theBROKER_CLIENT_CERT_REFRESH_DURATIONparameter, enabling the agent to use the configured refresh interval.
362-363: LGTM! Secret references updated consistently.The
clientCertFileandclientKeyFilereferences have been updated to use the corrected parameter namesMQTT_CLIENT_CERTandMQTT_CLIENT_KEY.templates/agent-template.yml (1)
66-70: LGTM! Changes consistent with agent-tls-template.yml.The parameter renames (MQTT_CLENT_CERT/KEY → MQTT_CLIENT_CERT/KEY), new BROKER_CLIENT_CERT_REFRESH_DURATION parameter, CERT_CALLBACK_REFRESH_DURATION environment variable, and updated secret references are all consistent with the changes in agent-tls-template.yml. Good template synchronization.
Also applies to: 72-74, 310-311, 346-347
test/setup/env_setup.sh (3)
88-93: LGTM! Kubelet configuration optimized for certificate rotation testing.The
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"settings enable rapid detection of secret updates, which is essential for validating certificate rotation in the e2e test without long waits.
144-147: LGTM! RSA-based MQTT certificates and CA secret properly configured.The explicit
--kty RSAflag ensures consistent key type across all MQTT certificates, and the newmaestro-mqtt-casecret in the agent namespace provides the rotation test with the necessary CA keys to sign new client certificates.Also applies to: 155-156
174-175: LGTM! RSA-based gRPC certificates and per-namespace secrets properly configured.The gRPC certificate generation uses consistent RSA key types, and the separate
maestro-grpc-broker-certsecrets in both namespaces along withmaestro-grpc-broker-cain the agent namespace enable proper isolation and support for the gRPC rotation test.Also applies to: 183-183, 187-189
test/e2e/pkg/cert_rotation_test.go (7)
33-60: LGTM! BeforeAll properly saves original certificates with correct error handling.The secret fetching now correctly handles both successful retrieval and NotFound cases, avoiding nil dereferences. The deep copy of secret data ensures the originals can be safely restored.
62-102: LGTM! AfterAll properly restores state and cleans up test resources.The restoration logic correctly checks for saved certificates before restoring, includes an appropriate wait for certificate reload, and ensures proper cleanup of test resources with timeouts.
104-136: LGTM! Baseline connectivity test properly validates pre-rotation agent functionality.The test establishes that the agent can successfully create deployments and report status back before certificate rotation, providing a solid baseline for comparison.
138-145: LGTM! Certificate rotation test properly validates that rotation occurred.The test correctly verifies that
rotateCertificatesreturnedtrue, ensuring at least one CA secret was found and rotation actually happened. The 15-second wait allows time for the agent to detect and reload the updated certificates.
147-182: LGTM! Post-rotation verification comprehensively validates agent functionality.The test properly validates that the agent remains healthy and can successfully process updates after certificate rotation, confirming the rotation mechanism works end-to-end.
187-283: LGTM! Certificate rotation logic is robust and handles edge cases properly.The
rotateCertificatesfunction correctly:
- Returns a boolean flag to prevent false-positive tests when no CA secrets exist
- Validates input parameters
- Handles both MQTT and gRPC rotation independently
- Guards against nil
Secret.Data- Wraps errors with context for easier debugging
285-354: LGTM! Helper functions are well-implemented with appropriate error handling.The certificate parsing and signing helpers properly handle:
- PEM decoding failures
- Both PKCS1 and PKCS8 private key formats
- RSA key type validation
- Appropriate KeyUsage and ExtKeyUsage for client certificates
- 2048-bit RSA keys (adequate for test environments)
The hardcoded
CommonName: "test-client"is acceptable for test code.
161b652 to
27a7f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/setup/env_setup.sh (1)
183-189: Consider adding delete-before-create for idempotency.The gRPC broker CA secret creation on line 189 doesn't have a preceding delete command, unlike the other secrets. This could cause the script to fail on re-runs if the secret already exists.
kubectl delete secret maestro-grpc-broker-cert -n "$namespace" --ignore-not-found kubectl delete secret maestro-grpc-broker-cert -n "$agent_namespace" --ignore-not-found + kubectl delete secret maestro-grpc-broker-ca -n "$agent_namespace" --ignore-not-found kubectl create secret generic maestro-grpc-broker-cert -n "$namespace" --from-file=ca.crt=${grpc_broker_cert_dir}/ca.crt --from-file=server.crt=${grpc_broker_cert_dir}/server.crt --from-file=server.key=${grpc_broker_cert_dir}/server.key kubectl create secret generic maestro-grpc-broker-cert -n "$agent_namespace" --from-file=ca.crt=${grpc_broker_cert_dir}/ca.crt --from-file=client.crt=${grpc_broker_cert_dir}/client.crt --from-file=client.key=${grpc_broker_cert_dir}/client.key kubectl create secret generic maestro-grpc-broker-ca -n "$agent_namespace" --from-file=ca.crt=${grpc_broker_cert_dir}/ca.crt --from-file=ca.key=${grpc_broker_cert_dir}/ca.keytest/e2e/pkg/cert_rotation_test.go (2)
80-82: Hardcoded sleep may cause flakiness.The 30-second sleep assumes the agent's cert refresh will complete within this window. Consider polling for a signal that the agent has reloaded (e.g., checking pod restart count or a readiness probe) rather than using a fixed duration, or at minimum, add a comment explaining the timing relationship with
BROKER_CLIENT_CERT_REFRESH_DURATION.- // wait for certificate reload and agent reconnection - time.Sleep(30 * time.Second) + // Wait for certificate reload and agent reconnection. + // The agent's CERT_CALLBACK_REFRESH_DURATION defaults to 5m in production, + // but is configured to a shorter interval for e2e tests. + // 30s should be sufficient for the agent to detect and reload the restored certs. + time.Sleep(30 * time.Second)
331-341: Consider using cryptographically random serial number.Using
time.Now().Unix()for the serial number could theoretically produce collisions if certificates are generated within the same second. While unlikely in tests, usingcrypto/randwould be more robust.+ 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/setup/deploy_agent.sh
- test/setup/deploy_server.sh
- Makefile
- templates/agent-template.yml
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-grpc-broker
🔇 Additional comments (10)
test/setup/env_setup.sh (3)
88-93: Good addition for certificate rotation testing.The kubelet configuration with
syncFrequency: "1s"andconfigMapAndSecretChangeDetectionStrategy: "Watch"ensures rapid secret change detection, which is essential for validating cert rotation behavior in e2e tests.
144-147: RSA key type specification is appropriate.Explicit
--kty RSAensures consistency across all certificate generation and aligns with theparsePrivateKeyhelper in the e2e test that expects RSA keys.
155-156: LGTM - Previous issue addressed.The variable name is now correctly
${mqtt_cert_dir}(matching the declaration on line 140), and the namespace is properly quoted.templates/agent-tls-template.yml (3)
66-75: Good fixes and additions.The parameter name corrections (
MQTT_CLENT_*→MQTT_CLIENT_*) and the newBROKER_CLIENT_CERT_REFRESH_DURATIONparameter with a sensible default enable configurable certificate refresh behavior.
310-311: LGTM - Environment variable properly wired.The
CERT_CALLBACK_REFRESH_DURATIONenvironment variable correctly references the template parameter, allowing the agent to pick up the configured refresh duration.
362-363: Secret references updated correctly.The MQTT config now uses the corrected parameter names
${MQTT_CLIENT_CERT}and${MQTT_CLIENT_KEY}.test/e2e/pkg/cert_rotation_test.go (4)
33-60: Error handling improvements look good.The secret fetching logic now correctly handles three cases: success (save certs), NotFound (skip), and other errors (fail test). This addresses the previous review feedback.
138-145: Test timing appears reasonable given author clarification.The 60s certificate duration with a 10s sleep before verification provides sufficient margin for the agent to reload the new certificate while still being short enough for meaningful testing.
186-191: Good defensive validation.The duration validation and the
rotatedflag pattern ensure the test fails explicitly if no rotation occurred, addressing the previous false-positive concern.
285-319: Helper functions are well-implemented.The
parseCertificateandparsePrivateKeyfunctions handle PEM decoding correctly, andparsePrivateKeyappropriately supports both PKCS1 and PKCS8 formats with clear error messages.
b815644 to
673e60d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/agent-tls-template.yml (1)
321-340: Fix typo in gRPC certificate mount path and configuration references.Line 327 has a typo:
mountPath: /secretes/grpc-broker-certsshould be/secrets/grpc-broker-certsto match the naming convention used for other volume mounts (lines 323, 325). This also requires updating the corresponding configuration file references at lines 348–350 (caFile,clientCertFile,clientKeyFile) from/secretes/grpc-broker-certs/to/secrets/grpc-broker-certs/to maintain consistency.
♻️ Duplicate comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
138-145: Rotation verification is still sleep-based; risk of false-positive / flake.
The test rotates to a 60s cert and only sleeps 10s (Line 138-145), then asserts an update “before expiration” (Line 171-181). This can still pass without proving the agent actually reloaded the rotated cert (and it’s also sensitive to the configured refresh duration).#!/bin/bash # Confirm e2e sets refresh duration short enough, and consider asserting on an observable "reload/reconnect" signal. rg -n "CERT_CALLBACK_REFRESH_DURATION|BROKER_CLIENT_CERT_REFRESH_DURATION|refresh" test -SAlso applies to: 147-182
🧹 Nitpick comments (3)
test/setup/env_setup.sh (2)
138-167: Make cert/secret provisioning rerunnable; don’t key it off “directory exists”.
Both MQTT and gRPC sections gate cert+secret creation onif [ ! -d "$*_cert_dir" ](Line 141-157, 171-190). If the directory exists but a secret was deleted (or only some files exist), reruns will skip recreating required secrets (maestro-mqtt-ca,maestro-grpc-broker-ca, etc.), making e2e setup flaky.Suggested change: gate on secrets (or specific files) instead, and always
kubectl delete secret ... --ignore-not-foundbefore create for the CA secrets too. Also consider temporarily disablingset -xwhile creating secrets that includeca.keyto avoid logging key-related commands.- if [ ! -d "$mqtt_cert_dir" ]; then + if [ ! -f "${mqtt_cert_dir}/ca.crt" ] || [ ! -f "${mqtt_cert_dir}/ca.key" ]; then # create certs mkdir -p "$mqtt_cert_dir" step certificate create "maestro-mqtt-ca" ${mqtt_cert_dir}/ca.crt ${mqtt_cert_dir}/ca.key --kty RSA --profile root-ca --no-password --insecure @@ - kubectl create secret generic maestro-mqtt-certs -n "${namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=server.crt=${mqtt_cert_dir}/server.crt --from-file=server.key=${mqtt_cert_dir}/server.key + kubectl delete secret maestro-mqtt-ca -n "${agent_namespace}" --ignore-not-found + set +x + kubectl create secret generic maestro-mqtt-certs -n "${namespace}" \ + --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" \ + --from-file=server.crt="${mqtt_cert_dir}/server.crt" \ + --from-file=server.key="${mqtt_cert_dir}/server.key" @@ - kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" --from-file=ca.crt=${mqtt_cert_dir}/ca.crt --from-file=ca.key=${mqtt_cert_dir}/ca.key + kubectl create secret generic maestro-mqtt-ca -n "${agent_namespace}" \ + --from-file=ca.crt="${mqtt_cert_dir}/ca.crt" \ + --from-file=ca.key="${mqtt_cert_dir}/ca.key" + set -x fiAlso applies to: 168-191
76-95: Consider isolating kubelet patches to e2e-only runs.
ThekubeadmConfigPatchessets validKubeletConfigurationfields (configMapAndSecretChangeDetectionStrategy: "Watch",syncFrequency: "1s"). While these are supported in kubelet.config.k8s.io/v1beta1 and safe for rotation tests, restricting this aggressive configuration to e2e test runs would prevent unintended side effects on development or standard cluster setups.test/e2e/pkg/cert_rotation_test.go (1)
321-353: Harden client cert issuance: random serial + skew-tolerant NotBefore.
SerialNumber: big.NewInt(time.Now().Unix())(Line 333) can collide;NotBefore: now(Line 337) can flake on minor clock skew.@@ func signClientCertificate(caCert *x509.Certificate, caKey *rsa.PrivateKey, duration time.Duration) (certPEM, keyPEM []byte, err error) { @@ - now := time.Now() + now := time.Now() + serialLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serial, err := rand.Int(rand.Reader, serialLimit) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate serial number: %w", err) + } @@ clientCertTemplate := &x509.Certificate{ - SerialNumber: big.NewInt(time.Now().Unix()), + SerialNumber: serial, Subject: pkix.Name{ CommonName: "test-client", }, - NotBefore: now.UTC(), + NotBefore: now.Add(-1 * time.Minute).UTC(), NotAfter: now.Add(duration).UTC(), KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- README.md
- templates/mqtt-tls-template.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- test/setup/deploy_server.sh
- test/setup/deploy_agent.sh
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (2)
templates/agent-template.yml (1)
346-347: Parameter rename looks consistent in mqtt config.yaml.
clientCertFile/clientKeyFileupdated to${MQTT_CLIENT_CERT}/${MQTT_CLIENT_KEY}(Line 346-347).templates/agent-tls-template.yml (1)
362-363: MQTT cert/key parameter rename in TLS template config looks correct.
673e60d to
ad65453
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- templates/mqtt-tls-template.yml
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- test/setup/deploy_server.sh
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
🔇 Additional comments (19)
test/setup/deploy_agent.sh (1)
44-50: LGTM! E2E certificate rotation configuration is properly set.The 5s refresh duration is appropriate for rapid certificate rotation testing, and all MQTT-related environment variables are correctly configured to align with the template parameters.
test/setup/env_setup.sh (4)
88-93: LGTM! Kubelet configuration optimized for certificate rotation testing.The 1s
syncFrequencyandWatchdetection strategy ensure the kubelet quickly detects secret updates, which is essential for testing certificate rotation with the 5s refresh interval.
144-147: LGTM! RSA key type ensures compatibility with certificate rotation tests.The explicit
--kty RSAspecification aligns with the test code's RSA-specific certificate parsing and signing operations.
155-156: LGTM! CA secret creation is properly configured.The
maestro-mqtt-casecret with both CA certificate and key is correctly created in the agent namespace, enabling the certificate rotation test to generate new client certificates.
187-189: LGTM! Per-namespace gRPC secret provisioning supports certificate rotation.Creating the gRPC broker certificates in both namespaces and the CA secret in the agent namespace enables the rotation test to generate new client certificates while maintaining proper isolation.
templates/agent-template.yml (3)
66-70: LGTM! Parameter naming corrected.The typo fixes (MQTT_CLENT_* → MQTT_CLIENT_*) improve clarity and consistency.
72-74: LGTM! Certificate refresh duration parameter properly configured.The 5m default is appropriate for production deployments, while e2e tests override it to 5s for rapid rotation testing.
346-347: LGTM! Secret configuration references updated.The MQTT secret configuration now correctly references the renamed parameters.
templates/agent-tls-template.yml (3)
66-74: LGTM! Parameter declarations align with agent-template.yml.The typo fixes and
BROKER_CLIENT_CERT_REFRESH_DURATIONparameter addition mirror the changes inagent-template.yml, maintaining consistency across templates.
310-311: LGTM! Environment variable injection is consistent.The
CERT_CALLBACK_REFRESH_DURATIONenvironment variable properly propagates the refresh duration to the agent container, consistent with the non-TLS template.
362-363: LGTM! MQTT secret configuration updated.The corrected parameter names are properly applied in the MQTT secret configuration.
test/e2e/pkg/cert_rotation_test.go (8)
37-60: LGTM! Original certificate backup handles errors properly.The BeforeAll logic correctly saves original certificates when they exist, gracefully handles missing secrets, and fails the test on unexpected errors.
62-102: LGTM! Cleanup and restoration logic is thorough.The AfterAll properly restores original certificates, waits for agent reconnection, and cleans up test resources with appropriate timeouts.
104-136: LGTM! Baseline connectivity test establishes agent health before rotation.The test properly verifies the agent can receive and apply resources before certificate rotation, providing a clear baseline.
138-145: LGTM! Certificate rotation test properly validates that rotation occurred.The test generates short-lived (60s) certificates and verifies that
rotateCertificatesactually performed rotation (by checking the returned bool), addressing potential false positives.
147-182: LGTM! Post-rotation verification confirms agent operates with new certificates.The test verifies the agent pod is healthy and can successfully process resource updates after certificate rotation, demonstrating end-to-end certificate rotation functionality.
187-283: LGTM! Certificate rotation implementation is robust and well-structured.The function properly:
- Validates input parameters
- Handles both MQTT and gRPC certificate rotation independently
- Provides clear error messages with context
- Guards against nil secret data maps
- Returns a boolean indicating whether any rotation occurred, preventing false-positive test results
285-319: LGTM! Certificate and key parsing handles multiple formats gracefully.The parsing functions properly support both PKCS1 and PKCS8 RSA key formats, providing flexibility for different certificate generation tools while maintaining clear error messages.
321-354: LGTM! Client certificate generation is appropriate for testing.The function generates properly configured client certificates with:
- Secure random key generation (2048-bit RSA)
- Appropriate key usage flags for client authentication
- Configurable validity duration for testing scenarios
- Standard PEM encoding
ad65453 to
524ef4b
Compare
|
/assign @clyang82 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
138-182: Rotation test still doesn’t strongly prove the agent reloaded the rotated certificate.Even with the 60s cert and 5s refresh interval, the flow:
- rotate certificates,
- sleep 10s,
- then verify a deployment update succeeds
can still pass if the agent never actually reloaded the new client cert (e.g., it keeps using an existing connection or long‑lived prior cert). The assertions validate “agent communication still works shortly after we changed the secret,” but not that the rotation mechanism (reload based on the refreshed cert material) was exercised.
To more directly prove rotation, consider tightening one of these:
- After calling
rotateCertificates, explicitly verify that the agent’s client cert material changed (e.g., by comparing theclient.crtin the agent pod filesystem or in the mounted Secret against the pre‑rotation snapshot), and only then perform the deployment update; or- Introduce an observable signal from the agent (log line, metric, or status condition) on cert reload and make the test wait for that before asserting the update works.
This would reduce the chance of a false green where the rotation logic is never actually exercised.
🧹 Nitpick comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
186-354: Certificate rotation helper and crypto utilities are correct for test use; minor nit on serial numbers only.The
rotateCertificates,parseCertificate,parsePrivateKey, andsignClientCertificatehelpers correctly:
- Validate presence/content of CA secrets and client cert secrets,
- Support both PKCS1/PKCS8 RSA keys,
- Generate a client cert with appropriate usages and duration, and
- Safely write back to Secrets with proper error propagation and
Datamap initialization.For a test‑only helper this is perfectly adequate. If you ever re‑use this pattern outside tests, you might want to switch the serial number generation to a random big.Int (e.g.,
new(big.Int).Rand(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128))) instead oftime.Now().Unix()to avoid potential collisions, but that’s purely a nicety here.
📜 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 (9)
Makefile(2 hunks)README.md(0 hunks)templates/agent-template.yml(3 hunks)templates/agent-tls-template.yml(3 hunks)templates/mqtt-tls-template.yml(0 hunks)test/e2e/pkg/cert_rotation_test.go(1 hunks)test/setup/deploy_agent.sh(1 hunks)test/setup/deploy_server.sh(1 hunks)test/setup/env_setup.sh(3 hunks)
💤 Files with no reviewable changes (2)
- templates/mqtt-tls-template.yml
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/setup/env_setup.sh
- Makefile
- test/setup/deploy_server.sh
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-with-istio
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (4)
test/e2e/pkg/cert_rotation_test.go (1)
33-136: Setup/teardown and baseline connectivity checks look solid.The
BeforeAll/AfterAlllogic for saving/restoring MQTT and gRPC secrets, plus the work/deployment cleanup and the initial “agent works with current certificates” verification, are all structured correctly and handle errors defensively. I don’t see functional or flakiness red flags in this part.test/setup/deploy_agent.sh (1)
44-50: E2E override of broker client cert refresh duration is consistent and appropriate.The added MQTT env exports and the
broker_client_cert_refresh_duration=5soverride are aligned with the new template parameters and give you a tight refresh loop suitable for the 60s rotation test without impacting the template’s more conservative 5m default. No changes needed here.templates/agent-tls-template.yml (1)
66-75: Template parameter and env wiring for MQTT certs and refresh duration looks coherent.The introduction of
MQTT_CLIENT_CERT/MQTT_CLIENT_KEYandBROKER_CLIENT_CERT_REFRESH_DURATIONplus the injection ofCERT_CALLBACK_REFRESH_DURATIONinto the deployment, and the updated MQTT Secret references, all line up and keep the TLS template consistent with the non‑TLS variant and the test setup script. With the e2e script overridingBROKER_CLIENT_CERT_REFRESH_DURATIONto 5s, this should behave correctly in both production (5m default) and test scenarios.Also applies to: 310-311, 362-363
templates/agent-template.yml (1)
66-75: Non‑TLS agent template is now consistent with TLS variant for MQTT client certs and refresh duration.The renamed MQTT client cert/key parameters, the new
BROKER_CLIENT_CERT_REFRESH_DURATIONparameter, and theCERT_CALLBACK_REFRESH_DURATIONenv injection are wired correctly and match the TLS template and setup script conventions. This should make it straightforward to control broker client cert refresh behavior across both deployment modes.Also applies to: 310-311, 346-347
Signed-off-by: Morven Cao <lcao@redhat.com>
524ef4b to
aa955db
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/pkg/cert_rotation_test.go (2)
206-303: Rotation helper is correct but has duplicated MQTT/gRPC pathsFunctionally this looks good: duration validation, NotFound handling for CA secrets, strict checks for
ca.crt/ca.key, and arotatedflag that correctly reflects whether any client secret was updated.The MQTT and gRPC branches are nearly identical though (read CA secret → parse → sign → read client secret → update
client.crt/client.key→ mark rotated). If this grows further, consider extracting a small helper like:type caAndClient struct { caSecret, clientSecret string crtKey, keyKey string } func rotateClientCert(ctx context.Context, kube kubernetes.Interface, ns string, cfg caAndClient, duration time.Duration) (bool, error) { ... }and calling it twice with different secret names. That would reduce duplication and keep future changes (e.g., key names or error messages) in one place.
342-373: Serial number generation is simplistic; OK for tests but could be randomizedUsing
SerialNumber: big.NewInt(time.Now().Unix())is sufficient for this e2e test, but will generate the same serial if multiple certs are signed within the same second by the same CA. If you ever need closer alignment with typical X.509 practices (or reuse this helper beyond tests), consider using a random serial, for example:serialLimit := new(big.Int).Lsh(big.NewInt(1), 128) serial, err := rand.Int(rand.Reader, serialLimit) if err != nil { return nil, nil, fmt.Errorf("failed to generate serial: %w", err) } clientCertTemplate.SerialNumber = serialFor the current e2e scope, the existing approach is acceptable.
📜 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 (1)
test/e2e/pkg/cert_rotation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: e2e
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: upgrade
- GitHub Check: e2e-with-istio
🔇 Additional comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
33-60: Secret backup/restore and cleanup flow looks solidThe original MQTT/gRPC certs are deep‑copied (per‑key byte slices) and only restored when a snapshot exists, and cleanup ensures both the ManifestWork and the test Deployment are removed. The NotFound handling in
BeforeAllavoids nil derefs and spurious failures when cert secrets are absent.Also applies to: 103-146
54d6e3f to
aa41866
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/pkg/cert_rotation_test.go (1)
325-358: Optional: Consider using crypto/rand for certificate serial numbers.The current implementation uses
time.Now().Unix()for the serial number (line 337), which works fine for test certificates but could theoretically collide. Usingcrypto/randwould be more robust:- SerialNumber: big.NewInt(time.Now().Unix()), + serialNumber, _ := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + SerialNumber: serialNumber,However, given this is test code with short-lived, non-overlapping certificates, the current approach is acceptable.
📜 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 (1)
test/e2e/pkg/cert_rotation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: Red Hat Konflux / maestro-on-pull-request
🔇 Additional comments (6)
test/e2e/pkg/cert_rotation_test.go (6)
33-101: LGTM: BeforeAll setup is well-structured and robust.The certificate backup logic correctly handles all scenarios (secret exists, NotFound, or other errors). The rotation with short expiration (30s) followed by deployment restart and work creation provides a solid foundation for testing certificate reload behavior.
103-146: LGTM: AfterAll cleanup is thorough.The restoration of original certificates and proper verification of resource deletion ensures the test suite cleans up after itself without leaving side effects.
148-186: LGTM: Test case correctly validates certificate reload and connectivity.The test flow properly exercises the scenario: cert expires → rotate to long-lived cert → wait for reload → verify agent can process updates. The 10-second wait at line 158 assumes the agent will reload the certificate within this window; this appears sufficient based on the expected behavior where connection failures trigger immediate cert reloads.
190-287: LGTM: Certificate rotation logic is comprehensive and well-implemented.The function properly handles both MQTT and gRPC broker certificates with thorough error checking at each step. The boolean return value ensures callers can verify that rotation actually occurred, and the nil Secret.Data guards prevent panics.
289-323: LGTM: Certificate parsing helpers are robust.Both
parseCertificateandparsePrivateKeyhandle their respective formats correctly, withparsePrivateKeysupporting both PKCS1 and PKCS8 formats for better compatibility.
360-410: LGTM: Deployment restart with rollout verification is well-implemented.The function correctly triggers a deployment restart via annotation, guards against nil
Spec.Replicas, and waits for complete rollout before returning. The 5-second buffer at line 407 is appropriate for allowing connections to stabilize in an e2e test environment.
aa41866 to
2efe91e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e/pkg/cert_rotation_test.go (2)
148-186: Consider factoring rotation durations into shared constants or config to reduce drift risk.The test currently hard-codes
30*time.Secondfor expiry wait,1*time.Hourfor the long-lived cert, and10*time.Secondfor reload. If the broker/client refresh durations or cert TTLs change in templates or setup scripts, it’s easy for this test to fall out of sync. Consider pulling these into named constants (or deriving them from the same config/env used in setup) so the timing assumptions stay aligned across the suite.
298-323: RSA-only assumption in key parsing/signing is fine for this test but worth keeping explicit.
parsePrivateKeyonly returns*rsa.PrivateKey(PKCS1/PKCS8) andsignClientCertificateexpects an RSA CA key. That’s perfectly fine given the test harness now generates RSA CAs, but it does mean this helper cannot be reused as-is against clusters with ECDSA (or other) CA keys. If you ever need broader coverage, you could generalize tocrypto.Signerand handle additional key types; for the current CI scenario, the explicit RSA requirement is acceptable.Also applies to: 325-358
📜 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 (1)
test/e2e/pkg/cert_rotation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e
- GitHub Check: upgrade
- GitHub Check: e2e-grpc-broker
- GitHub Check: e2e-with-istio
🔇 Additional comments (2)
test/e2e/pkg/cert_rotation_test.go (2)
190-287:rotateCertificatesimplementation and error handling look solid.The rotation helper cleanly validates inputs (
duration > 0), distinguishes NotFound from real API errors for CA Secrets, validatesca.crt/ca.keypresence, guards against nilDataon target Secrets, and uses therotatedflag to avoid treating a total no-op as success. This matches the intended test semantics and should fail loudly on any misconfiguration rather than silently passing.
360-410:restartDeploymenthelper is robust and defensive.The rollout helper correctly annotates the Pod template, waits for updated/ready/available replicas to match
.spec.replicas, guards against a nil.spec.replicas, and adds a small post-rollout sleep to let connections stabilize. The error messages are clear and should make debugging failed rollouts straightforward.
2efe91e to
1b4ab2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/e2e/pkg/cert_rotation_test.go (4)
26-101: Test flow is solid; consider tightening time-based waits to reduce flakiness.The overall setup (initial 30s cert, work creation, then expiry + 1h rotation and scale‑up) reads well and clearly exercises rotation. The main risk is the fixed
time.Sleep(30 * time.Second)andtime.Sleep(10 * time.Second)waits: if the agent’s cert‑reload or broker reconnect intervals change, these fixed sleeps could become either unnecessarily slow or flaky.Consider:
- Basing the waits on the configured refresh interval (e.g., a shared test constant) rather than literals, and/or
- Replacing the post‑rotation
Sleep(10 * time.Second)with anEventuallycondition that observes some signal of “agent has processed the new cert” (e.g., a status/condition, metric, or successful lightweight broker operation) before sending the scale‑up.This keeps the scenario while making the test more robust to timing changes.
Also applies to: 148-186
190-287: ClarifyrotateCertificatesbehavior when neither CA secret is present.
rotateCertificatescurrently returns(false, nil)when bothmaestro-mqtt-caandmaestro-grpc-broker-caare NotFound, and the call sites explicitly assertrotated == true, so this test suite will fail correctly if rotation can’t run.If this helper might ever be reused elsewhere, that
(false, nil)contract could be misread as “success but nothing changed.” You may want to either:
- Treat “no CA secrets found” as an explicit error (e.g.,
return false, fmt.Errorf("no CA secrets found for MQTT or gRPC")whenrotatedis still false at the end), or- Document in the function comment that
rotated == false && err == nilspecifically means “no applicable CA secrets were found, nothing was rotated,” so future callers don’t accidentally ignore a misconfiguration.For this PR’s current usage it’s fine; this is about future‑proofing the helper.
289-357: Cert/key helpers look good; serial generation is fine for tests but avoid in production.
parseCertificate,parsePrivateKey(PKCS1 + PKCS8), andsignClientCertificateare all well‑structured for the e2e scenario. One nuance:signClientCertificateusesbig.NewInt(time.Now().Unix())forSerialNumber, which is predictable and can collide if reused heavily.That’s perfectly acceptable for test‑only code, but if this helper is ever promoted into non‑test code, consider switching to a crypto‑random serial (e.g., using
rand.Intwith a suitably large upper bound) to align with X.509 best practices.
360-410:restartDeploymentworks well; embedding Gomega inside the helper limits reuse.The rollout logic and readiness checks are correct and nicely harden against nil
.spec.replicas. The only structural tradeoff is that the helper directly usesEventually(...).ShouldNot(HaveOccurred()), which ties it tightly to Ginkgo/Gomega and means it can’t be reused from non‑Ginkgo contexts without panicking the suite.Since this lives under
test/e2e, that’s acceptable; if you ever want to reuse it more widely, consider refactoring sorestartDeploymentjust returns anerror(doing its own poll internally) and let callers assert via Gomega in the test layer.
📜 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 (1)
test/e2e/pkg/cert_rotation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-grpc-broker
- GitHub Check: upgrade
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e
- GitHub Check: e2e-with-istio
Signed-off-by: Morven Cao <lcao@redhat.com>
1b4ab2d to
d4266fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/e2e/pkg/cert_rotation_test.go (3)
148-186: Time‑based rotation test is clear; consider aligning waits with refresh intervalThe main
Itblock clearly models: let the short cert age, rotate to a long‑lived cert without restarting, wait for reload, then assert a post‑rotation update succeeds. That matches the stated goal of validating reload behavior.One thing to double‑check is that
time.Sleep(10 * time.Second)is comfortably larger thanBROKER_CLIENT_CERT_REFRESH_DURATIONin the test environment (including any jitter), otherwise this could be flaky if the agent hasn’t yet hit its refresh window when the work patch is applied.
190-287: Rotation helper is robust; only minor duplication and assumptions
rotateCertificateshas good validation, clear error messages, and properly distinguishes “no CA secret” from real failures. The boolean return to signal actual rotation nicely guards against no‑op passes.The only nits are:
- MQTT and gRPC rotation blocks are almost identical; extracting a small helper (e.g.
rotateClientCert(caSecretName, clientSecretName, duration)) would reduce duplication.- The function assumes the client cert secrets (
maestro-agent-certs,maestro-grpc-broker-cert) always exist; if that ever changes, you may want to special‑case NotFound to give a clearer error.Both are nice‑to‑have improvements, not blockers.
325-358: Consider using a cryptographically random serial number for client certs
signClientCertificatecurrently usesbig.NewInt(time.Now().Unix())for the serial, which can collide if multiple certs are issued within the same second. Even in tests it’s trivial to avoid this by using a random big.Int instead of a timestamp‑based value (e.g., generate a 128‑bit random serial withrand.Reader).Not a blocker for an e2e test, but switching to a random serial is more in line with X.509 best practices and avoids any potential surprises if the helper is reused elsewhere.
📜 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 (1)
test/e2e/pkg/cert_rotation_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/pkg/cert_rotation_test.go (3)
test/performance/pkg/util/util.go (1)
Eventually(30-49)test/e2e/pkg/sourceclient_test.go (1)
AssertWorkNotFound(675-686)pkg/client/cloudevents/grpcsource/util.go (1)
ToWorkPatch(182-226)
⏰ 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: Red Hat Konflux / maestro-on-pull-request
- GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
- GitHub Check: e2e-with-istio
- GitHub Check: e2e
- GitHub Check: e2e-broadcast-subscription
- GitHub Check: e2e-grpc-broker
- GitHub Check: upgrade
🔇 Additional comments (4)
test/e2e/pkg/cert_rotation_test.go (4)
26-101: Overall test setup looks solid and focused on the right behaviorThe
BeforeAllflow (backing up secrets, doing an initial short‑lived rotation, restarting the agent, creating work, and asserting deployment/work status) is coherent and exercises the intended path without obvious correctness issues. Given the guarantees you rely on for the secrets and replicas, this looks good as‑is.
103-146: Cleanup logic is consistent with the setup assumptionsThe
AfterAllrestoration of MQTT and gRPC broker certs, followed by an agent restart and resource cleanup, matches the assumptions from the setup (secrets present with data, non‑nil maps). The flow is straightforward and should reliably return the cluster to its original state under those assumptions.
289-323: Certificate and key parsing helpers are well‑scoped
parseCertificateandparsePrivateKeycleanly handle PEM decoding, PKCS1/PKCS8 formats, and non‑RSA keys with precise errors. These helpers are small, focused, and make the rotation logic easier to follow.
360-410:restartDeploymenthelper is defensive and matches k8s rollout semanticsThe restart helper is nicely hardened: it ensures the annotation map is non‑nil, checks
.spec.replicasbefore dereferencing, and validatesUpdated/Ready/AvailableReplicasagainst the desired count viaEventuallybefore returning. This should give reliable rollouts in test environments while keeping the call sites simple.
2 participants
ref: https://issues.redhat.com/browse/ACM-27141