CNTRLPLANE-3562, CNTRLPLANE-3563: test(healthcheck): add unit tests for AWS identity provider#8829
CNTRLPLANE-3562, CNTRLPLANE-3563: test(healthcheck): add unit tests for AWS identity provider#8829mgencur wants to merge 4 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mgencur: This pull request references CNTRLPLANE-3562 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 task to target the "5.0.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
Bot
added
do-not-merge/needs-area
area/control-plane-operator
openshift-ci
Bot
added
the
area/platform/aws
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAWS identity-provider health checking now uses a shared EC2-backed helper. Hostedcluster AWS cleanup now logs endpoint-service finalizer removal with a credential-based reason, treats partial S3 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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.
🧹 Nitpick comments (1)
control-plane-operator/controllers/healthcheck/aws_test.go (1)
159-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelper-direct branch leaves the entrypoint wiring untested.
For all KAS-
Truecases the test callsvalidateAWSIdentityProviderdirectly and never exercisesawsHealthCheckIdentityProvider→GetEC2Client(ctx). That's the seam most likely to regress (see the typed-nil concern inaws.go), so the EC2-client acquisition path stays uncovered. Consider a follow-up that drives the real entrypoint, or at minimum a comment documenting that the helper is tested in isolation by design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/healthcheck/aws_test.go` around lines 159 - 178, The KAS-True branch in the AWS healthcheck test bypasses the real entrypoint and only exercises validateAWSIdentityProvider, leaving awsHealthCheckIdentityProvider and GetEC2Client(ctx) untested. Update the test in aws_test.go to cover the actual awsHealthCheckIdentityProvider path for KAS-True cases, or add an explicit note if helper-only coverage is intentional, so the EC2 client acquisition seam is verified alongside validateAWSIdentityProvider and aws.go behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@control-plane-operator/controllers/healthcheck/aws_test.go`:
- Around line 159-178: The KAS-True branch in the AWS healthcheck test bypasses
the real entrypoint and only exercises validateAWSIdentityProvider, leaving
awsHealthCheckIdentityProvider and GetEC2Client(ctx) untested. Update the test
in aws_test.go to cover the actual awsHealthCheckIdentityProvider path for
KAS-True cases, or add an explicit note if helper-only coverage is intentional,
so the EC2 client acquisition seam is verified alongside
validateAWSIdentityProvider and aws.go behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4f062843-bcb5-4f51-8a7a-538fff759525
📒 Files selected for processing (2)
control-plane-operator/controllers/healthcheck/aws.gocontrol-plane-operator/controllers/healthcheck/aws_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8829 +/- ##
==========================================
+ Coverage 42.95% 43.21% +0.26%
==========================================
Files 766 766
Lines 94722 94884 +162
==========================================
+ Hits 40688 41008 +320
+ Misses 51223 51021 -202
- Partials 2811 2855 +44
... and 14 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Extract validateAWSIdentityProvider from awsHealthCheckIdentityProvider to enable testing with a mock EC2 client. Add test cases covering DescribeVpcEndpoints error paths (WebIdentityErr, other API errors, non-API errors) and the success path that were previously untested. Signed-off-by: Martin Gencur <mgencur@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mgencur
force-pushed
the
CNTRLPLANE-3562_idp_deletion
branch
from
7445f05 to
efbc004
Compare
mgencur
changed the title
|
@mgencur: This pull request references CNTRLPLANE-3562 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 task to target the "5.0.0" version, but no target version was set. This pull request references CNTRLPLANE-3563 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 task to target the "5.0.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
Bot
added
the
area/hypershift-operator
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hypershift-operator/controllers/hostedcluster/aws_oidc_test.go (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
ctrlmock parameter to avoid shadowing the controller-runtime alias.Each
setupS3Mockclosure introducesctrl *gomock.Controller, which shadows the importedctrlpackage alias used later on Line 146. Renaming the parameter keeps the test less error-prone and matches the repo's Go guideline. As per coding guidelines,**/!(*.pb).go: Avoid variable shadowing in Go files.Also applies to: 64-65, 85-86, 104-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/hostedcluster/aws_oidc_test.go` around lines 45 - 46, The setupS3Mock closures use a parameter named ctrl that shadows the imported controller-runtime alias used elsewhere in aws_oidc_test.go. Rename each closure parameter in the affected test cases to a distinct name, and update the NewMockS3API call within those closures accordingly so the gomock controller argument is still passed through without variable shadowing.Source: Coding guidelines
hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go (1)
41-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case covering the grace-period skip path.
Both table entries exercise finalizer removal. There's no case for
GetCredentialStatus == ValidwithinawsEndpointDeletionGracePeriod, wheredeleteAWSEndpointServiceshits the earlycontinueand the CPO finalizer must be preserved (andpendingstaystrue). Adding it locks in the grace-period guard against regressions.{ name: "When endpoint is deleting with valid creds inside grace period, it should preserve CPO finalizer", hc: hostedClusterWithCredentialConditions(metav1.ConditionTrue, metav1.ConditionTrue), endpoints: []hyperv1.AWSEndpointService{ { ObjectMeta: metav1.ObjectMeta{ Name: "ep-1", Namespace: namespace, DeletionTimestamp: &metav1.Time{Time: time.Now().Add(-1 * time.Minute)}, Finalizers: []string{cpoFinalizer}, }, }, }, expectPending: true, expectFinalizerRemoved: false, },Note: the current assertion block only verifies removal when
expectFinalizerRemovedis true; you'll want a matching branch to assert the finalizer is still present for this case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go` around lines 41 - 81, Add a negative test case in aws_endpoint_services_test for the grace-period skip path in deleteAWSEndpointServices: when GetCredentialStatus is Valid but the AWSEndpointService deletion timestamp is still inside awsEndpointDeletionGracePeriod, the CPO finalizer should be preserved and pending should remain true. Update the table-driven test alongside the existing hostedClusterWithCredentialConditions cases, and extend the final assertion logic to handle expectFinalizerRemoved == false by verifying the cpoFinalizer is still present on the endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go`:
- Around line 361-377: TestDeleteOrphanedMachines is missing coverage for the
valid-creds skip-cleanup path because the finalizer assertion only runs for
machines without a DeletionTimestamp. Update the test case for “When credentials
are valid, it should skip cleanup” so it actually verifies that test-finalizer
remains on a non-deleting AWSMachine, either by removing the DeletionTimestamp
guard in the finalizer check or by adding a machine that is not marked for
deletion. Use the existing TestDeleteOrphanedMachines,
tc.expectFinalizersCleared, and machineList checks to locate the assertion
logic.
---
Nitpick comments:
In `@hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.go`:
- Around line 41-81: Add a negative test case in aws_endpoint_services_test for
the grace-period skip path in deleteAWSEndpointServices: when
GetCredentialStatus is Valid but the AWSEndpointService deletion timestamp is
still inside awsEndpointDeletionGracePeriod, the CPO finalizer should be
preserved and pending should remain true. Update the table-driven test alongside
the existing hostedClusterWithCredentialConditions cases, and extend the final
assertion logic to handle expectFinalizerRemoved == false by verifying the
cpoFinalizer is still present on the endpoint.
In `@hypershift-operator/controllers/hostedcluster/aws_oidc_test.go`:
- Around line 45-46: The setupS3Mock closures use a parameter named ctrl that
shadows the imported controller-runtime alias used elsewhere in
aws_oidc_test.go. Rename each closure parameter in the affected test cases to a
distinct name, and update the NewMockS3API call within those closures
accordingly so the gomock controller argument is still passed through without
variable shadowing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c9e6fbef-8bf4-42f4-9687-f666ee7ac93b
📒 Files selected for processing (4)
hypershift-operator/controllers/hostedcluster/aws_endpoint_services_test.gohypershift-operator/controllers/hostedcluster/aws_oidc_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/internal/platform/aws/aws_test.go
Add tests for the AWS identity provider deletion path (OCP-60484):
- TestCleanupAWSOIDCBucketData: S3 cleanup, error handling, NoSuchBucket tolerance
- TestDeleteAWSEndpointServices: CPO finalizer removal for invalid creds and expired grace period
- TestDeleteOrphanedMachines: AWSMachine finalizer cleanup based on credential status
These tests exercise the condition chain: OIDC upload fails → no finalizer → cleanup no-op →
GetCredentialStatus=Invalid → deleteAWSEndpointServices removes CPO finalizer → DeleteOrphanedMachines clears
AWSMachine finalizers → deletion proceeds.
Improve existing TestGetCredentialStatus: standardize test names to
"it should" convention, replace &[]T{v}[0] with ptr.To(), and extract
hostedClusterWithCredentialConditions helper to reduce duplication.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… endpoint service log Differentiate between "no valid credentials" and "grace period expired" when logging CPO finalizer removal for AWSEndpointService resources. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mgencur
force-pushed
the
CNTRLPLANE-3562_idp_deletion
branch
from
7babf03 to
8cefa9b
Compare
The S3 DeleteObjects API can return (output, nil) with per-object errors in output.Errors, meaning some objects failed to delete while the call itself succeeded. Previously the output was discarded, so partial failures silently removed the finalizer, leaving orphaned S3 objects with no retry path. Capture the output and return an error on partial failure, keeping the finalizer in place so the controller retries on the next reconcile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mgencur
force-pushed
the
CNTRLPLANE-3562_idp_deletion
branch
from
8cefa9b to
5293291
Compare
|
Now I have all the data. Let me verify one detail about the exact check run ID: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is caused by insufficient test coverage on newly added/modified executable lines in two production files: File 1: The PR extracted
File 2: Lines 3549–3554 ( Lines 4855–4867 ( Recommendations
Evidence
|
|
@mgencur: 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. |
2 participants
What this PR does / why we need it:
Adds unit tests for the AWS identity provider deletion path and fixes two production code issues discovered during review.
Tests:
Fixes:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3562
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3563
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit