CNTRLPLANE-2217: feat(aws): migrate kms and Resource Tagging to AWS SDK v2#7795
CNTRLPLANE-2217: feat(aws): migrate kms and Resource Tagging to AWS SDK v2#7795LiangquanLi930 wants to merge 2 commits intoopenshift:mainfrom
Conversation
…lents Signed-off-by: Liangquan Li <liangli@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughMigrates AWS SDK usage from v1 to v2 across the codebase: KMS client calls and credential handling updated to SDK v2 with context propagation; resource-group-tagging client and types moved to v2; two AWS SDK v2 service dependencies added to go.mod; test call sites updated to pass context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
LiangquanLi930
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@LiangquanLi930: This pull request references CNTRLPLANE-2217 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
bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiangquanLi930 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 |
|
@LiangquanLi930: This pull request references CNTRLPLANE-2217 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2777-2793:⚠️ Potential issue | 🔴 CriticalGuard
r.awsSessionv2before dereference to avoid panic.At Line 2777,
awsSessionv2 := *r.awsSessionv2can panic when the AWS v2 config wasn’t initialized.GetEC2Clientmay leave it nil, so this path needs a defensive check.🔧 Proposed fix
+ if r.awsSessionv2 == nil { + condition := metav1.Condition{ + Type: string(hyperv1.ValidAWSKMSConfig), + ObservedGeneration: hcp.Generation, + Status: metav1.ConditionFalse, + Message: "AWS SDK v2 session is not initialized", + Reason: hyperv1.AWSErrorReason, + } + meta.SetStatusCondition(&hcp.Status.Conditions, condition) + return + } + awsSessionv2 := *r.awsSessionv2 awsSessionv2.Credentials = credentialsv2.NewStaticCredentialsProvider( v2creds.AccessKeyID, v2creds.SecretAccessKey, v2creds.SessionToken, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2777 - 2793, Guard the nil dereference of r.awsSessionv2 before assigning awsSessionv2: check if r.awsSessionv2 is nil (the path where GetEC2Client may leave it uninitialized) and handle that case (e.g., set the ValidAWSKMSConfig condition to False or return/error appropriately) instead of doing awsSessionv2 := *r.awsSessionv2; only create kms.NewFromConfig(awsSessionv2) after confirming r.awsSessionv2 is non-nil so you avoid a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2777-2793: Guard the nil dereference of r.awsSessionv2 before
assigning awsSessionv2: check if r.awsSessionv2 is nil (the path where
GetEC2Client may leave it uninitialized) and handle that case (e.g., set the
ValidAWSKMSConfig condition to False or return/error appropriately) instead of
doing awsSessionv2 := *r.awsSessionv2; only create
kms.NewFromConfig(awsSessionv2) after confirming r.awsSessionv2 is non-nil so
you avoid a panic.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (104)
go.sumis excluded by!**/*.sumvendor/github.com/aws/aws-sdk-go-v2/service/kms/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_CancelKeyDeletion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ConnectCustomKeyStore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_CreateAlias.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_CreateCustomKeyStore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_CreateGrant.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_CreateKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_Decrypt.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DeleteAlias.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DeleteCustomKeyStore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DeleteImportedKeyMaterial.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DeriveSharedSecret.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DescribeCustomKeyStores.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DescribeKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DisableKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DisableKeyRotation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_DisconnectCustomKeyStore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_EnableKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_EnableKeyRotation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_Encrypt.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateDataKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateDataKeyPair.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateDataKeyPairWithoutPlaintext.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateDataKeyWithoutPlaintext.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateMac.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GenerateRandom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GetKeyPolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GetKeyRotationStatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GetParametersForImport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_GetPublicKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ImportKeyMaterial.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListAliases.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListGrants.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListKeyPolicies.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListKeyRotations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListKeys.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListResourceTags.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ListRetirableGrants.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_PutKeyPolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ReEncrypt.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ReplicateKey.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_RetireGrant.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_RevokeGrant.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_RotateKeyOnDemand.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_ScheduleKeyDeletion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_Sign.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_TagResource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_UntagResource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_UpdateAlias.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_UpdateCustomKeyStore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_UpdateKeyDescription.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_UpdatePrimaryRegion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_Verify.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/api_op_VerifyMac.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/deserializers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/endpoints.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/generated.jsonis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/go_module_metadata.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/internal/endpoints/endpoints.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/serializers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/types/enums.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/types/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/types/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/kms/validators.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_DescribeReportCreation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_GetComplianceSummary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_GetResources.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_GetTagKeys.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_GetTagValues.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_ListRequiredTags.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_StartReportCreation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_TagResources.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/api_op_UntagResources.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/deserializers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/endpoints.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/generated.jsonis excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/go_module_metadata.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/internal/endpoints/endpoints.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/serializers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types/enums.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/validators.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/kms/service.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi/service.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gogo.modtest/e2e/create_cluster_test.gotest/e2e/nodepool_kms_root_volume_test.gotest/e2e/util/aws.gotest/e2e/util/fixture.go
Migrate validateAWSKMSConfig and e2e test utilities to v2 KMS and resourcegroupstaggingapi clients, propagating context through all callers and switching from pointer-type to value-type SDK fields. Signed-off-by: Liangquan Li <liangli@redhat.com>
LiangquanLi930
force-pushed
the
aws-sdk-v2-kms-tagging-2
branch
from
5bc9e9e to
d08dd07
Compare
|
@LiangquanLi930: This pull request references CNTRLPLANE-2217 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. |
|
@LiangquanLi930: This pull request references CNTRLPLANE-2217 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. |
|
@LiangquanLi930: This pull request references CNTRLPLANE-2217 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2777-2782:⚠️ Potential issue | 🟡 MinorGuard against nil dereference before dereferencing
r.awsSessionv2.If
r.awsSessionv2isnil(e.g.,GetEC2ClientreturnsnilwhenAWS_SHARED_CREDENTIALS_FILEis unset), line 2777 will panic. WhileAssumeRoleWithWebIdentityon line 2764 would likely surface the nil earlier, there is no explicit guard here.🛡️ Proposed defensive nil check
+ if r.awsSessionv2 == nil { + condition := metav1.Condition{ + Type: string(hyperv1.ValidAWSKMSConfig), + ObservedGeneration: hcp.Generation, + Status: metav1.ConditionFalse, + Message: "AWS session not initialized", + Reason: hyperv1.AWSErrorReason, + } + meta.SetStatusCondition(&hcp.Status.Conditions, condition) + return + } awsSessionv2 := *r.awsSessionv2 awsSessionv2.Credentials = credentialsv2.NewStaticCredentialsProvider(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2777 - 2782, Guard against nil dereference of r.awsSessionv2 before copying and mutating it: check if r.awsSessionv2 == nil and handle the case (return an error, log and return, or create a default session) instead of dereferencing; then only perform awsSessionv2 := *r.awsSessionv2 and set awsSessionv2.Credentials = credentialsv2.NewStaticCredentialsProvider(...) when non-nil. Reference the r.awsSessionv2 field and the awsSessionv2 local variable (and the earlier AssumeRoleWithWebIdentity call) to locate where to add the nil check and error-handling path.
🧹 Nitpick comments (1)
test/e2e/util/fixture.go (1)
296-321:GetResourcesis called without pagination — large clusters may silently miss resources.
GetResourcesis paginated; if the number of tagged resources exceeds the page limit,ResourceTagMappingListwill be incomplete and the cleanup validation could falsely report success. This is a pre-existing gap, but the migration is a good moment to address it.♻️ Suggested pagination loop
- output, err = taggingClient.GetResources(ctx, &resourcegroupstaggingapi.GetResourcesInput{ - ResourceTypeFilters: []string{ - "elasticloadbalancing:loadbalancer", - "ec2:volume", - "s3", - }, - TagFilters: []resourcegroupstaggingapitypes.TagFilter{ - { - Key: awsv2.String(clusterTag(infraID)), - Values: []string{"owned"}, - }, - }, - }) - if err != nil { - return true, err - } + input := &resourcegroupstaggingapi.GetResourcesInput{ + ResourceTypeFilters: []string{ + "elasticloadbalancing:loadbalancer", + "ec2:volume", + "s3", + }, + TagFilters: []resourcegroupstaggingapitypes.TagFilter{ + { + Key: awsv2.String(clusterTag(infraID)), + Values: []string{"owned"}, + }, + }, + } + output = &resourcegroupstaggingapi.GetResourcesOutput{} + for { + page, err := taggingClient.GetResources(ctx, input) + if err != nil { + return true, err + } + output.ResourceTagMappingList = append(output.ResourceTagMappingList, page.ResourceTagMappingList...) + if page.PaginationToken == nil || *page.PaginationToken == "" { + break + } + input.PaginationToken = page.PaginationToken + }Happy to open a follow-up issue to track this if you'd like to defer it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/fixture.go` around lines 296 - 321, The current GetResources call inside the wait.PollUntilContextTimeout loop may miss pages; replace the single taggingClient.GetResources call with a paginated loop that iterates all pages (using the SDK paginator or NextToken) and accumulates ResourceTagMappingList into a single slice before calling hasGuestResources; ensure you still pass the same GetResourcesInput (resource filters and TagFilters) and use the combined list for the hasGuestResources check so cleanup validation is correct (refer to taggingClient.GetResources, GetResourcesInput, output.ResourceTagMappingList, hasGuestResources, and the surrounding wait.PollUntilContextTimeout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2777-2782: Guard against nil dereference of r.awsSessionv2 before
copying and mutating it: check if r.awsSessionv2 == nil and handle the case
(return an error, log and return, or create a default session) instead of
dereferencing; then only perform awsSessionv2 := *r.awsSessionv2 and set
awsSessionv2.Credentials = credentialsv2.NewStaticCredentialsProvider(...) when
non-nil. Reference the r.awsSessionv2 field and the awsSessionv2 local variable
(and the earlier AssumeRoleWithWebIdentity call) to locate where to add the nil
check and error-handling path.
---
Nitpick comments:
In `@test/e2e/util/fixture.go`:
- Around line 296-321: The current GetResources call inside the
wait.PollUntilContextTimeout loop may miss pages; replace the single
taggingClient.GetResources call with a paginated loop that iterates all pages
(using the SDK paginator or NextToken) and accumulates ResourceTagMappingList
into a single slice before calling hasGuestResources; ensure you still pass the
same GetResourcesInput (resource filters and TagFilters) and use the combined
list for the hasGuestResources check so cleanup validation is correct (refer to
taggingClient.GetResources, GetResourcesInput, output.ResourceTagMappingList,
hasGuestResources, and the surrounding wait.PollUntilContextTimeout).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gotest/e2e/create_cluster_test.gotest/e2e/nodepool_kms_root_volume_test.gotest/e2e/util/aws.gotest/e2e/util/fixture.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/create_cluster_test.go
Test Resultse2e-aws
e2e-aks
|
|
/test e2e-aks-4-21, e2e-aws, e2e-aws-upgrade-hypershift-operator, e2e-kubevirt-aws-ovn-reduced, e2e-v2-aws |
|
/test e2e-aws, e2e-aws-4-21 |
1 similar comment
|
/test e2e-aws, e2e-aws-4-21 |
|
/test e2e-aws-upgrade-hypershift-operator, e2e-kubevirt-aws-ovn-reduced, e2e-v2-aws |
|
/test e2e-aws |
|
/test e2e-aws-upgrade-hypershift-operator |
|
/test e2e-v2-aws |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-aks |
|
/test e2e-aws-4-21 |
|
/test e2e-aws |
|
/test e2e-aks |
|
@LiangquanLi930: The following tests failed, say
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. |
3 participants
What this PR does / why we need it:
migrate kms and Resource Tagging to AWS SDK v2
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-2217
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Chores
Refactor
Tests