CM-937: Automate e2e tests for Azure DNS01 Service Principal auth#374
CM-937: Automate e2e tests for Azure DNS01 Service Principal auth#374lunarwhite wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughAdds Azure DNS-01 support to the ACME e2e tests: helpers to read Azure Service Principal credentials from cluster secrets, parse Azure DNS zone identifiers, copy secrets into test namespaces, and provision ambient credentials via a CredentialsRequest for CCO-driven scenarios; adds explicit and ambient Azure test contexts. Changes
Sequence Diagram(s)sequenceDiagram
actor Test as E2E Test Suite
participant K8s as Kubernetes API
participant CCO as Cloud Credential Operator
participant Azure as Azure API
participant AzureDNS as Azure DNS Solver
Test->>K8s: Create CredentialsRequest (cert-manager-azure-dns)
K8s->>CCO: Notify CCO of CredentialsRequest
CCO->>Azure: Request DNS Zone Contributor credentials
Azure-->>CCO: Return credentials
CCO->>K8s: Create Secret (`azure-credentials`) in `cert-manager` namespace
Test->>K8s: Read Azure creds from `kube-system` Secret
Test->>K8s: Copy Secret to test namespace
Test->>K8s: Create ACME Issuer with AzureDNS solver referencing secret
K8s->>AzureDNS: Provide credentials to solver
AzureDNS->>Azure: Authenticate and update DNS records for DNS-01 challenge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lunarwhite The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![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 (1)
test/e2e/issuer_acme_dns01_test.go (1)
465-470: Harden Azure resource ID parsing to avoid silent mis-parsing.Line 465–470 assumes fixed indexes after
strings.Split. If the ID shape changes slightly, values can shift without validating segment names.♻️ Proposed refactor
- parts := strings.Split(zoneID, "/") - // parts: [0]="" [1]="subscriptions" [2]=<sub> [3]="resourceGroups" [4]=<rg> [5]="providers" [6]="Microsoft.Network" [7]="dnszones" [8]=<zone> - Expect(len(parts)).To(BeNumerically(">=", 9), "unexpected Azure resource ID format: %s", zoneID) - subscriptionID = parts[2] - resourceGroupName = parts[4] - hostedZoneName = parts[8] + parts := strings.Split(strings.Trim(zoneID, "/"), "/") + // expected: + // [0]="subscriptions" [1]=<sub> [2]="resourceGroups" [3]=<rg> [4]="providers" [5]="Microsoft.Network" [6]="dnszones" [7]=<zone> + Expect(len(parts)).To(BeNumerically(">=", 8), "unexpected Azure resource ID format: %s", zoneID) + Expect(parts[0]).To(Equal("subscriptions"), "unexpected Azure resource ID format: %s", zoneID) + Expect(parts[2]).To(Equal("resourceGroups"), "unexpected Azure resource ID format: %s", zoneID) + Expect(strings.EqualFold(parts[6], "dnszones")).To(BeTrue(), "unexpected Azure resource ID format: %s", zoneID) + subscriptionID = parts[1] + resourceGroupName = parts[3] + hostedZoneName = parts[7]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/issuer_acme_dns01_test.go` around lines 465 - 470, The current parsing of zoneID via fixed indexes after strings.Split can mis-parse; update the test to validate segment names and bounds before assigning subscriptionID/resourceGroupName/hostedZoneName: split zoneID into parts, check that parts length is sufficient and that parts[1]=="subscriptions", parts[3]=="resourceGroups", parts[5]=="providers" and parts[7]=="dnszones" (or locate those segment names dynamically), then extract the following elements into subscriptionID, resourceGroupName, and hostedZoneName; if any check fails, call Expect(...).To(HaveOccurred()/Fail) or similar with a clear error including the full zoneID so the test fails loudly rather than silently mis-parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/issuer_acme_dns01_test.go`:
- Around line 479-485: The test claims "ambient Azure credentials" but still
wires explicit TenantID/ClientID/ClientSecret in setupAmbientAzureCredentials
and the Issuer creation; either convert this to real ambient behavior when Azure
support is added to withCloudCredentials in credentials_request.go (remove
explicit TenantID/ClientID/ClientSecret usage in setupAmbientAzureCredentials
and the Issuer construction so the test relies on the operator-mapped ambient
secret) or change the test scope/naming and comments to reflect explicit
credential wiring (rename setupAmbientAzureCredentials and the test case, and
update the comment to say this is an explicit credential wiring test rather than
ambient); apply the chosen change consistently to the
setupAmbientAzureCredentials function, the Issuer construction that sets
TenantID/ClientID/ClientSecret, and the top comment describing the test.
---
Nitpick comments:
In `@test/e2e/issuer_acme_dns01_test.go`:
- Around line 465-470: The current parsing of zoneID via fixed indexes after
strings.Split can mis-parse; update the test to validate segment names and
bounds before assigning subscriptionID/resourceGroupName/hostedZoneName: split
zoneID into parts, check that parts length is sufficient and that
parts[1]=="subscriptions", parts[3]=="resourceGroups", parts[5]=="providers" and
parts[7]=="dnszones" (or locate those segment names dynamically), then extract
the following elements into subscriptionID, resourceGroupName, and
hostedZoneName; if any check fails, call Expect(...).To(HaveOccurred()/Fail) or
similar with a clear error including the full zoneID so the test fails loudly
rather than silently mis-parsing.
ℹ️ Review info
Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fd43aa9-f49f-4568-bf4a-fa1d10b88cff
📒 Files selected for processing (2)
test/e2e/issuer_acme_dns01_test.gotest/e2e/testdata/credentials/credentialsrequest_azure.yaml
lunarwhite
force-pushed
the
e2e-az
branch
2 times, most recently
from
6f04605 to
2b05a73
Compare
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@lunarwhite: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@lunarwhite: This pull request references CM-937 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test ? |
|
/test e2e-operator-azure-ovn |
2 participants
First (1/2) part of https://issues.redhat.com/browse/CM-937
Changes
Add new e2e tests to initiate Azure DNS-01 coverage:
Dependency
Summary by CodeRabbit