CNTRLPLANE-3763: tag CLI and e2e AWS resources with infra-id, cluster-name, and source - #8909
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request references CNTRLPLANE-3763 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 "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
area/cli
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared Hypershift AWS tag keys and threads Sequence Diagram(s)sequenceDiagram
participant createCluster
participant E2ETagsFromEnvironment
participant CreateInfra
participant CreateIAM
createCluster->>E2ETagsFromEnvironment: read environment tags
createCluster->>CreateInfra: append tags for infrastructure creation
createCluster->>CreateIAM: append tags for IAM creation
sequenceDiagram
participant CreateBastionOpts.Run
participant AWS EC2 API
participant Route53API
CreateBastionOpts.Run->>AWS EC2 API: create bastion resources with infra-id and cluster-name tags
CreateBastionOpts.Run->>Route53API: change hosted zone tags
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Still a WIP /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8909 +/- ##
==========================================
+ Coverage 43.45% 43.49% +0.03%
==========================================
Files 771 771
Lines 95718 95789 +71
==========================================
+ Hits 41597 41659 +62
+ Misses 51234 51228 -6
- Partials 2887 2902 +15
... and 2 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
cmd/bastion/aws/create.go (1)
105-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty
cluster-nametag value when bastion is created via--infra-id/--region.When
Runis invoked without--name(theelsebranch at Line 139-142, required whenever--infra-id/--regionare used perValidate),o.Nameis empty, soclusterNamepassed intoensureBastionSecurityGroup,ensureBastionKeyPair, andrunEC2BastionInstanceis"". Unlikeec2Tagsincmd/infra/aws/ec2.go(which only appendsHypershiftClusterNameTagKey/HypershiftInfraIDTagKeywhen non-empty), these three call sites unconditionally add thehypershift.openshift.io/cluster-nametag with an empty value, producing a meaningless empty tag on the bastion security group, key pair, and EC2 instance.🏷️ Proposed fix (illustrated for the security group; apply the same pattern at the key-pair and instance tag sites)
+func hypershiftTags(infraID, clusterName string) []ec2types.Tag { + var tags []ec2types.Tag + if len(infraID) > 0 { + tags = append(tags, ec2types.Tag{ + Key: aws.String("hypershift.openshift.io/infra-id"), + Value: aws.String(infraID), + }) + } + if len(clusterName) > 0 { + tags = append(tags, ec2types.Tag{ + Key: aws.String("hypershift.openshift.io/cluster-name"), + Value: aws.String(clusterName), + }) + } + return tags +} + Tags: []ec2types.Tag{ { Key: aws.String(fmt.Sprintf("kubernetes.io/cluster/%s", infraID)), Value: aws.String("owned"), }, { Key: aws.String("Name"), Value: aws.String(name), }, - { - Key: aws.String("hypershift.openshift.io/infra-id"), - Value: aws.String(infraID), - }, - { - Key: aws.String("hypershift.openshift.io/cluster-name"), - Value: aws.String(clusterName), - }, - }, + }..., + append(hypershiftTags(infraID, clusterName)), + ),Also applies to: 221-228, 377-384, 512-519
🤖 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 `@cmd/bastion/aws/create.go` around lines 105 - 186, The bastion tagging logic in Run and its helpers is unconditionally emitting the cluster-name tag even when o.Name is empty in the --infra-id/--region path. Update ensureBastionSecurityGroup, ensureBastionKeyPair, and runEC2BastionInstance so they only add hypershift.openshift.io/cluster-name when the cluster name is non-empty, following the same conditional tag pattern used by ec2Tags in cmd/infra/aws/ec2.go.cmd/infra/aws/route53.go (1)
74-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTags are never applied when the private hosted zone already exists.
CreatePrivateZoneonly callsChangeTagsForResourceon the newly-created-zone path (Lines 117-128). The early-return "found existing private zone" path (Lines 75-83) skips tagging entirely and returns immediately aftersetSOAMinimum. Since this lookup-then-create flow is explicitly designed to be idempotent (re-running infra creation against an existing cluster), any hosted zone created before this PR — or any zone whose tagging call previously failed after the zone was successfully created — will never get thehypershift.openshift.io/infra-id/cluster-nametags on subsequent reconciles, permanently defeating the goal of consistent per-cluster tagging for this resource type.🏷️ Proposed fix: tag on both the found-existing and newly-created paths
func (o *CreateInfraOptions) CreatePrivateZone(ctx context.Context, logger logr.Logger, client awsapi.ROUTE53API, name, vpcID string, authorizeAssociation bool, vpcOwnerClient awsapi.ROUTE53API, initialVPC string) (string, error) { id, err := LookupZone(ctx, client, name, true) if err == nil { logger.Info("Found existing private zone", "name", name, "id", id) + if _, err := client.ChangeTagsForResource(ctx, &route53.ChangeTagsForResourceInput{ + ResourceId: aws.String(id), + ResourceType: route53types.TagResourceTypeHostedzone, + AddTags: []route53types.Tag{ + {Key: aws.String(clusterTag(o.InfraID)), Value: aws.String(clusterTagValue)}, + {Key: aws.String(HypershiftInfraIDTagKey), Value: aws.String(o.InfraID)}, + {Key: aws.String(HypershiftClusterNameTagKey), Value: aws.String(o.Name)}, + }, + }); err != nil { + return "", fmt.Errorf("failed to tag hosted zone: %w", err) + } err := setSOAMinimum(ctx, client, id, name) if err != nil { return "", err } return id, err }🤖 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 `@cmd/infra/aws/route53.go` around lines 74 - 129, CreatePrivateZone only tags Route53 hosted zones on the create path, so existing private zones are returned without applying the required cluster tags. Update CreatePrivateZone to call ChangeTagsForResource in both the LookupZone success path and the newly-created-zone path, reusing the same tag set built from clusterTag(o.InfraID), HypershiftInfraIDTagKey, and HypershiftClusterNameTagKey, while keeping the existing setSOAMinimum behavior and error handling intact.
🧹 Nitpick comments (5)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2856-2859: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMap-based assignment is safe from duplicate-key API errors, but silently overrides user tags.
Unlike the slice-based tagging in
awsprivatelink_controller.go, this uses amap[string]string, so settingtags["hypershift.openshift.io/infra-id"]etc. unconditionally won't cause an AWS API duplicate-key error — but it will silently override a user-suppliedResourceTagsentry with the same key, whereas theclusterKey/Name/karpenter.sh/discoverytags above use an existence check (if _, exist := ...) before setting. Consider applying the same existence guard for consistency, or intentionally document that these hypershift-reserved tags always take precedence.🤖 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/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2856 - 2859, The tag assignment in hostedcontrolplane_controller.go unconditionally overwrites any existing user-provided entries in the tags map for the hypershift reserved keys. Update the tag population around the hosted control plane resource creation to match the existing pattern used for clusterKey/Name/karpenter.sh/discovery by checking whether each key already exists before setting it, using the same symbols and tag map in the hostedcontrolplane controller. If the overwrite is intentional, add a brief note in that tag-building block to document that these hypershift tags always take precedence.cmd/fix/dr_oidc_iam.go (2)
813-817: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManual query-string concatenation for S3 object tagging.
Building the
Taggingvalue via string concatenation ("key1=" + esc(v1) + "&key2=" + esc(v2)) works for the current two fixed keys but is easy to break if a third tag is added later (missing&, wrong escaping order, etc.). Prefernet/url.Valuesto build and encode the query string.♻️ Proposed refactor using url.Values
+ tagValues := url.Values{} + tagValues.Set("hypershift.openshift.io/infra-id", o.InfraID) + tagValues.Set("hypershift.openshift.io/cluster-name", o.HostedClusterName) _, err = s3Client.PutObject(ctx, &s3.PutObjectInput{ Bucket: aws.String(o.OIDCStorageProviderS3Bucket), Key: aws.String(o.InfraID + path), Body: bodyReader, ContentType: aws.String("application/json"), - Tagging: aws.String("hypershift.openshift.io/infra-id=" + url.QueryEscape(o.InfraID) + "&hypershift.openshift.io/cluster-name=" + url.QueryEscape(o.HostedClusterName)), + Tagging: aws.String(tagValues.Encode()), })🤖 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 `@cmd/fix/dr_oidc_iam.go` around lines 813 - 817, The S3 object Tagging value is being assembled manually in the request built around o.InfraID and o.HostedClusterName, which is fragile for future tags. Update the tagging construction to use net/url.Values in the same spot where the aws.String(...) request fields are set, so each tag is added as a key/value pair and encoded once. Keep the existing tag names and values but let url.Values generate the final query string before assigning it to Tagging.
761-773: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared tag key constants here. These literals duplicate
HypershiftInfraIDTagKeyandHypershiftClusterNameTagKeyfromcmd/infra/aws/create.go; switching the bucket tags, object tagging, and OIDC provider tags to the shared constants avoids drift and typos.🤖 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 `@cmd/fix/dr_oidc_iam.go` around lines 761 - 773, The S3 bucket tagging block in the OIDC IAM flow is hardcoding tag key strings that duplicate the shared tag constants. Update the tagging logic around s3Client.PutBucketTagging in dr_oidc_iam.go to use HypershiftInfraIDTagKey and HypershiftClusterNameTagKey from cmd/infra/aws/create.go instead of inline literals, and apply the same shared constants anywhere object tagging or OIDC provider tags are set to keep keys consistent and avoid drift.test/e2e/util/aws.go (2)
217-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFixed tags +
additionalTagsappended via slice concat risks duplicate EC2 tag keys.
CreateTestSubnetandCreateCapacityReservationbuild a fixed[]ec2types.Tag(withhypershift.openshift.io/infra-id/cluster-name) and then blindly append every entry from the caller-suppliedadditionalTagsmap. Today's only caller passesE2ETagsFromEnvironment()(which only containssource/prow-job-id), so there's no overlap — but if a future caller passesadditionalTagscontaininginfra-idorcluster-name, the resultingTagSpecificationwould contain duplicate keys, which EC2 rejects. Contrast this withnodepool_spot_termination_handler_test.go, which merges tags via map assignment and is immune to this.Merging via a map before converting to
[]ec2types.Tagwould make this robust regardless of caller input.♻️ Proposed fix using map-based merge (illustrated for CreateTestSubnet)
- subnetTags := []ec2types.Tag{ - {Key: awsv2.String("Name"), Value: awsv2.String(subnetName)}, - {Key: awsv2.String(fmt.Sprintf("kubernetes.io/cluster/%s", infraID)), Value: awsv2.String("owned")}, - {Key: awsv2.String("hypershift.openshift.io/infra-id"), Value: awsv2.String(infraID)}, - {Key: awsv2.String("hypershift.openshift.io/cluster-name"), Value: awsv2.String(clusterName)}, - } - for k, v := range additionalTags { - subnetTags = append(subnetTags, ec2types.Tag{Key: awsv2.String(k), Value: awsv2.String(v)}) - } + tagMap := map[string]string{ + "Name": subnetName, + fmt.Sprintf("kubernetes.io/cluster/%s", infraID): "owned", + "hypershift.openshift.io/infra-id": infraID, + "hypershift.openshift.io/cluster-name": clusterName, + } + for k, v := range additionalTags { + tagMap[k] = v + } + subnetTags := make([]ec2types.Tag, 0, len(tagMap)) + for k, v := range tagMap { + subnetTags = append(subnetTags, ec2types.Tag{Key: awsv2.String(k), Value: awsv2.String(v)}) + }Also applies to: 371-398
🤖 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 `@test/e2e/util/aws.go` around lines 217 - 233, The tag construction in CreateTestSubnet (and the similar CreateCapacityReservation path) currently appends additionalTags directly onto a fixed slice, which can produce duplicate EC2 tag keys if callers override infra-id or cluster-name. Merge the fixed tags and additionalTags into a map first, letting caller-supplied values replace defaults, then convert that merged set back into the []ec2types.Tag used in the CreateSubnetInput/TagSpecifications. Keep the existing tag keys and behavior, but make the merge duplicate-safe for future callers.
35-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded tag-key strings duplicate the new constants from cohort 1.
The PR introduces exported constants for
hypershift.openshift.io/infra-id,hypershift.openshift.io/cluster-name, andhypershift.openshift.io/sourceincmd/infra/aws/create.go(per the stack summary), but this function re-hardcodes"hypershift.openshift.io/source"as a literal, andCreateTestSubnet/CreateCapacityReservationfurther down hardcode"hypershift.openshift.io/infra-id"and"hypershift.openshift.io/cluster-name". If those keys ever change, this file (andnodepool_spot_termination_handler_test.go) would silently drift out of sync.Consider importing the shared constants if they're exported from an importable package, to keep a single source of truth.
#!/bin/bash # Locate where the new hypershift tag-key constants are defined and check if they're exported/importable. rg -n 'hypershift.openshift.io/(infra-id|cluster-name|source)' --type=go -C2🤖 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 `@test/e2e/util/aws.go` around lines 35 - 47, The tag-key literals in E2ETagsFromEnvironment, CreateTestSubnet, and CreateCapacityReservation are duplicating the new shared hypershift tag constants and can drift out of sync. Update this test helper to use the exported constants for hypershift.openshift.io/source, hypershift.openshift.io/infra-id, and hypershift.openshift.io/cluster-name instead of hardcoded strings. If the constants are not yet in an importable package, move or re-export them there and use the shared source of truth everywhere, including nodepool_spot_termination_handler_test.go.
🤖 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
`@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go`:
- Around line 1087-1100: The new hypershift tags are being appended
unconditionally in awsprivatelink_controller.go, which can conflict with
user-provided ResourceTags and cause CreateSecurityGroup to fail on duplicate
tag keys. Update the existing tagKeys-based deduplication logic in the same flow
that handles clusterKey and Name so the new infra-id, cluster-name, and source
tags are only added when their keys are not already present. Use the existing
tagKeys guard in the controller method that builds tags for the security group.
- Around line 864-878: The tag assembly in awsprivatelink_controller.go can emit
duplicate EC2 tag keys when apiTagToEC2Tag includes user-supplied ResourceTags
that match the hypershift tags being appended. Update the tag-building logic
around the createVpcEndpoint path (and the matching createSecurityGroup flow) to
upsert or dedupe by Key instead of blindly appending, and switch the repeated
hypershift tag literals to shared exported constants so the same keys are used
consistently across the controller and infra helpers.
In `@hypershift-operator/controllers/platform/aws/controller.go`:
- Around line 564-578: The TagSpecifications built in the AWS controller’s
endpoint service path can end up with duplicate Tag Key entries because `tags`
starts from `apiTagToEC2Tag(awsEndpointService.Spec.ResourceTags)` and then
unconditionally appends the Hypershift tags. Update the tag assembly in the
`controller.go` flow to guard against existing keys before appending the
`hypershift.openshift.io/infra-id`, `hypershift.openshift.io/cluster-name`, and
`hypershift.openshift.io/source` tags, so
`CreateVpcEndpointServiceConfiguration` never receives duplicate keys.
---
Outside diff comments:
In `@cmd/bastion/aws/create.go`:
- Around line 105-186: The bastion tagging logic in Run and its helpers is
unconditionally emitting the cluster-name tag even when o.Name is empty in the
--infra-id/--region path. Update ensureBastionSecurityGroup,
ensureBastionKeyPair, and runEC2BastionInstance so they only add
hypershift.openshift.io/cluster-name when the cluster name is non-empty,
following the same conditional tag pattern used by ec2Tags in
cmd/infra/aws/ec2.go.
In `@cmd/infra/aws/route53.go`:
- Around line 74-129: CreatePrivateZone only tags Route53 hosted zones on the
create path, so existing private zones are returned without applying the
required cluster tags. Update CreatePrivateZone to call ChangeTagsForResource in
both the LookupZone success path and the newly-created-zone path, reusing the
same tag set built from clusterTag(o.InfraID), HypershiftInfraIDTagKey, and
HypershiftClusterNameTagKey, while keeping the existing setSOAMinimum behavior
and error handling intact.
---
Nitpick comments:
In `@cmd/fix/dr_oidc_iam.go`:
- Around line 813-817: The S3 object Tagging value is being assembled manually
in the request built around o.InfraID and o.HostedClusterName, which is fragile
for future tags. Update the tagging construction to use net/url.Values in the
same spot where the aws.String(...) request fields are set, so each tag is added
as a key/value pair and encoded once. Keep the existing tag names and values but
let url.Values generate the final query string before assigning it to Tagging.
- Around line 761-773: The S3 bucket tagging block in the OIDC IAM flow is
hardcoding tag key strings that duplicate the shared tag constants. Update the
tagging logic around s3Client.PutBucketTagging in dr_oidc_iam.go to use
HypershiftInfraIDTagKey and HypershiftClusterNameTagKey from
cmd/infra/aws/create.go instead of inline literals, and apply the same shared
constants anywhere object tagging or OIDC provider tags are set to keep keys
consistent and avoid drift.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2856-2859: The tag assignment in hostedcontrolplane_controller.go
unconditionally overwrites any existing user-provided entries in the tags map
for the hypershift reserved keys. Update the tag population around the hosted
control plane resource creation to match the existing pattern used for
clusterKey/Name/karpenter.sh/discovery by checking whether each key already
exists before setting it, using the same symbols and tag map in the
hostedcontrolplane controller. If the overwrite is intentional, add a brief note
in that tag-building block to document that these hypershift tags always take
precedence.
In `@test/e2e/util/aws.go`:
- Around line 217-233: The tag construction in CreateTestSubnet (and the similar
CreateCapacityReservation path) currently appends additionalTags directly onto a
fixed slice, which can produce duplicate EC2 tag keys if callers override
infra-id or cluster-name. Merge the fixed tags and additionalTags into a map
first, letting caller-supplied values replace defaults, then convert that merged
set back into the []ec2types.Tag used in the
CreateSubnetInput/TagSpecifications. Keep the existing tag keys and behavior,
but make the merge duplicate-safe for future callers.
- Around line 35-47: The tag-key literals in E2ETagsFromEnvironment,
CreateTestSubnet, and CreateCapacityReservation are duplicating the new shared
hypershift tag constants and can drift out of sync. Update this test helper to
use the exported constants for hypershift.openshift.io/source,
hypershift.openshift.io/infra-id, and hypershift.openshift.io/cluster-name
instead of hardcoded strings. If the constants are not yet in an importable
package, move or re-export them there and use the shared source of truth
everywhere, including nodepool_spot_termination_handler_test.go.
🪄 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: 75070588-359c-404e-a034-3a7b929d8a0a
📒 Files selected for processing (16)
cmd/bastion/aws/create.gocmd/cluster/aws/create.gocmd/fix/dr_oidc_iam.gocmd/infra/aws/create.gocmd/infra/aws/create_iam.gocmd/infra/aws/delegatingclientgenerator/main.gocmd/infra/aws/ec2.gocmd/infra/aws/route53.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gohypershift-operator/controllers/platform/aws/controller.gosupport/awsapi/route53.gotest/e2e/karpenter_test.gotest/e2e/nodepool_spot_termination_handler_test.gotest/e2e/util/aws.gotest/e2e/util/fixture.go
d1e539b to
0ceda54
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/infra/aws/route53_test.go (1)
211-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify actual tag content passed to
ChangeTagsForResource.Both new expectations use
gomock.Any()for the input argument, so the test only confirms the call happens — it doesn't assert thatAddTagsactually containsHypershiftInfraIDTagKey/HypershiftClusterNameTagKey(the behavior this PR is adding). A regression that drops or mis-sets these tags inCreatePrivateZonewould not be caught.♻️ Suggested matcher to validate tag content
- m.EXPECT().ChangeTagsForResource(gomock.Any(), gomock.Any(), gomock.Any()). - Return(&route53.ChangeTagsForResourceOutput{}, nil) + m.EXPECT().ChangeTagsForResource(gomock.Any(), gomock.AssignableToTypeOf(&route53.ChangeTagsForResourceInput{}), gomock.Any()). + DoAndReturn(func(_ context.Context, in *route53.ChangeTagsForResourceInput, _ ...func(*route53.Options)) (*route53.ChangeTagsForResourceOutput, error) { + tagKeys := map[string]string{} + for _, tag := range in.AddTags { + tagKeys[aws.ToString(tag.Key)] = aws.ToString(tag.Value) + } + if tagKeys[supportawsutil.HypershiftInfraIDTagKey] == "" || tagKeys[supportawsutil.HypershiftClusterNameTagKey] == "" { + t.Errorf("expected hypershift tags in ChangeTagsForResource, got %v", tagKeys) + } + return &route53.ChangeTagsForResourceOutput{}, nil + })Also applies to: 238-239
🤖 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 `@cmd/infra/aws/route53_test.go` around lines 211 - 212, The Route53 test is only checking that ChangeTagsForResource is called, not that the request contains the expected tags. Update the expectations in CreatePrivateZone-related tests to inspect the input passed to ChangeTagsForResource and assert that AddTags includes HypershiftInfraIDTagKey and HypershiftClusterNameTagKey with the correct values, using the relevant gomock matcher or a custom argument check instead of gomock.Any().
🤖 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 `@cmd/infra/aws/route53_test.go`:
- Around line 211-212: The Route53 test is only checking that
ChangeTagsForResource is called, not that the request contains the expected
tags. Update the expectations in CreatePrivateZone-related tests to inspect the
input passed to ChangeTagsForResource and assert that AddTags includes
HypershiftInfraIDTagKey and HypershiftClusterNameTagKey with the correct values,
using the relevant gomock matcher or a custom argument check instead of
gomock.Any().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d19cd033-76a3-465c-96ef-9a388025d342
📒 Files selected for processing (19)
cmd/bastion/aws/create.gocmd/cluster/aws/create.gocmd/fix/dr_oidc_iam.gocmd/infra/aws/create.gocmd/infra/aws/create_iam.gocmd/infra/aws/delegatingclientgenerator/main.gocmd/infra/aws/ec2.gocmd/infra/aws/route53.gocmd/infra/aws/route53_test.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gohypershift-operator/controllers/platform/aws/controller.gosupport/awsapi/route53.gosupport/awsutil/tags.gotest/e2e/karpenter_test.gotest/e2e/nodepool_spot_termination_handler_test.gotest/e2e/util/aws.gotest/e2e/util/fixture.go
✅ Files skipped from review due to trivial changes (1)
- support/awsutil/tags.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/infra/aws/delegatingclientgenerator/main.go
- support/awsapi/route53.go
- cmd/infra/aws/create_iam.go
- hypershift-operator/controllers/platform/aws/controller.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- cmd/infra/aws/route53.go
- test/e2e/karpenter_test.go
- test/e2e/util/fixture.go
- cmd/infra/aws/create.go
- test/e2e/nodepool_spot_termination_handler_test.go
- cmd/fix/dr_oidc_iam.go
- cmd/cluster/aws/create.go
- test/e2e/util/aws.go
- cmd/bastion/aws/create.go
- control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
- cmd/infra/aws/ec2.go
0ceda54 to
e193ed0
Compare
|
@ironcladlou before merging this, we should check with rosa SRE to make sure we're not impacting customer tags. I vaguely remember that there is a limit on the number of tags you can have for AWS resources, and having our own tags reduce that number. |
|
@csrwng okay, will follow up... a quick AI analysis shows this change will result in 2-4 tags per resource, about 70-90 tags total per cluster (public/private). Haven't deeply vetted those numbers but it might be a good starting place for the discussion |
|
Something else I can do right away to conserve tags is to collapse these into a single encoded field like |
|
Actually, if I pack those elements into a single tag, filtering with the tag API becomes troublesome... need to think about that some more |
|
Talked with @csrwng and since our primary area of concern is infra created from ad-hoc and CI flows through the CLI and e2e infra, we will remove the CPO from the scope of these changes because adding tags there would consume customer tag quota and needs to be more carefully considered. |
e193ed0 to
c21e530
Compare
|
Removed operator-owned resources from the tagging changes |
|
/hold Revision 50e6267 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
|
Pretty sure I missed some of the resources associated with bastion tests (keypairs, SG, and ec2 instance). Might just do it in a followup given this is already tagged and the state of CI right now |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest e2e-aws |
|
/test e2e-aws |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/acknowledge-critical-fixes-only |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
@ironcladlou: 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. |
8fac928
into
openshift:main
|
Now I have all the details. The codecov/patch check explicitly states the target is 43.45% but the patch only achieved 9.37%. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe codecov/patch failure is not a product bug or test infrastructure issue — it is a coverage gap on new code added in areas with no existing unit tests. Why coverage is low: All 7 files flagged by Codecov are CLI commands ( Breakdown of missing lines by file:
The Why the target is 43.45%: Codecov's default behavior when no explicit patch threshold is configured is to use the project's base coverage as the target. The project sits at ~43.45% coverage, so any patch achieving less than that triggers a failure. This is a known challenge for PRs touching CLI/infra code that has near-zero coverage. Recommendations
Evidence
|
The bastion keypair, security group, and EC2 instance were missed in PR openshift#8909. Plumb AdditionalTags through CreateBastionOpts so the e2e journal dump path tags them with source and prow-job-id.
The bastion keypair, security group, and EC2 instance were missed in PR openshift#8909. Plumb AdditionalTags through CreateBastionOpts so the e2e journal dump path tags them with source and prow-job-id.
The bastion keypair, security group, and EC2 instance were missed in PR openshift#8909. Plumb AdditionalTags through CreateBastionOpts so the e2e journal dump path tags them with source and prow-job-id.
4 participants
Add hypershift.openshift.io/infra-id, hypershift.openshift.io/cluster-name,
and hypershift.openshift.io/source tags to AWS resources created by CLI
infrastructure and IAM commands, the bastion tool, the DR OIDC fix tool,
and e2e test helpers. The source tag identifies the creator (cli or e2e).
E2e tests additionally tag resources with hypershift.openshift.io/prow-job-id
when running in Prow CI.
Tag key strings are centralized as constants in support/awsutil/tags.go.
Operator-created resources (CPO, hypershift-operator) are intentionally
excluded to avoid fleet-wide tag quota impact.
Fixes CNTRLPLANE-3763
Summary by CodeRabbit
New Features
Bug Fixes
Tests