Repository navigation
fix: address shubham's review feedback on DPT CACertRef handling - #2
Conversation
- Resolve the CA once per reconcile (resolveCAData) and pass the bytes into initializeProvider/initializeAWSProvider instead of re-reading CACertRef, so one reconcile uses one CA value and does one Secret lookup. - Skip CA resolution when skipTLSVerify is true, so a missing CACertRef Secret no longer fails a DPT that disables verification. - Resolve the CA only for AWS-compatible providers; GCP and Azure do not receive caCertData, so resolving it only added an unused Secret dependency. - E2E helper: fail when status.uploadTest.success is false (including ErrorMessage), since the reconciler marks the DPT Complete even when the upload failed and phase alone made the CACertRef test a false positive. - Tests: retrieveCAData case with both CACertRef and inline CACert asserting the Secret value wins; TestResolveCAData covering aws, skipTLSVerify with a missing Secret, aws with a missing Secret, gcp/azure, and a nil spec. Co-authored-by: Hermes Agent <noreply@hermes-agent> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused changes match the controller’s TLS and status handling, with relevant unit coverage and no unresolved blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Refines certificate-authority handling in OADP’s DataProtectionTest controller and strengthens end-to-end validation.
Changes:
- Resolves CA data once per reconcile, skipping lookup for non-AWS providers or disabled TLS verification.
- Makes the end-to-end helper reject failed uploads even when the phase is
Complete. - Adds tests for Secret precedence and CA-resolution conditions.
| File | Description |
|---|---|
| tests/e2e/lib/dpt.go | Checks upload success and reports failure details. |
| internal/controller/dataprotectiontest_controller.go | Resolves CA data conditionally and reuses it during provider initialization. |
| internal/controller/dataprotectiontest_controller_test.go | Updates initialization calls and tests CA-selection behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if r.dpt != nil && r.dpt.Spec.SkipTLSVerify { | ||
| return nil, nil | ||
| } | ||
| if backupLocationSpec == nil || !strings.EqualFold(backupLocationSpec.Provider, AWSProvider) { |
There was a problem hiding this comment.
The Azure/GCP plugins don't respect providing custom CAs anyways.
I don't see how this helps, anyone doing so is configuring the object incorrectly. But I don't see this hurting to force ignoring for anything other than aws.
| if dpt.Status.Phase != "Complete" { | ||
| return fmt.Errorf("DataProtectionTest %s reached phase %q (error: %s)", dpt.Name, dpt.Status.Phase, dpt.Status.ErrorMessage) | ||
| } | ||
| // The reconciler marks the DPT Complete even when the upload itself failed |
There was a problem hiding this comment.
If there is ever a DPTv2 I would ask this to change.
One expects when the phase is "Completed" all parts were successful and otherwise either PartialFailed or PartialComplete or PartialSuccess or similar. With a condition array added to explain.
| Client: fakeClient, | ||
| Log: logr.Discard(), | ||
| NamespacedName: types.NamespacedName{Namespace: namespace, Name: "test-obj"}, | ||
| Context: context.Background(), |
There was a problem hiding this comment.
Should be t.Context(). I'll submit a commit to fix this.
| }, | ||
| } | ||
|
|
||
| caData, err := reconciler.resolveCAData(context.Background(), tt.bsl) |
Addresses @shubham-pampattiwar's review feedback on openshift#2443 (5 inline comments plus the follow-up comment on
retrieveCAData).initializeProviderresolveCAData) and passed intoinitializeProviderandinitializeAWSProvider. One reconcile now uses one CA value and does one Secret lookup.skipTLSVerify=truefails on a missingCACertRefSecretresolveCADatareturns nil without touching the Secret whenskipTLSVerifyis set. Covered by a new test.CACertRefresolved for GCP/Azure, which never receivecaCertDataCreateDPTAndAssertCompletenow also returns an error whenstatus.uploadTest.successis false, includingErrorMessage.retrieveCADatacase with bothCACertRefand inlineCACertset, asserting the Secret value is returned.CA selection stays in
retrieveCADataandbuildTLSConfigonly consumes the resolved bytes, as requested in the follow-up comment.Validation
go build ./...,go vet,golangci-lint run ./internal/controller/... ./tests/e2e/lib/...-> 0 issues.TestRetrieveCAData(including the new precedence case) and the newTestResolveCAData(aws, skipTLSVerify with a missing Secret, aws with a missing Secret, gcp, azure, nil spec).TestAPIs) needsbin/k8s/.../etcd, which is not installed here. It fails identically on the unmodified base, so it is an environment limitation, not a regression.CACertRefe2e test need a cluster and were not run here.Not addressed: the comment asking for a DPT test that makes an HTTPS request using a referenced CA Secret is the existing e2e (
cacert_suite_test.go), which this PR's stricter helper now makes meaningful.