AUTOSCALE-551: Add argument for default tag#9046
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@jkyros: This pull request references AUTOSCALE-551 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
do-not-merge/needs-area
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkyros 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 |
📝 WalkthroughWalkthroughAdds an exported annotation constant for overriding the AWS cluster-name tag key. The HostedCluster controller mirrors this annotation to the HostedControlPlane, where the Karpenter deployment receives it as Sequence Diagram(s)sequenceDiagram
participant HostedCluster
participant HostedControlPlane
participant Karpenter
participant AWS_EC2
HostedCluster->>HostedControlPlane: mirror custom tag key annotation
HostedControlPlane->>Karpenter: set CLUSTER_NAME_TAG_KEY
Karpenter->>AWS_EC2: provision node with custom cluster tag
AWS_EC2-->>Karpenter: return provisioned instance
AWS_EC2-->>HostedCluster: expose custom tag with InfraID value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/karpenter/deployment.go (1)
44-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo validation on the custom tag key value.
clusterNameTagKeyis taken straight from the HCP annotation and injected verbatim as the AWS tag key Karpenter will use on managed EC2 instances, with no check against empty values or the reservedhypershift.openshift.io/*tag namespace. Based on learnings, AWS resource tag keys underhypershift.openshift.io/*should be treated as reserved/internal, and user-supplied tag conflicts should be rejected early rather than silently applied.🛡️ Suggested guard
if clusterNameTagKey, exists := hcp.Annotations[hyperkarpenterv1.KarpenterProviderAWSClusterNameTagKey]; exists { + if clusterNameTagKey != "" && !strings.HasPrefix(clusterNameTagKey, "hypershift.openshift.io/") { c.Env = append(c.Env, corev1.EnvVar{ Name: "CLUSTER_NAME_TAG_KEY", Value: clusterNameTagKey, }) + } }🤖 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/v2/karpenter/deployment.go` around lines 44 - 49, Validate clusterNameTagKey before appending the CLUSTER_NAME_TAG_KEY environment variable: reject empty values and any key beginning with the reserved hypershift.openshift.io/ namespace. Propagate an early error through the surrounding deployment/reconciliation path instead of injecting the invalid annotation value.Source: Learnings
🤖 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 `@test/e2e/karpenter_cluster_name_tag_test.go`:
- Around line 45-51: Remove the KarpenterProviderAWSImage annotation override
and its associated temporary TODO from the cluster setup in the test, leaving
only the intended cluster-name tag configuration. Do not reference the personal
quay.io image in CI.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/karpenter/deployment.go`:
- Around line 44-49: Validate clusterNameTagKey before appending the
CLUSTER_NAME_TAG_KEY environment variable: reject empty values and any key
beginning with the reserved hypershift.openshift.io/ namespace. Propagate an
early error through the surrounding deployment/reconciliation path instead of
injecting the invalid annotation value.
🪄 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: 8ad7f995-b540-463c-af4c-98e4e3c17ecb
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/karpenter/v1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
api/karpenter/v1/karpenter_types.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenter/deployment.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gotest/e2e/karpenter_cluster_name_tag_test.go
| customTagKey := "openshift:cluster-name" | ||
| clusterOpts.Annotations = append(clusterOpts.Annotations, | ||
| fmt.Sprintf("%s=%s", hyperkarpenterv1.KarpenterProviderAWSClusterNameTagKey, customTagKey), | ||
| // TODO(jkyros): Tag stuff hasn't merged upstream yet, take this out once it has, we're just testing plumbing for now, | ||
| // this absolutely needs to come out before we merge | ||
| fmt.Sprintf("%s=%s", hyperkarpenterv1.KarpenterProviderAWSImage, "quay.io/jkyros/aws-karpenter-provider-aws:tags"), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Hardcoded personal quay.io image must be removed before merge.
The KarpenterProviderAWSImage override points at a personal fork (quay.io/jkyros/aws-karpenter-provider-aws:tags) to test unreleased upstream tag-key support. The TODO already flags this as unmergeable as-is — an unmaintained, single-user registry image is a supply-chain/availability risk if this lands in CI (image could be deleted, retagged, or become stale).
Do you want me to open a follow-up issue to track removing this once the upstream Karpenter tag-key support merges?
🤖 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/karpenter_cluster_name_tag_test.go` around lines 45 - 51, Remove the
KarpenterProviderAWSImage annotation override and its associated temporary TODO
from the cluster setup in the test, leaving only the intended cluster-name tag
configuration. Do not reference the personal quay.io image in CI.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9046 +/- ##
==========================================
+ Coverage 43.50% 44.38% +0.87%
==========================================
Files 771 781 +10
Lines 95722 98709 +2987
==========================================
+ Hits 41648 43816 +2168
- Misses 51174 51827 +653
- Partials 2900 3066 +166
... and 95 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:
|
6881351 to
a44501e
Compare
Upstream is potentially adding plumbing for allowing changing the default eks: tag, so this just attempts to consume it via hypershift for ergonomic feel/smoke test reasons. It seems like it works. Signed-off-by: John Kyros <jkyros@redhat.com>
a44501e to
7d3fed2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hostedcontrolplane/v2/karpenter/component_test.go`:
- Around line 118-119: Update the table-driven test case name in the Karpenter
component tests to follow the required “When ... it should ...” format,
capitalizing the opening word and polishing the wording while preserving the
described annotation and CLUSTER_NAME_TAG_KEY behavior.
🪄 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: 3c0b3a1d-83dc-4c67-8da4-16e3513401d3
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/karpenter/v1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (6)
api/.golangci.ymlapi/karpenter/v1/karpenter_types.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenter/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/karpenter/deployment.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gotest/e2e/karpenter_cluster_name_tag_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- control-plane-operator/controllers/hostedcontrolplane/v2/karpenter/deployment.go
- api/karpenter/v1/karpenter_types.go
- test/e2e/karpenter_cluster_name_tag_test.go
| { | ||
| name: "when HCP has KarpenterProviderAWSClusterNameTagKey annotation, it should set CLUSTER_NAME_TAG_KEY env var", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required When ... it should ... test-case format.
Capitalize and slightly polish the new table-driven test description to match the repository convention.
Proposed fix
- name: "when HCP has KarpenterProviderAWSClusterNameTagKey annotation, it should set CLUSTER_NAME_TAG_KEY env var",
+ name: "When the HCP has the KarpenterProviderAWSClusterNameTagKey annotation, it should set the CLUSTER_NAME_TAG_KEY env var",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "when HCP has KarpenterProviderAWSClusterNameTagKey annotation, it should set CLUSTER_NAME_TAG_KEY env var", | |
| name: "When the HCP has the KarpenterProviderAWSClusterNameTagKey annotation, it should set the CLUSTER_NAME_TAG_KEY env var", |
🤖 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/v2/karpenter/component_test.go`
around lines 118 - 119, Update the table-driven test case name in the Karpenter
component tests to follow the required “When ... it should ...” format,
capitalizing the opening word and polishing the wording while preserving the
described annotation and CLUSTER_NAME_TAG_KEY behavior.
Source: Coding guidelines
2 participants
What this PR does / why we need it:
Ignore this for now, upstream is potentially adding plumbing for allowing changing the default eks: tag in aws/karpenter-provider-aws#9252, this just attempts to consume it via hypershift for ergonomic feel/smoke test reasons. Seems like it works 😄
Which issue(s) this PR fixes:
Fixes AUTOSCALE-551
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
CLUSTER_NAME_TAG_KEY.