NO-JIRA: api: fix AutoNode types to comply with OpenShift API conventions#7859
Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom Mar 10, 2026
Merged
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
|
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
|
@enxebre: This pull request explicitly references no jira issue. 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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
Contributor
|
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:
WalkthroughAutoNode.Provisioner changed from a pointer to a value and new provisioner-related types (Provisioner, ProvisionerConfig, KarpenterConfig, KarpenterAWSConfig) and validations were added. CRD OpenAPI descriptions and x-kubernetes-validations expanded; initializers/tests updated and a nil-check in Karpenter support code was removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
openshift-ci
bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/cluster/aws/create.go (1)
275-283:⚠️ Potential issue | 🟠 MajorFail fast when
--auto-nodeis enabled but Karpenter role ARN is empty.This path can build an invalid
AutoNodespec (roleARNis now required/pattern-validated) and fail later during API submission. Add an early guard for clearer CLI UX.🛠️ Proposed fix
if o.AutoNode { + if o.iamInfo.KarpenterRoleARN == "" { + return fmt.Errorf("auto-node requires a non-empty Karpenter role ARN") + } cluster.Spec.AutoNode = &hyperv1.AutoNode{ Provisioner: hyperv1.ProvisionerConfig{ Name: hyperv1.ProvisionerKarpenter, Karpenter: &hyperv1.KarpenterConfig{ Platform: hyperv1.AWSPlatform, AWS: &hyperv1.KarpenterAWSConfig{ RoleARN: o.iamInfo.KarpenterRoleARN, }, }, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster/aws/create.go` around lines 275 - 283, When o.AutoNode is true, add a guard that checks o.iamInfo.KarpenterRoleARN and fail fast with a clear CLI error if it's empty; specifically before assigning cluster.Spec.AutoNode set: if o.AutoNode && o.iamInfo.KarpenterRoleARN == "" return fmt.Errorf("--auto-node requires a Karpenter role ARN") (or use the command's error flow), so you don't build cluster.Spec.AutoNode with an empty hyperv1.KarpenterAWSConfig.RoleARN; reference o.AutoNode, o.iamInfo.KarpenterRoleARN, and the cluster.Spec.AutoNode / hyperv1.KarpenterAWSConfig.RoleARN assignment to locate where to insert the check.
🧹 Nitpick comments (2)
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml (1)
116-119: Fix minor docs typo inautoNodedescription.Line 118 is missing a space in
provisioner(e.g. Karpenter).✏️ Suggested wording fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g. Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml` around lines 116 - 119, Fix the minor typo in the autoNode description: add a space between "provisioner" and the parenthesis so the phrase reads "provisioner (e.g. Karpenter)"; update the multi-line description under the autoNode property in the CRD manifest (the block containing "autoNode specifies the configuration..." and the text "provisioner(e.g. Karpenter)") to use "provisioner (e.g. Karpenter)".docs/content/reference/aggregated-docs.md (1)
31396-31398: Avoid nested<p>tags in generated docs output.These sections render as
<p><p>...</p></p>, which is invalid HTML structure and can cause inconsistent rendering across doc tooling. Please adjust the doc generation template to emit a single paragraph wrapper.Also applies to: 37611-37613, 37870-37873, 41733-41735, 41753-41756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 31396 - 31398, The generated docs are emitting nested paragraph wrappers (e.g., "<p><p>AutoNode..."), causing invalid HTML; update the documentation generation template/macro that wraps sections (the code that produces paragraph wrappers around section content) to emit only a single <p> wrapper — detect if the section content already starts/ends with <p> and avoid adding another, or strip inner <p> tags before wrapping; target the template/macro responsible for rendering section bodies (the place producing "<p><p>") so fixes apply to the "AutoNode" section and the other affected ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 1645-1646: The kubebuilder marker for the RoleARN field uses an
invalid "Pattern:=" syntax so the regex isn't applied; update the annotation on
the RoleARN declaration in hostedcluster_types.go to use the correct marker form
"+kubebuilder:validation:Pattern=`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$`"
(replace ":=" with "=") and keep the existing MaxLength marker as-is so the CRD
generator will pick up the ARN regex validation.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 114-120: Update the AutoNode description string so the phrase
"provisioner(e.g. Karpenter)" includes a space before the parenthesis; locate
the autoNode description block (property name "autoNode" / description text) and
change "provisioner(e.g. Karpenter)" to "provisioner (e.g. Karpenter)" to fix
the spacing.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 116-118: The autoNode field description contains a typo/spacing
issue around "provisioner(e.g." — update the autoNode description text (the
description for the autoNode field) to include a space and proper punctuation,
e.g. change "provisioner(e.g. Karpenter)" to "provisioner (e.g., Karpenter)" so
generated API docs use correct spacing and punctuation.
- Line 213: The roleARN pattern currently allows invalid role suffixes; update
the YAML pattern for the roleARN field to a stricter regex that enforces AWS IAM
role name characters, allows optional path segments, disallows spaces and
trailing slashes, and requires a non-empty final segment. Replace the existing
pattern value with a regex such as
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9_+=,.@-]+\/)*[A-Za-z0-9_+=,.@-]+$
so the schema rejects empty/whitespace-only suffixes, trailing slashes, and
invalid characters while still permitting valid path-qualified names (e.g.,
service-role/MyRole).
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 212-213: The Role ARN regex in the generated CRD manifests
currently uses `role\/.*$`, which allows an empty role name; update the
generation logic that emits the `pattern:
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` entry (seen in the
hostedclusters-Hypershift-*.crd.yaml and
hostedcontrolplanes-Hypershift-*.crd.yaml outputs) to use `role\/.+$` instead so
the pattern becomes `^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$`;
locate the template or generator function that writes the `pattern` field for
Role ARNs and change the `.*` to `.+` so generated CRDs no longer accept empty
role names.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 103-183: The roleARN schema uses a regex that allows an empty role
name ("role/.*") — update the pattern for the roleARN fields to require at least
one character after "role/" by replacing
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ with
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$ in all three occurrences;
locate the roleARN entries in the CRD (look for the description block
referencing roleARN and the schema keys named roleARN) and update the pattern
string there so validation rejects ARNs ending with "role/".
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Line 183: The roleARN regex currently allows an empty role name because it
ends with "role\/" and no required characters after the slash; update the CRD
schema entry for roleARN (the pattern value currently shown) to require at least
one allowed role-name character after the slash by replacing the trailing part
with a character class that includes letters, digits and the allowed symbols
(such as + = , . @ _ -) and a "+" quantifier so the role name must be one or
more characters.
In `@docs/content/reference/aggregated-docs.md`:
- Around line 29066-29067: Update the prose for the autoNode description to fix
punctuation and spacing: replace occurrences of "provisioner(e.g. Karpenter)"
with "provisioner (e.g., Karpenter)" so there is a space before the parenthesis
and a comma after "e.g."; check both occurrences referenced in the diff and
update the text associated with the autoNode description string accordingly.
- Around line 37630-37860: The IAM JSON examples under the roleARN section (the
OIDC trust example and the large policy document shown as JSON) are being
rendered as paragraph text and getting corrupted (e.g., ARN wildcards turned
into <em>), so wrap each JSON example in a literal code block (fenced
triple-backticks or <pre><code>) and escape any angle-bracket placeholders
(e.g., <oidc-provider-arn>, <oidc-provider-name>, and ARN wildcards) as
<...> so the JSON remains literal and copy-pastable; update the trust
relationship example and the policy document example blocks accordingly.
In `@docs/content/reference/api.md`:
- Around line 9346-9347: Update the doc text for KarpenterConfig.platform to
state that it currently only supports AWS (instead of the platform-agnostic
sentence). Locate the description for the platform field
(KarpenterConfig.platform) and change the paragraph to explicitly note
"Currently only AWS is supported" and, if helpful, reference that validation
constrains the field to AWS so readers aren't misled.
- Around line 520-521: Replace the missing space before the parenthesis in the
example text: find occurrences of the string "provisioner(e.g. Karpenter)" in
the docs content (e.g., within the autoNode description and the other occurrence
later) and update them to "provisioner (e.g. Karpenter)" so both instances
(around line 521 and the later occurrence near lines 7239-7240) are corrected
for readability.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 4642-4644: The test fixture uses an invalid union discriminator by
setting ProvisionerConfig.Name to "test-provisioner"; update the fixture in
hostedcluster_controller_test.go so the ProvisionerConfig uses the enum
discriminator for the Karpenter union (set ProvisionerConfig.Name to "Karpenter"
or the corresponding constant) when providing a KarpenterConfig, ensuring the
object is valid against hyperv1.ProvisionerConfig and will exercise
Karpenter-specific behavior.
---
Outside diff comments:
In `@cmd/cluster/aws/create.go`:
- Around line 275-283: When o.AutoNode is true, add a guard that checks
o.iamInfo.KarpenterRoleARN and fail fast with a clear CLI error if it's empty;
specifically before assigning cluster.Spec.AutoNode set: if o.AutoNode &&
o.iamInfo.KarpenterRoleARN == "" return fmt.Errorf("--auto-node requires a
Karpenter role ARN") (or use the command's error flow), so you don't build
cluster.Spec.AutoNode with an empty hyperv1.KarpenterAWSConfig.RoleARN;
reference o.AutoNode, o.iamInfo.KarpenterRoleARN, and the cluster.Spec.AutoNode
/ hyperv1.KarpenterAWSConfig.RoleARN assignment to locate where to insert the
check.
---
Nitpick comments:
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 116-119: Fix the minor typo in the autoNode description: add a
space between "provisioner" and the parenthesis so the phrase reads "provisioner
(e.g. Karpenter)"; update the multi-line description under the autoNode property
in the CRD manifest (the block containing "autoNode specifies the
configuration..." and the text "provisioner(e.g. Karpenter)") to use
"provisioner (e.g. Karpenter)".
In `@docs/content/reference/aggregated-docs.md`:
- Around line 31396-31398: The generated docs are emitting nested paragraph
wrappers (e.g., "<p><p>AutoNode..."), causing invalid HTML; update the
documentation generation template/macro that wraps sections (the code that
produces paragraph wrappers around section content) to emit only a single <p>
wrapper — detect if the section content already starts/ends with <p> and avoid
adding another, or strip inner <p> tags before wrapping; target the
template/macro responsible for rendering section bodies (the place producing
"<p><p>") so fixes apply to the "AutoNode" section and the other affected
ranges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 624cf0b2-d59c-4750-a7f5-6eba94c03be2
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (15)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/aws/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.gosupport/karpenter/karpenter.go
Comment on lines
+116
to
+118
| description: |- | ||
| autoNode specifies the configuration for automatic node provisioning and lifecycle management. | ||
| When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. |
Contributor
There was a problem hiding this comment.
Fix the autoNode description typo.
Line 118 has a small docs typo (provisioner(e.g.), which propagates to generated API docs.
Suggested text fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads.
+ When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`
around lines 116 - 118, The autoNode field description contains a typo/spacing
issue around "provisioner(e.g." — update the autoNode description text (the
description for the autoNode field) to include a space and proper punctuation,
e.g. change "provisioner(e.g. Karpenter)" to "provisioner (e.g., Karpenter)" so
generated API docs use correct spacing and punctuation.
| \"Allow\",\n\t\t\t\"Resource\": \"*\",\n\t\t\t\"Action\": | ||
| \"iam:ListInstanceProfiles\"\n\t\t}\n\t]\n}" | ||
| maxLength: 2048 | ||
| pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and display the relevant file to confirm line 213
fd "hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml" --type fRepository: openshift/hypershift
Length of output: 180
🏁 Script executed:
# Once we know the file exists, let's examine line 213 and surrounding context
if [ -f "cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml" ]; then
sed -n '210,220p' "cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml"
fiRepository: openshift/hypershift
Length of output: 735
🏁 Script executed:
# Run the verification script from the review comment
python - <<'PY'
import re
current = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$')
suggested = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/[A-Za-z0-9+=,.@_-](?:[A-Za-z0-9+=,.@_\/-]*[A-Za-z0-9+=,.@_-])?$')
cases = {
"valid_role": "arn:aws:iam::123456789012:role/MyRole",
"invalid_empty_suffix": "arn:aws:iam::123456789012:role/",
"invalid_whitespace_suffix": "arn:aws:iam::123456789012:role/ ",
"aws_service_role": "arn:aws:iam::123456789012:role/aws-service-role/ec2.amazonaws.com/AWSServiceRoleForEC2",
"valid_with_slash": "arn:aws:iam::123456789012:role/service-role/MyRole",
"valid_with_hyphens": "arn:aws:iam::123456789012:role/my-test-role",
"valid_with_underscores": "arn:aws:iam::123456789012:role/my_test_role",
"invalid_space_in_middle": "arn:aws:iam::123456789012:role/my role",
"invalid_double_slash": "arn:aws:iam::123456789012:role//MyRole",
"valid_with_plus": "arn:aws:iam::123456789012:role/MyRole+Test",
}
print("=== Current Pattern ===")
for name, value in cases.items():
result = bool(current.fullmatch(value))
print(f"{name:30s}: {result}")
print("\n=== Suggested Pattern ===")
for name, value in cases.items():
result = bool(suggested.fullmatch(value))
print(f"{name:30s}: {result}")
PYRepository: openshift/hypershift
Length of output: 858
🌐 Web query:
AWS IAM role naming convention rules allowed characters
💡 Result:
AWS IAM role name rules (the RoleName in CreateRole):
- Length: 1–64 characters [1]
- Allowed characters: upper/lowercase letters and digits (no spaces), plus
_ + = , . @ -[1] - Validation regex:
[\w+=,.@-]+[1] - Uniqueness: must be unique in the account; case-insensitive (e.g.,
Role1androle1conflict) [1][2]
Sources: [1] AWS IAM API Reference – CreateRole (RoleName) [2] AWS CloudFormation AWS::IAM::Role (RoleName)
Tighten roleARN pattern to reject invalid role suffixes.
The current pattern role\/.* permits empty, whitespace-only, and space-containing role segments, admitting invalid ARNs at the schema level. The suggested regex enforces AWS IAM role naming rules (allowed characters: A-Z, a-z, 0-9, _ + = , . @ -, with no spaces) and correctly rejects edge cases like trailing slashes, empty suffixes, and spaces while accepting valid role paths like service-role/MyRole.
Suggested regex hardening
- pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$
+ pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/[A-Za-z0-9+=,.@_-](?:[A-Za-z0-9+=,.@_\/-]*[A-Za-z0-9+=,.@_-])?$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`
at line 213, The roleARN pattern currently allows invalid role suffixes; update
the YAML pattern for the roleARN field to a stricter regex that enforces AWS IAM
role name characters, allows optional path segments, disallows spaces and
trailing slashes, and requires a non-empty final segment. Replace the existing
pattern value with a regex such as
^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9_+=,.@-]+\/)*[A-Za-z0-9_+=,.@-]+$
so the schema rejects empty/whitespace-only suffixes, trailing slashes, and
invalid characters while still permitting valid path-qualified names (e.g.,
service-role/MyRole).
Comment on lines
+37630
to
+37860
| <p>roleARN specifies the ARN of the IAM role that Karpenter assumes to provision | ||
| and manage EC2 instances in the hosted cluster’s AWS account.</p> | ||
| <p>The referenced role must have a trust relationship that allows it to be assumed | ||
| by the karpenter service account in the hosted cluster via OIDC. | ||
| Example: | ||
| { | ||
| “Version”: “2012-10-17”, | ||
| “Statement”: [ | ||
| { | ||
| “Effect”: “Allow”, | ||
| “Principal”: { | ||
| “Federated”: “<oidc-provider-arn>” | ||
| }, | ||
| “Action”: “sts:AssumeRoleWithWebIdentity”, | ||
| “Condition”: { | ||
| “StringEquals”: { | ||
| “<oidc-provider-name>:sub”: “system:serviceaccount:kube-system:karpenter” | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }</p> | ||
| <p>The following is an example of the policy document for this role.</p> | ||
| <p>{ | ||
| “Version”: “2012-10-17”, | ||
| “Statement”: [ | ||
| { | ||
| “Sid”: “AllowScopedEC2InstanceAccessActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: [ | ||
| “arn:<em>:ec2:</em>::image/<em>”, | ||
| “arn:</em>:ec2:<em>::snapshot/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:security-group/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:subnet/</em>” | ||
| ], | ||
| “Action”: [ | ||
| “ec2:RunInstances”, | ||
| “ec2:CreateFleet” | ||
| ] | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedEC2LaunchTemplateAccessActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:<em>:ec2:</em>:<em>:launch-template/</em>”, | ||
| “Action”: [ | ||
| “ec2:RunInstances”, | ||
| “ec2:CreateFleet” | ||
| ] | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedEC2InstanceActionsWithTags”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: [ | ||
| “arn:<em>:ec2:</em>:<em>:fleet/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:instance/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:volume/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:network-interface/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:launch-template/</em>”, | ||
| “arn:<em>:ec2:</em>:<em>:spot-instances-request/</em>” | ||
| ], | ||
| “Action”: [ | ||
| “ec2:RunInstances”, | ||
| “ec2:CreateFleet”, | ||
| “ec2:CreateLaunchTemplate” | ||
| ], | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:RequestTag/karpenter.sh/nodepool”: “<em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedResourceCreationTagging”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: [ | ||
| “arn:</em>:ec2:<em>:</em>:fleet/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:instance/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:volume/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:network-interface/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:launch-template/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:spot-instances-request/<em>” | ||
| ], | ||
| “Action”: “ec2:CreateTags”, | ||
| “Condition”: { | ||
| “StringEquals”: { | ||
| “ec2:CreateAction”: [ | ||
| “RunInstances”, | ||
| “CreateFleet”, | ||
| “CreateLaunchTemplate” | ||
| ] | ||
| }, | ||
| “StringLike”: { | ||
| “aws:RequestTag/karpenter.sh/nodepool”: “</em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedResourceTagging”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:<em>:ec2:</em>:<em>:instance/</em>”, | ||
| “Action”: “ec2:CreateTags”, | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:ResourceTag/karpenter.sh/nodepool”: “<em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedDeletion”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: [ | ||
| “arn:</em>:ec2:<em>:</em>:instance/<em>”, | ||
| “arn:</em>:ec2:<em>:</em>:launch-template/<em>” | ||
| ], | ||
| “Action”: [ | ||
| “ec2:TerminateInstances”, | ||
| “ec2:DeleteLaunchTemplate” | ||
| ], | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:ResourceTag/karpenter.sh/nodepool”: “</em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowRegionalReadActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “<em>”, | ||
| “Action”: [ | ||
| “ec2:DescribeImages”, | ||
| “ec2:DescribeInstances”, | ||
| “ec2:DescribeInstanceTypeOfferings”, | ||
| “ec2:DescribeInstanceTypes”, | ||
| “ec2:DescribeLaunchTemplates”, | ||
| “ec2:DescribeSecurityGroups”, | ||
| “ec2:DescribeSpotPriceHistory”, | ||
| “ec2:DescribeSubnets” | ||
| ] | ||
| }, | ||
| { | ||
| “Sid”: “AllowSSMReadActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:</em>:ssm:<em>::parameter/aws/service/</em>”, | ||
| “Action”: “ssm:GetParameter” | ||
| }, | ||
| { | ||
| “Sid”: “AllowPricingReadActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “<em>”, | ||
| “Action”: “pricing:GetProducts” | ||
| }, | ||
| { | ||
| “Sid”: “AllowInterruptionQueueActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “</em>”, | ||
| “Action”: [ | ||
| “sqs:DeleteMessage”, | ||
| “sqs:GetQueueUrl”, | ||
| “sqs:ReceiveMessage” | ||
| ] | ||
| }, | ||
| { | ||
| “Sid”: “AllowPassingInstanceRole”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:<em>:iam::</em>:role/<em>”, | ||
| “Action”: “iam:PassRole”, | ||
| “Condition”: { | ||
| “StringEquals”: { | ||
| “iam:PassedToService”: [ | ||
| “ec2.amazonaws.com”, | ||
| “ec2.amazonaws.com.cn” | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedInstanceProfileCreationActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:</em>:iam::<em>:instance-profile/</em>”, | ||
| “Action”: [ | ||
| “iam:CreateInstanceProfile” | ||
| ], | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:RequestTag/karpenter.k8s.aws/ec2nodeclass”: “<em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedInstanceProfileTagActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:</em>:iam::<em>:instance-profile/</em>”, | ||
| “Action”: [ | ||
| “iam:TagInstanceProfile” | ||
| ], | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:ResourceTag/karpenter.k8s.aws/ec2nodeclass”: “<em>”, | ||
| “aws:RequestTag/karpenter.k8s.aws/ec2nodeclass”: “</em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowScopedInstanceProfileActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:<em>:iam::</em>:instance-profile/<em>”, | ||
| “Action”: [ | ||
| “iam:AddRoleToInstanceProfile”, | ||
| “iam:RemoveRoleFromInstanceProfile”, | ||
| “iam:DeleteInstanceProfile” | ||
| ], | ||
| “Condition”: { | ||
| “StringLike”: { | ||
| “aws:ResourceTag/karpenter.k8s.aws/ec2nodeclass”: “</em>” | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| “Sid”: “AllowInstanceProfileReadActions”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “arn:<em>:iam::</em>:instance-profile/<em>”, | ||
| “Action”: “iam:GetInstanceProfile” | ||
| }, | ||
| { | ||
| “Sid”: “AllowUnscopedInstanceProfileListAction”, | ||
| “Effect”: “Allow”, | ||
| “Resource”: “</em>”, | ||
| “Action”: “iam:ListInstanceProfiles” | ||
| } | ||
| ] | ||
| }</p> |
Contributor
There was a problem hiding this comment.
Move IAM JSON examples to fenced code blocks to prevent markup corruption.
Line 37630 through Line 37860 render policy JSON inside <p> text, and ARN wildcards are being transformed into <em> markup (for example, arn:<em>:ec2...). That makes examples unsafe to copy and can lead to invalid IAM policies.
Use fenced code blocks (or <pre><code>) and escape placeholders (<...>) so examples remain literal.
💡 Proposed documentation fix
-<p>The following is an example of the policy document for this role.</p>
-<p>{
- "Version": "2012-10-17",
- ...
-}</p>
+<p>The following is an example policy document for this role.</p>
+<pre><code>{
+ "Version": "2012-10-17",
+ "Statement": [
+ {
+ "Sid": "AllowRegionalReadActions",
+ "Effect": "Allow",
+ "Resource": "*",
+ "Action": [
+ "ec2:DescribeImages",
+ "ec2:DescribeInstances"
+ ]
+ }
+ ]
+}</code></pre>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/reference/aggregated-docs.md` around lines 37630 - 37860, The
IAM JSON examples under the roleARN section (the OIDC trust example and the
large policy document shown as JSON) are being rendered as paragraph text and
getting corrupted (e.g., ARN wildcards turned into <em>), so wrap each JSON
example in a literal code block (fenced triple-backticks or <pre><code>) and
escape any angle-bracket placeholders (e.g., <oidc-provider-arn>,
<oidc-provider-name>, and ARN wildcards) as <...> so the JSON remains
literal and copy-pastable; update the trust relationship example and the policy
document example blocks accordingly.
Comment on lines
+9346
to
9347
| <p>platform specifies the infrastructure platform that Karpenter should provision nodes on.</p> | ||
| </td> |
Contributor
There was a problem hiding this comment.
Clarify AWS-only support for KarpenterConfig.platform.
The description is currently platform-agnostic, but this field is constrained to AWS by validation. Please explicitly say it currently supports only AWS to avoid misleading users (Line 9346).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/reference/api.md` around lines 9346 - 9347, Update the doc text
for KarpenterConfig.platform to state that it currently only supports AWS
(instead of the platform-agnostic sentence). Locate the description for the
platform field (KarpenterConfig.platform) and change the paragraph to explicitly
note "Currently only AWS is supported" and, if helpful, reference that
validation constrains the field to AWS so readers aren't misled.
JoelSpeed
reviewed
Mar 5, 2026
| // provisionerConfig specifies the provisioner used for automatic node management. | ||
| // +required | ||
| Provisioner *ProvisionerConfig `json:"provisionerConfig"` | ||
| Provisioner ProvisionerConfig `json:"provisionerConfig"` |
Contributor
There was a problem hiding this comment.
How long have these types existed? Who is this likely to break?
| // provisionerConfig specifies the provisioner used for automatic node management. | ||
| // +required | ||
| Provisioner *ProvisionerConfig `json:"provisionerConfig"` | ||
| Provisioner ProvisionerConfig `json:"provisionerConfig"` |
Contributor
There was a problem hiding this comment.
To meet current conventions, all fields should have omitempty or omitzero for structs
Contributor
There was a problem hiding this comment.
struct is required with 1 required field, omitzero is never used. Why do we need to add it? It's a bit confusing
Contributor
There was a problem hiding this comment.
It is used. Imagine a client builds a structured object and doesn't populate anything within here. With omitzero the key provisionerConfig is not marshalled, and they will get provisioner is required within the error
If you didn't have omitzero, they would marshal provisioner: {} and potentially get other errors, or just mess
We updated the API conventions last year to try and bring the behaviour of structured clients and unstructured clients closer together and this is an important part of that alignment
| // | ||
| // +required | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:Pattern:=`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` |
Contributor
There was a problem hiding this comment.
We prefer to use CEL to implement patterns like this as you can then customise the error message to be helpful, with this it'll just dump the regex which is less user friendly generally
| // +required | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:Pattern:=`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` | ||
| // +kubebuilder:validation:MaxLength=2048 |
Contributor
There was a problem hiding this comment.
How did you come up with the extended maximum length here?
Member
Author
There was a problem hiding this comment.
I was just being conservative
Contributor
There was a problem hiding this comment.
You can probably keep this tighter and relax the validation if you find issues later. I would be looking to see if there is a length limit imposed by AWS on the variable part (role name?) and then the rest we can calculate the length of to come up with a suitable max length
Member
Author
There was a problem hiding this comment.
It seems 2048 is actually accurate https://docs.aws.amazon.com/IAM/latest/APIReference/API_Role.html#API_Role_Contents.
enxebre
force-pushed
the
fix-autonode-api-conventions
branch
from
467cb68 to
cdab6ad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
4642-4644:⚠️ Potential issue | 🟡 MinorUse a valid union discriminator for the Karpenter fixture.
Line 4643 sets
ProvisionerConfig.Nameto"test-provisioner", but this discriminator is enum-constrained toKarpenterfor theKarpenterunion member at Line 4644. This makes the fixture invalid against the API contract and can mask Karpenter-specific behavior.Suggested fix
AutoNode: &hyperv1.AutoNode{ Provisioner: hyperv1.ProvisionerConfig{ - Name: "test-provisioner", + Name: "Karpenter", Karpenter: &hyperv1.KarpenterConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 4642 - 4644, The test fixture is using an invalid union discriminator: set ProvisionerConfig.Name to the Karpenter discriminator value instead of "test-provisioner" so the Karpenter union member is valid; locate the Provisioner: hyperv1.ProvisionerConfig block (and its nested Karpenter: &hyperv1.KarpenterConfig) and replace the Name field with the correct discriminator (e.g., "Karpenter" or the corresponding constant) so the fixture matches the API's enum constraint for the Karpenter union member.docs/content/reference/api.md (2)
520-521:⚠️ Potential issue | 🟡 MinorFix repeated typo in provisioner example formatting.
provisioner(e.g. Karpenter)is missing a space before the parenthesis in both places. Update toprovisioner (e.g. Karpenter)for readability and consistency.Also applies to: 7238-7239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 520 - 521, Fix the repeated formatting typo where the phrase "provisioner(e.g. Karpenter)" is missing a space before the parenthesis; update all occurrences of the exact token "provisioner(e.g. Karpenter)" in the document (including the other occurrence with the same token) to "provisioner (e.g. Karpenter)" to restore consistent spacing and readability.
9345-9346:⚠️ Potential issue | 🟡 MinorClarify that
KarpenterConfig.platformis currently AWS-only.The description is platform-agnostic, but this field is constrained to
AWS. Please state that explicitly to avoid misleading API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 9345 - 9346, Update the documentation for the platform field to explicitly state that KarpenterConfig.platform currently supports only AWS; locate the description for the platform field (referenced as KarpenterConfig.platform / "platform specifies the infrastructure platform") and change the text to mention "AWS-only" (or equivalent phrasing) and, if helpful, add a short note about future platform support being planned or to check compatibility docs.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml (1)
212-213:⚠️ Potential issue | 🟡 MinorTighten
roleARNvalidation to reject empty role names.On Line 213,
role\/.*$also matches...:role/(empty role name). Please require at least one character afterrole/.🔧 Proposed fix
- pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$Since this is generated output, please apply the fix in the source type/generator path to avoid regressions on regen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml` around lines 212 - 213, The regex for roleARN currently allows an empty role name because it uses "role\/.*$"; update the schema/generator that emits the roleARN pattern (the CRD field named roleARN in the HostedCluster TechPreviewNoUpgrade type) to require at least one character after "role/", e.g. change "role\/.*$" to "role\/.+$" (or equivalent) in the generator/source type so regenerated CRD will reject ARNs ending with "role/"; ensure you modify the source type or generator template rather than the generated YAML to prevent regressions on regen.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml (1)
183-183:⚠️ Potential issue | 🟡 Minor
roleARNregex still accepts empty role names (...:role/).This still permits invalid ARNs and should require at least one character after
role/. This same fix should be applied to all three occurrences in this file (Line 183, Line 4946, Line 4967).🔧 Proposed fix
- pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$ ... - pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$ ... - pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$#!/bin/bash set -euo pipefail file="cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml" # Verify all occurrences still use the permissive pattern rg -n 'pattern: \^arn:\(aws\|aws-cn\|aws-us-gov\):iam::\[0-9\]\{12\}:role\\/\.\*\$' "$file" # Verify behavior: current regex accepts empty role name. python3 - <<'PY' import re pat = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.*$') tests = [ "arn:aws:iam::123456789012:role/", "arn:aws:iam::123456789012:role/my-role", ] for t in tests: print(t, "=>", bool(pat.fullmatch(t))) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml` at line 183, The roleARN regex currently allows empty role names because it uses ".*"; update the three pattern entries that match role ARNs in hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml to require at least one character after "role/" by replacing "role\/.*$" with "role\/.+$" (i.e., change the pattern to ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$) for each occurrence referenced (the three roleARN pattern lines).cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml (2)
116-118:⚠️ Potential issue | 🟡 MinorFix the
autoNodedescription punctuation typo.Line 118 still has
provisioner(e.g. Karpenter). Please add spacing/punctuation for generated docs quality.Suggested doc fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 116 - 118, Update the autoNode description text in the CRD manifest to fix punctuation around the example: change "provisioner(e.g. Karpenter)" to "provisioner (e.g., Karpenter)" in the description for autoNode so the generated docs include proper spacing and the comma after "e.g.".
213-213:⚠️ Potential issue | 🟡 MinorHarden
roleARNpattern to reject invalid role suffixes.Line 213 currently accepts empty/whitespace role suffixes (
role\/.*). Tighten this to enforce non-empty IAM role path/name segments and disallow spaces/trailing slash.Suggested regex hardening
- pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$ + pattern: ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9_+=,.@-]+\/)*[A-Za-z0-9_+=,.@-]+$#!/bin/bash set -euo pipefail FILE="cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml" sed -n '210,216p' "$FILE" python - <<'PY' import re current = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$') proposed = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9_+=,.@-]+\/)*[A-Za-z0-9_+=,.@-]+$') samples = { "valid_basic": "arn:aws:iam::123456789012:role/MyRole", "valid_path": "arn:aws:iam::123456789012:role/service-role/MyRole", "invalid_empty_suffix": "arn:aws:iam::123456789012:role/", "invalid_space_only": "arn:aws:iam::123456789012:role/ ", "invalid_space_in_name": "arn:aws:iam::123456789012:role/my role", "invalid_trailing_slash": "arn:aws:iam::123456789012:role/path/" } print("=== current ===") for k, v in samples.items(): print(f"{k:24} {bool(current.fullmatch(v))}") print("\n=== proposed ===") for k, v in samples.items(): print(f"{k:24} {bool(proposed.fullmatch(v))}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` at line 213, The roleARN schema pattern currently allows empty or whitespace-only suffixes; update the pattern value for the roleARN field (the YAML "pattern:" under the roleARN schema) to a stricter regular expression that requires one or more non-empty path/name segments, disallows trailing slashes and spaces, permits only valid IAM name characters (letters, digits and the characters _ += , . @ -) in each segment, allows optional intermediate path segments separated by '/', and ensures the final segment is non-empty; replace the existing loose "role\/.*" pattern with this tightened pattern.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
114-116:⚠️ Potential issue | 🟡 MinorFix the remaining spacing typo in the AutoNode description.
Line 116 still reads
provisioner(e.g. Karpenter)and should include a space before(.✏️ Proposed fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g. Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 114 - 116, The AutoNode description string still contains the typo "provisioner(e.g. Karpenter)"; update the autoNode description so it reads "provisioner (e.g. Karpenter)" (i.e., insert a single space before the opening parenthesis). Locate the AutoNode/autoNode description block in the generated CRD manifest and modify that literal description value accordingly to preserve formatting and newlines.docs/content/reference/aggregated-docs.md (2)
29066-29067:⚠️ Potential issue | 🟡 MinorFix unresolved punctuation/spacing in provisioner text.
Line 29067 and Line 35785 still use
provisioner(e.g. Karpenter). Please change toprovisioner (e.g., Karpenter)for readability consistency.Also applies to: 35784-35785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 29066 - 29067, Update the phrasing in the autoNode documentation to fix spacing and punctuation: replace occurrences of "provisioner(e.g. Karpenter)" with "provisioner (e.g., Karpenter)" in the aggregated docs (look for the autoNode paragraph and any other instances referenced by the reviewer), ensuring the space before the parenthesis and the comma after "e." are added for consistency.
37629-37859:⚠️ Potential issue | 🟠 MajorMove IAM JSON examples to literal code blocks to prevent corruption.
Line 37629 onward still embeds JSON in paragraph markup; ARN wildcard and placeholder content is rendered with
<em>artifacts, making examples unsafe to copy/paste. Please wrap both JSON examples in fenced/preformatted code blocks and escape placeholders as literals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 37629 - 37859, The embedded JSON examples (the OIDC trust relationship example beginning with the { "Version": "2012-10-17", "Statement": ... } trust block and the subsequent policy document starting with { "Version": "2012-10-17", "Statement": [ ... ] }) are placed inside paragraph markup and getting corrupted by <em> rendering; replace those paragraph-wrapped JSON blobs with fenced/preformatted code blocks (triple backticks) so the JSON is preserved verbatim, and ensure placeholders/ARNs are either HTML-escaped (e.g., <oidc-provider-arn>) or left raw inside the code fences so no <em> tags are injected.api/hypershift/v1beta1/hostedcluster_types.go (1)
1645-1646:⚠️ Potential issue | 🔴 CriticalFix invalid kubebuilder marker syntax on
RoleARNpattern.Line 1645 uses
Pattern:=, which is not valid marker syntax; this can prevent the regex validation from being applied in generated schemas.Proposed fix
- // +kubebuilder:validation:Pattern:=`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` + // +kubebuilder:validation:Pattern=`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$`#!/bin/bash set -euo pipefail echo "Check for invalid kubebuilder Pattern markers using ':='" rg -n --type go '\+kubebuilder:validation:Pattern:=' api/hypershift/v1beta1/hostedcluster_types.go echo echo "Check corrected Pattern marker exists for RoleARN" rg -n --type go '\+kubebuilder:validation:Pattern=`\^arn:\(aws\|aws-cn\|aws-us-gov\):iam::\[0-9\]\{12\}:role\\/.\*\$`' api/hypershift/v1beta1/hostedcluster_types.go || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 1645 - 1646, The kubebuilder marker on the RoleARN field uses invalid syntax `Pattern:=` so the OpenAPI regex isn't applied; update the kubebuilder marker for RoleARN to use `+kubebuilder:validation:Pattern=` (remove the colon-equals) and ensure the regex is wrapped in backticks exactly as `^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` so the schema generator picks up the pattern validation for the RoleARN field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 101-181: The roleARN pattern allows an empty role identifier;
update the regex for the roleARN field (the pattern attribute under roleARN) so
it requires at least one character after "role/". Replace the trailing "/.*$"
style token with a construct that forbids empty names (e.g. require one or more
characters after role/), keeping the existing ARN prefix and account-ID match
intact so roleARN validation rejects ARNs that end with "role/".
---
Duplicate comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 1645-1646: The kubebuilder marker on the RoleARN field uses
invalid syntax `Pattern:=` so the OpenAPI regex isn't applied; update the
kubebuilder marker for RoleARN to use `+kubebuilder:validation:Pattern=` (remove
the colon-equals) and ensure the regex is wrapped in backticks exactly as
`^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.*$` so the schema generator
picks up the pattern validation for the RoleARN field.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 114-116: The AutoNode description string still contains the typo
"provisioner(e.g. Karpenter)"; update the autoNode description so it reads
"provisioner (e.g. Karpenter)" (i.e., insert a single space before the opening
parenthesis). Locate the AutoNode/autoNode description block in the generated
CRD manifest and modify that literal description value accordingly to preserve
formatting and newlines.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 116-118: Update the autoNode description text in the CRD manifest
to fix punctuation around the example: change "provisioner(e.g. Karpenter)" to
"provisioner (e.g., Karpenter)" in the description for autoNode so the generated
docs include proper spacing and the comma after "e.g.".
- Line 213: The roleARN schema pattern currently allows empty or whitespace-only
suffixes; update the pattern value for the roleARN field (the YAML "pattern:"
under the roleARN schema) to a stricter regular expression that requires one or
more non-empty path/name segments, disallows trailing slashes and spaces,
permits only valid IAM name characters (letters, digits and the characters _ +=
, . @ -) in each segment, allows optional intermediate path segments separated
by '/', and ensures the final segment is non-empty; replace the existing loose
"role\/.*" pattern with this tightened pattern.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 212-213: The regex for roleARN currently allows an empty role name
because it uses "role\/.*$"; update the schema/generator that emits the roleARN
pattern (the CRD field named roleARN in the HostedCluster TechPreviewNoUpgrade
type) to require at least one character after "role/", e.g. change "role\/.*$"
to "role\/.+$" (or equivalent) in the generator/source type so regenerated CRD
will reject ARNs ending with "role/"; ensure you modify the source type or
generator template rather than the generated YAML to prevent regressions on
regen.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml`:
- Line 183: The roleARN regex currently allows empty role names because it uses
".*"; update the three pattern entries that match role ARNs in
hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yaml to require at least one
character after "role/" by replacing "role\/.*$" with "role\/.+$" (i.e., change
the pattern to ^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/.+$) for each
occurrence referenced (the three roleARN pattern lines).
In `@docs/content/reference/aggregated-docs.md`:
- Around line 29066-29067: Update the phrasing in the autoNode documentation to
fix spacing and punctuation: replace occurrences of "provisioner(e.g.
Karpenter)" with "provisioner (e.g., Karpenter)" in the aggregated docs (look
for the autoNode paragraph and any other instances referenced by the reviewer),
ensuring the space before the parenthesis and the comma after "e." are added for
consistency.
- Around line 37629-37859: The embedded JSON examples (the OIDC trust
relationship example beginning with the { "Version": "2012-10-17", "Statement":
... } trust block and the subsequent policy document starting with { "Version":
"2012-10-17", "Statement": [ ... ] }) are placed inside paragraph markup and
getting corrupted by <em> rendering; replace those paragraph-wrapped JSON blobs
with fenced/preformatted code blocks (triple backticks) so the JSON is preserved
verbatim, and ensure placeholders/ARNs are either HTML-escaped (e.g.,
<oidc-provider-arn>) or left raw inside the code fences so no <em> tags
are injected.
In `@docs/content/reference/api.md`:
- Around line 520-521: Fix the repeated formatting typo where the phrase
"provisioner(e.g. Karpenter)" is missing a space before the parenthesis; update
all occurrences of the exact token "provisioner(e.g. Karpenter)" in the document
(including the other occurrence with the same token) to "provisioner (e.g.
Karpenter)" to restore consistent spacing and readability.
- Around line 9345-9346: Update the documentation for the platform field to
explicitly state that KarpenterConfig.platform currently supports only AWS;
locate the description for the platform field (referenced as
KarpenterConfig.platform / "platform specifies the infrastructure platform") and
change the text to mention "AWS-only" (or equivalent phrasing) and, if helpful,
add a short note about future platform support being planned or to check
compatibility docs.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 4642-4644: The test fixture is using an invalid union
discriminator: set ProvisionerConfig.Name to the Karpenter discriminator value
instead of "test-provisioner" so the Karpenter union member is valid; locate the
Provisioner: hyperv1.ProvisionerConfig block (and its nested Karpenter:
&hyperv1.KarpenterConfig) and replace the Name field with the correct
discriminator (e.g., "Karpenter" or the corresponding constant) so the fixture
matches the API's enum constraint for the Karpenter union member.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f81374d-4933-4fdc-ab15-4caaafd612f4
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (15)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/aws/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.gosupport/karpenter/karpenter.go
🚧 Files skipped from review as they are similar to previous changes (4)
- karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
- cmd/cluster/aws/create.go
- support/karpenter/karpenter.go
enxebre
force-pushed
the
fix-autonode-api-conventions
branch
from
cdab6ad to
d35fcc6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
docs/content/reference/api.md (2)
9345-9346:⚠️ Potential issue | 🟡 MinorClarify that
KarpenterConfig.platformcurrently supports only AWS.The description is still platform-agnostic, but this field is constrained to
AWS. Please make that explicit in the field description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 9345 - 9346, The field description for KarpenterConfig.platform is currently generic; update the documentation text for the KarpenterConfig.platform field to explicitly state that only AWS is supported today (e.g., "platform specifies the infrastructure platform that Karpenter should provision nodes on; currently only 'AWS' is supported"), and list the allowed/expected value ("AWS") or mark other platforms as unsupported so readers know this is constrained to AWS.
520-521:⚠️ Potential issue | 🟡 MinorFix repeated typo in provisioner example formatting.
provisioner(e.g. Karpenter)is still missing a space in both places. Please change toprovisioner (e.g. Karpenter)for readability and consistency.Also applies to: 7238-7239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 520 - 521, Replace the missing space in the provisioner phrasing: change every occurrence of "provisioner(e.g. Karpenter)" to "provisioner (e.g. Karpenter)" in the API reference text (notably within the autoNode description and the other occurrence referenced around lines 7238-7239) so the documentation reads "provisioner (e.g. Karpenter)" for consistency and readability.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
114-120:⚠️ Potential issue | 🟡 MinorFix spacing typo in AutoNode description.
Line 116 still has
provisioner(e.g. Karpenter); it should include a space before the parenthesis.✏️ Suggested fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g. Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 114 - 120, Fix the spacing typo in the AutoNode description: locate the description field under AutoNode (the block describing "autoNode specifies the configuration...") and change "provisioner(e.g. Karpenter)" to "provisioner (e.g. Karpenter)"; ensure the surrounding YAML (properties -> provisionerConfig description) remains unchanged.docs/content/reference/aggregated-docs.md (2)
37629-37859:⚠️ Potential issue | 🟠 MajorRender IAM JSON examples as code blocks (current markup corrupts ARNs).
The JSON examples are still in paragraph markup and are being mangled (e.g., wildcard/ARN segments converted to
<em>), which makes them unsafe to copy as policies. Use fenced code blocks (or<pre><code>) and escape angle-bracket placeholders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 37629 - 37859, The IAM JSON examples (around the roleARN section and the policy statements with Sids like "AllowScopedEC2InstanceAccessActions", "AllowScopedEC2LaunchTemplateAccessActions", etc.) are rendered as paragraphs and mangling ARNs; replace those paragraph-wrapped JSON blocks with proper fenced code blocks (or <pre><code>) and escape angle-bracket placeholders (e.g., <oidc-provider-arn>, <oidc-provider-name>, ARNs/wildcards) so they appear literally and are safe to copy; ensure each JSON example is enclosed in a single code block and retains original quotation/curly-brace formatting.
29066-29067:⚠️ Potential issue | 🟡 MinorFix the
provisioner(e.g. Karpenter)punctuation/spacing.This wording still misses the space before
(and the comma aftere.g.. Please useprovisioner (e.g., Karpenter)consistently in both occurrences (Line 29067 and Line 35785).Also applies to: 35784-35785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 29066 - 29067, The phrase "provisioner(e.g. Karpenter)" in the autoNode documentation is incorrectly spaced and punctuated; update both occurrences of that snippet to "provisioner (e.g., Karpenter)" so there is a space before the parenthesis and the abbreviation includes the comma after "e.g."; search for the exact string "provisioner(e.g. Karpenter)" near the "autoNode" description and replace both instances with "provisioner (e.g., Karpenter)".hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
4642-4644:⚠️ Potential issue | 🟡 MinorUse a valid union discriminator for
ProvisionerConfig.Name.Line 4643 uses
"test-provisioner", but this field is enum-constrained toKarpenter. Keep the fixture API-valid by using the typed constant.Suggested fix
AutoNode: &hyperv1.AutoNode{ Provisioner: hyperv1.ProvisionerConfig{ - Name: "test-provisioner", + Name: hyperv1.ProvisionerKarpenter, Karpenter: &hyperv1.KarpenterConfig{ Platform: hyperv1.AWSPlatform, AWS: &hyperv1.KarpenterAWSConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 4642 - 4644, Replace the hard-coded string "test-provisioner" used as the union discriminator in ProvisionerConfig.Name with the API-typed enum constant (e.g., use the hyperv1 Karpenter discriminator constant such as hyperv1.KarpenterProvisioner or hyperv1.ProvisionerKarpenter) so the fixture is API-valid; update the ProvisionerConfig block (hyperv1.ProvisionerConfig{Name: ...}) to reference that constant instead of the literal string.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml (1)
184-187:⚠️ Potential issue | 🟡 Minor
roleARNregex is still too permissive for strict IAM role ARN validation.Line 187 accepts any characters after
role/, so non-IAM role resource strings can pass schema validation. If strict IAM-role ARN validation is intended, tighten this pattern.What characters are allowed in an AWS IAM Role ARN resource (role name and optional path), and what constraints apply to RoleName and Path?Suggested regex tightening
- rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') + rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/(?:[A-Za-z0-9+=,.@_-]+/)*[A-Za-z0-9+=,.@_-]+$')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml` around lines 184 - 187, The current x-kubernetes-validations rule for roleARN is too permissive (it allows any characters after "role/"); update the validation under x-kubernetes-validations for roleARN to a stricter regex that enforces the AWS IAM role ARN structure and allowed characters: keep the arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role prefix but require an optional path and a RoleName composed only of valid IAM characters (letters, digits and the IAM allowed symbols such as +=,.@-_) with reasonable length limits (RoleName max 64) and path segments using the same allowed set, so the rule for roleARN rejects arbitrary resource strings and only accepts valid IAM role ARNs.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
1409-1642: Consider shrinking the inline IAM policy example for maintainability.The embedded JSON example is very large, which makes this API type file harder to scan and review. A shorter example plus a link to canonical policy docs would keep this type definition leaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 1409 - 1642, The inline IAM policy example under the roleARN documentation is too large; replace the long JSON blob in the roleARN comment (the comment block referencing roleARN) with a concise minimal example showing just the trust relationship and one short permission statement, and add a one-line pointer to the canonical external policy docs (e.g., "see AWS/IAM docs" or a URL) for the full policy; ensure the shortened example still shows sts:AssumeRoleWithWebIdentity and mentions required tag/permission patterns so readers know the essentials while keeping hostedcluster_types.go compact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 116-118: The autoNode description text contains a minor
formatting/grammar issue: update the string under the description for autoNode
(in the Hypershift-TechPreviewNoUpgrade CRD manifest) so the phrase
"provisioner(e.g. Karpenter)" becomes "provisioner (e.g., Karpenter)" — add the
space before the parenthesis and include the comma after "e.g." to match the
suggested wording.
---
Duplicate comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 114-120: Fix the spacing typo in the AutoNode description: locate
the description field under AutoNode (the block describing "autoNode specifies
the configuration...") and change "provisioner(e.g. Karpenter)" to "provisioner
(e.g. Karpenter)"; ensure the surrounding YAML (properties -> provisionerConfig
description) remains unchanged.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 184-187: The current x-kubernetes-validations rule for roleARN is
too permissive (it allows any characters after "role/"); update the validation
under x-kubernetes-validations for roleARN to a stricter regex that enforces the
AWS IAM role ARN structure and allowed characters: keep the
arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role prefix but require an optional
path and a RoleName composed only of valid IAM characters (letters, digits and
the IAM allowed symbols such as +=,.@-_) with reasonable length limits (RoleName
max 64) and path segments using the same allowed set, so the rule for roleARN
rejects arbitrary resource strings and only accepts valid IAM role ARNs.
In `@docs/content/reference/aggregated-docs.md`:
- Around line 37629-37859: The IAM JSON examples (around the roleARN section and
the policy statements with Sids like "AllowScopedEC2InstanceAccessActions",
"AllowScopedEC2LaunchTemplateAccessActions", etc.) are rendered as paragraphs
and mangling ARNs; replace those paragraph-wrapped JSON blocks with proper
fenced code blocks (or <pre><code>) and escape angle-bracket placeholders (e.g.,
<oidc-provider-arn>, <oidc-provider-name>, ARNs/wildcards) so they appear
literally and are safe to copy; ensure each JSON example is enclosed in a single
code block and retains original quotation/curly-brace formatting.
- Around line 29066-29067: The phrase "provisioner(e.g. Karpenter)" in the
autoNode documentation is incorrectly spaced and punctuated; update both
occurrences of that snippet to "provisioner (e.g., Karpenter)" so there is a
space before the parenthesis and the abbreviation includes the comma after
"e.g."; search for the exact string "provisioner(e.g. Karpenter)" near the
"autoNode" description and replace both instances with "provisioner (e.g.,
Karpenter)".
In `@docs/content/reference/api.md`:
- Around line 9345-9346: The field description for KarpenterConfig.platform is
currently generic; update the documentation text for the
KarpenterConfig.platform field to explicitly state that only AWS is supported
today (e.g., "platform specifies the infrastructure platform that Karpenter
should provision nodes on; currently only 'AWS' is supported"), and list the
allowed/expected value ("AWS") or mark other platforms as unsupported so readers
know this is constrained to AWS.
- Around line 520-521: Replace the missing space in the provisioner phrasing:
change every occurrence of "provisioner(e.g. Karpenter)" to "provisioner (e.g.
Karpenter)" in the API reference text (notably within the autoNode description
and the other occurrence referenced around lines 7238-7239) so the documentation
reads "provisioner (e.g. Karpenter)" for consistency and readability.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 4642-4644: Replace the hard-coded string "test-provisioner" used
as the union discriminator in ProvisionerConfig.Name with the API-typed enum
constant (e.g., use the hyperv1 Karpenter discriminator constant such as
hyperv1.KarpenterProvisioner or hyperv1.ProvisionerKarpenter) so the fixture is
API-valid; update the ProvisionerConfig block (hyperv1.ProvisionerConfig{Name:
...}) to reference that constant instead of the literal string.
---
Nitpick comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 1409-1642: The inline IAM policy example under the roleARN
documentation is too large; replace the long JSON blob in the roleARN comment
(the comment block referencing roleARN) with a concise minimal example showing
just the trust relationship and one short permission statement, and add a
one-line pointer to the canonical external policy docs (e.g., "see AWS/IAM docs"
or a URL) for the full policy; ensure the shortened example still shows
sts:AssumeRoleWithWebIdentity and mentions required tag/permission patterns so
readers know the essentials while keeping hostedcluster_types.go compact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c187510d-2be8-42bd-a413-0f902d766f80
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (15)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/aws/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.gosupport/karpenter/karpenter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/hypershift/v1beta1/hosted_controlplane.go
- karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go
- cmd/cluster/aws/create.go
Comment on lines
+116
to
+118
| description: |- | ||
| autoNode specifies the configuration for automatic node provisioning and lifecycle management. | ||
| When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. |
Contributor
There was a problem hiding this comment.
Fix a minor typo in the AutoNode description.
Line 118 should read more clearly as “provisioner (e.g., Karpenter)” instead of “provisioner(e.g. Karpenter)”.
✏️ Suggested wording fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads.
+ When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.📝 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.
Suggested change
| description: |- | |
| autoNode specifies the configuration for automatic node provisioning and lifecycle management. | |
| When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. | |
| description: |- | |
| autoNode specifies the configuration for automatic node provisioning and lifecycle management. | |
| When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`
around lines 116 - 118, The autoNode description text contains a minor
formatting/grammar issue: update the string under the description for autoNode
(in the Hypershift-TechPreviewNoUpgrade CRD manifest) so the phrase
"provisioner(e.g. Karpenter)" becomes "provisioner (e.g., Karpenter)" — add the
space before the parenthesis and include the comma after "e.g." to match the
suggested wording.
d35fcc6 to
bfec0f5
Compare
Contributor
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)
karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go (1)
103-107:⚠️ Potential issue | 🟡 MinorPopulate the AWS union member when
Platformis AWS.Line [106] and Line [421] set
Platform: hyperv1.AWSPlatformbut omitKarpenterConfig.AWS. With discriminator/member validation, these fixtures should include the AWS member to stay contract-valid.Suggested fix
Provisioner: hyperv1.ProvisionerConfig{ Name: hyperv1.ProvisionerKarpenter, Karpenter: &hyperv1.KarpenterConfig{ Platform: hyperv1.AWSPlatform, + AWS: &hyperv1.KarpenterAWSConfig{ + RoleARN: "test-role-arn", + }, }, },Also applies to: 418-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go` around lines 103 - 107, The test fixtures set ProvisionerConfig with KarpenterConfig.Platform = hyperv1.AWSPlatform but omit the required AWS union member; update the fixtures to populate KarpenterConfig.AWS (e.g. set KarpenterConfig{Platform: hyperv1.AWSPlatform, AWS: &hyperv1.KarpenterAWSConfig{...}}) with a minimal valid value so the discriminator/member validation passes—look for the Provisioner: hyperv1.ProvisionerConfig and Karpenter: &hyperv1.KarpenterConfig instances in the test and add a non-nil AWS field for each AWSPlatform case.
♻️ Duplicate comments (7)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
114-116:⚠️ Potential issue | 🟡 MinorFix spacing typo in autoNode description.
Line 116 still has
provisioner(e.g. Karpenter); please add a space before(for consistency/readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 114 - 116, In the AutoNodeKarpenter CRD manifest update the autoNode description string: replace "provisioner(e.g. Karpenter)" with "provisioner (e.g. Karpenter)" to add a space before the opening parenthesis; look for the description under the autoNode field in the generated featuregated CRD (AutoNodeKarpenter) and update that text accordingly.docs/content/reference/aggregated-docs.md (2)
29067-29067:⚠️ Potential issue | 🟡 MinorFix
e.g.punctuation/spacing consistency in provisioner prose.Line 29067, Line 35785, and Line 36740 still use
e.g.formatting inconsistently (provisioner(e.g. Karpenter)/e.g. Karpenter). Useprovisioner (e.g., Karpenter)consistently for readability.✏️ Suggested doc text cleanup
-When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. +When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.-using the specified provisioner (e.g. Karpenter) instead of requiring +using the specified provisioner (e.g., Karpenter) instead of requiringAlso applies to: 35785-35785, 36740-36740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` at line 29067, Update the inconsistent punctuation/spacing for the provisioner example by replacing occurrences of "provisioner(e.g. Karpenter)" and "e.g. Karpenter" with the consistent form "provisioner (e.g., Karpenter)"; search for the exact snippet "provisioner(e.g. Karpenter)" and the standalone "e.g. Karpenter" in the document (notably the occurrences referenced in the review) and apply the change so all three instances match the "provisioner (e.g., Karpenter)" style.
37631-37859:⚠️ Potential issue | 🟠 MajorRender IAM JSON examples as literal code blocks to avoid corruption.
Line 37631 through Line 37859 still embeds JSON in
<p>tags, and ARNs/placeholders are being mangled by markup (for example,<em>fragments), which makes examples unsafe to copy.🧩 Suggested formatting fix
-<p>The referenced role must have a trust relationship ... Example: -{ ... }</p> +<p>The referenced role must have a trust relationship ... Example:</p> +<pre><code>{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": { "Federated": "<oidc-provider-arn>" }, + "Action": "sts:AssumeRoleWithWebIdentity", + "Condition": { + "StringEquals": { + "<oidc-provider-name>:sub": "system:serviceaccount:kube-system:karpenter" + } + } + } + ] +}</code></pre>-<p>{ ... large policy JSON ... }</p> +<pre><code>{ ... large policy JSON ... }</code></pre>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 37631 - 37859, The JSON examples (the OIDC trust relationship object and the long policy document) are embedded inside <p> tags and getting mangled by HTML (<em> tags); replace those <p>-wrapped JSON blobs with proper literal code blocks (e.g., <pre><code class="language-json">...</code></pre> or fenced ```json blocks) for the trust relationship JSON and the policy document JSON so ARNs/placeholders are not interpreted as markup, ensure the raw JSON is HTML-escaped or enclosed in a code container to prevent insertion of <em> fragments, and remove any inline <em> or paragraph wrappers around the JSON snippets so the examples for the trust relationship and the policy document render as copy-safe JSON.hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
4642-4644:⚠️ Potential issue | 🟡 MinorUse the enum discriminator constant in this fixture.
Line [4643] still uses
"test-provisioner"even thoughProvisionerConfig.Nameis enum-constrained; this makes the test object invalid against the API union contract.Suggested fix
AutoNode: &hyperv1.AutoNode{ Provisioner: hyperv1.ProvisionerConfig{ - Name: "test-provisioner", + Name: hyperv1.ProvisionerKarpenter, Karpenter: &hyperv1.KarpenterConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 4642 - 4644, The test fixture sets ProvisionerConfig.Name to a literal "test-provisioner" which violates the enum discriminator for the union; update the test to use the enum constant for the Karpenter provisioner (replace the string on ProvisionerConfig.Name with the appropriate hyperv1 enum constant for Karpenter, e.g. hyperv1.ProvisionerTypeKarpenter) so the ProvisionerConfig and hyperv1.KarpenterConfig union is valid against the API contract.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml (1)
116-118:⚠️ Potential issue | 🟡 MinorMinor wording issue remains in
autoNodedescription.Line 118 still has
provisioner(e.g. Karpenter); this should beprovisioner (e.g., Karpenter)for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml` around lines 116 - 118, The autoNode description in the CRD manifest (the description field under autoNode) contains the phrase "provisioner(e.g. Karpenter)"; update that text to "provisioner (e.g., Karpenter)" — i.e., add a space before the opening parenthesis, include a comma after "e.g.", and keep the rest of the sentence unchanged in the hostedclusters-Hypershift-TechPreviewNoUpgrade CRD YAML.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml (2)
116-118:⚠️ Potential issue | 🟡 MinorFix the
autoNodedescription typo/punctuation.
Line 118still hasprovisioner(e.g. Karpenter), which reads incorrectly in generated API docs.Suggested text fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 116 - 118, The autoNode description contains a punctuation/spacing typo: change "provisioner(e.g. Karpenter)" to a properly spaced and punctuated phrase such as "provisioner (e.g., Karpenter)". Update the description string for the autoNode field in the generated CRD manifest (the description block that mentions autoNode) to include the space before the parenthesis and add the comma after "e.g." so the generated API docs read correctly.
214-217:⚠️ Potential issue | 🟡 MinorHarden
roleARNCEL validation to reject invalid IAM role suffixes.
Line 217allows overly broad role suffixes (role/.+), including characters AWS IAM role names/paths should reject.Suggested regex hardening
- rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') + rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/(?:[A-Za-z0-9_+=,.@-]+/)*[A-Za-z0-9_+=,.@-]+$')AWS IAM RoleName and IAM role ARN path format allowed characters (CreateRole API), including whether spaces and trailing slashes are valid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 214 - 217, The current x-kubernetes-validations rule for the roleARN field uses an overly permissive suffix pattern "role/.+"; update the CEL validation for roleARN to enforce AWS IAM role name/path allowed characters and forbid trailing slashes — locate the x-kubernetes-validations entry for roleARN in the CRD and replace the suffix match with a stricter pattern that: keeps the existing arn:... iam::[0-9]{12}:role prefix, requires a single slash then a non-empty name/path, allows only valid IAM characters (letters, digits and the documented symbols such as +=,.@_-/) in the name/path, and disallows a trailing slash or empty path component so invalid suffixes are rejected.
🤖 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
`@karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go`:
- Around line 103-107: The test fixtures set ProvisionerConfig with
KarpenterConfig.Platform = hyperv1.AWSPlatform but omit the required AWS union
member; update the fixtures to populate KarpenterConfig.AWS (e.g. set
KarpenterConfig{Platform: hyperv1.AWSPlatform, AWS:
&hyperv1.KarpenterAWSConfig{...}}) with a minimal valid value so the
discriminator/member validation passes—look for the Provisioner:
hyperv1.ProvisionerConfig and Karpenter: &hyperv1.KarpenterConfig instances in
the test and add a non-nil AWS field for each AWSPlatform case.
---
Duplicate comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 114-116: In the AutoNodeKarpenter CRD manifest update the autoNode
description string: replace "provisioner(e.g. Karpenter)" with "provisioner
(e.g. Karpenter)" to add a space before the opening parenthesis; look for the
description under the autoNode field in the generated featuregated CRD
(AutoNodeKarpenter) and update that text accordingly.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 116-118: The autoNode description contains a punctuation/spacing
typo: change "provisioner(e.g. Karpenter)" to a properly spaced and punctuated
phrase such as "provisioner (e.g., Karpenter)". Update the description string
for the autoNode field in the generated CRD manifest (the description block that
mentions autoNode) to include the space before the parenthesis and add the comma
after "e.g." so the generated API docs read correctly.
- Around line 214-217: The current x-kubernetes-validations rule for the roleARN
field uses an overly permissive suffix pattern "role/.+"; update the CEL
validation for roleARN to enforce AWS IAM role name/path allowed characters and
forbid trailing slashes — locate the x-kubernetes-validations entry for roleARN
in the CRD and replace the suffix match with a stricter pattern that: keeps the
existing arn:... iam::[0-9]{12}:role prefix, requires a single slash then a
non-empty name/path, allows only valid IAM characters (letters, digits and the
documented symbols such as +=,.@_-/) in the name/path, and disallows a trailing
slash or empty path component so invalid suffixes are rejected.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 116-118: The autoNode description in the CRD manifest (the
description field under autoNode) contains the phrase "provisioner(e.g.
Karpenter)"; update that text to "provisioner (e.g., Karpenter)" — i.e., add a
space before the opening parenthesis, include a comma after "e.g.", and keep the
rest of the sentence unchanged in the
hostedclusters-Hypershift-TechPreviewNoUpgrade CRD YAML.
In `@docs/content/reference/aggregated-docs.md`:
- Line 29067: Update the inconsistent punctuation/spacing for the provisioner
example by replacing occurrences of "provisioner(e.g. Karpenter)" and "e.g.
Karpenter" with the consistent form "provisioner (e.g., Karpenter)"; search for
the exact snippet "provisioner(e.g. Karpenter)" and the standalone "e.g.
Karpenter" in the document (notably the occurrences referenced in the review)
and apply the change so all three instances match the "provisioner (e.g.,
Karpenter)" style.
- Around line 37631-37859: The JSON examples (the OIDC trust relationship object
and the long policy document) are embedded inside <p> tags and getting mangled
by HTML (<em> tags); replace those <p>-wrapped JSON blobs with proper literal
code blocks (e.g., <pre><code class="language-json">...</code></pre> or fenced
```json blocks) for the trust relationship JSON and the policy document JSON so
ARNs/placeholders are not interpreted as markup, ensure the raw JSON is
HTML-escaped or enclosed in a code container to prevent insertion of <em>
fragments, and remove any inline <em> or paragraph wrappers around the JSON
snippets so the examples for the trust relationship and the policy document
render as copy-safe JSON.
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 4642-4644: The test fixture sets ProvisionerConfig.Name to a
literal "test-provisioner" which violates the enum discriminator for the union;
update the test to use the enum constant for the Karpenter provisioner (replace
the string on ProvisionerConfig.Name with the appropriate hyperv1 enum constant
for Karpenter, e.g. hyperv1.ProvisionerTypeKarpenter) so the ProvisionerConfig
and hyperv1.KarpenterConfig union is valid against the API contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64c6ac39-21ac-4d6d-81aa-306e01e1ae47
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (15)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/aws/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.gosupport/karpenter/karpenter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/hypershift/v1beta1/hosted_controlplane.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
- cmd/cluster/aws/create.go
enxebre
force-pushed
the
fix-autonode-api-conventions
branch
from
bfec0f5 to
5776511
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
docs/content/reference/aggregated-docs.md (2)
37629-37859:⚠️ Potential issue | 🟠 MajorRender IAM JSON examples as literal code blocks (current text is corrupted).
The JSON is still embedded in paragraph text, and wildcard/placeholder content is being transformed (for example, ARN fragments wrapped in
<em>). This is not copy-safe and can produce invalid IAM policies.Suggested doc fix
-<p>The referenced role must have a trust relationship ... Example: -{ - ... -}</p> +<p>The referenced role must have a trust relationship ... Example:</p> +<pre><code>{ + ... +}</code></pre> -<p>{ - ... -}</p> +<pre><code>{ + ... +}</code></pre>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 37629 - 37859, The IAM JSON examples (the roleARN description and the policy document example block) are currently embedded in paragraph HTML and being mangled (HTML entities and <em> tags), so replace those paragraph-wrapped JSON snippets with literal fenced code blocks (rendered as preformatted text), unescape HTML entities (e.g. “/” to straight quotes), and preserve placeholders (e.g. <oidc-provider-arn>, <oidc-provider-name>, arn:aws:...) without introducing <em> or other inline tags; update the "roleARN" description and the large policy example to use these fenced code blocks so the JSON is copy-safe and valid.
29066-29067:⚠️ Potential issue | 🟡 MinorFix punctuation/spacing regression in provisioner example text.
provisioner(e.g. Karpenter)should beprovisioner (e.g., Karpenter)for readability/consistency. This appears to have regressed in both places.Also applies to: 35784-35785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/aggregated-docs.md` around lines 29066 - 29067, The prose for autoNode's provisioner example has a punctuation/spacing regression: replace instances of "provisioner(e.g. Karpenter)" with "provisioner (e.g., Karpenter)" in the aggregated docs text for autoNode; update both occurrences referenced (the two places noted around autoNode) so there is a space before the parenthesis and a comma after "e.g." for consistency and readability.docs/content/reference/api.md (2)
9345-9346:⚠️ Potential issue | 🟡 MinorClarify that
platformcurrently supports only AWS.The description at Line 9345 is platform-agnostic, but this field is enum-constrained to
AWSin this PR. Please state that explicitly to avoid misleading readers.Proposed text change
-<p>platform specifies the infrastructure platform that Karpenter should provision nodes on.</p> +<p>platform specifies the infrastructure platform that Karpenter should provision nodes on. Currently only AWS is supported.</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 9345 - 9346, Update the documentation text for the platform field to explicitly state that the platform enum currently only supports AWS: locate the platform field description (the paragraph starting "platform specifies the infrastructure platform that Karpenter should provision nodes on.") and change it to note that the enum is constrained to "AWS" in this PR (e.g., "platform specifies the infrastructure platform that Karpenter should provision nodes on; currently this field only supports the AWS value"). Ensure the wording clearly mentions the AWS-only limitation and keeps the existing context about provisioning nodes.
520-521:⚠️ Potential issue | 🟡 MinorFix missing space in “provisioner (e.g. Karpenter)”.
Both occurrences still use
provisioner(e.g. Karpenter)(Line 521 and Line 7239). Please add the missing space for readability.Proposed text change
-When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. +When set, the provisioner (e.g. Karpenter) will be used to provision nodes for targeted workloads.Also applies to: 7238-7239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/reference/api.md` around lines 520 - 521, Replace the missing space in the phrase "provisioner(e.g. Karpenter)" with "provisioner (e.g. Karpenter)" in the docs: locate each occurrence of the exact string "provisioner(e.g. Karpenter)" (appearing in the paragraph describing autoNode) and update both instances to include the space after "provisioner" so it reads "provisioner (e.g. Karpenter)" for improved readability.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
114-117:⚠️ Potential issue | 🟡 MinorFix the spacing typo in the AutoNode description.
Line 116 still has
provisioner(e.g. Karpenter); add a space before(for readability and consistency.✏️ Proposed fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g. Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml` around lines 114 - 117, The AutoNode description contains a spacing typo in the phrase "provisioner(e.g. Karpenter)"; update the description for the autoNode field so it reads "provisioner (e.g. Karpenter)" (i.e., insert a space before the opening parenthesis) to match the project's style and improve readability.cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml (2)
214-217:⚠️ Potential issue | 🟠 MajorHarden
roleARNCEL regex to reject invalid IAM role suffixes.Line 217 still allows overly broad role suffixes via
role/.+, which admits invalid forms (e.g., whitespace segments, malformed paths) at schema-validation time.Suggested regex hardening
- rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') + rule: self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\\/(?:[A-Za-z0-9+=,.@_-]+\\/)*[A-Za-z0-9+=,.@_-]+$')#!/bin/bash python - <<'PY' import re current = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') hardened = re.compile(r'^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\/(?:[A-Za-z0-9+=,.@_-]+\/)*[A-Za-z0-9+=,.@_-]+$') cases = { "valid_basic": "arn:aws:iam::123456789012:role/MyRole", "valid_path": "arn:aws:iam::123456789012:role/service-role/MyRole", "invalid_space": "arn:aws:iam::123456789012:role/my role", "invalid_trailing_slash": "arn:aws:iam::123456789012:role/my-role/", "invalid_empty_suffix": "arn:aws:iam::123456789012:role/", } print("current:") for k,v in cases.items(): print(k, bool(current.fullmatch(v))) print("\nhardened:") for k,v in cases.items(): print(k, bool(hardened.fullmatch(v))) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 214 - 217, Update the CEL validation regex for roleARN in the x-kubernetes-validations block so it rejects invalid IAM role suffixes (no spaces, no empty segments, no trailing slash); specifically replace the current rule self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') with a stricter pattern such as self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\\/(?:[A-Za-z0-9+=,.@_-]+\\/)*[A-Za-z0-9+=,.@_-]+$') to allow valid names and path segments only for roleARN.
116-118:⚠️ Potential issue | 🟡 MinorFix the
autoNodedescription punctuation/spacing typo.Line 118 still has
provisioner(e.g. Karpenter), which reads as a docs typo in generated API docs.Suggested wording fix
- When set, the provisioner(e.g. Karpenter) will be used to provision nodes for targeted workloads. + When set, the provisioner (e.g., Karpenter) will be used to provision nodes for targeted workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml` around lines 116 - 118, The autoNode description contains a punctuation/spacing typo; update the description string for the autoNode field so "provisioner(e.g. Karpenter)" becomes "provisioner (e.g., Karpenter)" (add a space before the parenthesis and a comma after "e. g.") in the CRD manifest where the autoNode description is defined (look for the autoNode description block in the generated CRD YAML).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 220-223: Add a top-level validation under spec
(x-kubernetes-validations) that enforces the HostedCluster's spec.platform.type
is "AWS" whenever AutoNode/karpenter is enabled or karpenter.platform is set to
"AWS"; update the AutoNode/karpenter schema entries (the enum on
karpenter.platform and the other similar entries at the referenced sections) to
include/require this cross-field guard so invalid combinations (e.g.,
spec.autoNode or karpenter.platform == AWS while spec.platform.type != AWS) are
rejected at admission time. Specifically, add a validation rule referencing
spec.autoNode (or spec.autoNode.enabled) and spec.karpenter.platform and
asserting spec.platform.type == "AWS" (and mirror the same guard for the other
occurrences listed) so the CRD prevents non-AWS HostedClusters from enabling
AutoNode/Karpenter.
In `@docs/content/reference/aggregated-docs.md`:
- Line 31397: The AutoNode paragraph (and other affected sections) contains
nested <p> tags producing invalid HTML; open the aggregated-docs.md entry for
the "AutoNode" section (and the other reported occurrences) and remove the inner
<p> wrapper so only a single <p> surrounds the text (e.g., replace
<p><p>...</p></p> with <p>...</p>), ensuring any adjacent markup or headings
remain intact; update the same fix for the other referenced occurrences (lines
noted in the review) to eliminate nested paragraph tags across the file.
In `@docs/content/reference/api.md`:
- Line 2863: The docs generator is rendering struct tag options (e.g.,
json:"provisionerConfig,omitzero") as part of the field name; update the
tag-parsing logic used when rendering JSON field names to split the struct tag
on the first comma and use only the left side (the actual field name). Locate
the code path that reads Go struct tags for the API docs (the function that
formats or renders JSON tags for fields — e.g., the routine that processes tags
from files like api/hypershift/v1beta1/hostedcluster_types.go and
api/hypershift/v1beta1/gcp.go) and change it to extract tagName :=
strings.Split(tagValue, ",")[0] (or equivalent) before outputting; regenerate
docs and add/adjust a unit/integration test to assert tags with options
(",omitzero", etc.) render only the base field name.
---
Duplicate comments:
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml`:
- Around line 114-117: The AutoNode description contains a spacing typo in the
phrase "provisioner(e.g. Karpenter)"; update the description for the autoNode
field so it reads "provisioner (e.g. Karpenter)" (i.e., insert a space before
the opening parenthesis) to match the project's style and improve readability.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 214-217: Update the CEL validation regex for roleARN in the
x-kubernetes-validations block so it rejects invalid IAM role suffixes (no
spaces, no empty segments, no trailing slash); specifically replace the current
rule self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role/.+$') with a
stricter pattern such as
self.matches('^arn:(aws|aws-cn|aws-us-gov):iam::[0-9]{12}:role\\/(?:[A-Za-z0-9+=,.@_-]+\\/)*[A-Za-z0-9+=,.@_-]+$')
to allow valid names and path segments only for roleARN.
- Around line 116-118: The autoNode description contains a punctuation/spacing
typo; update the description string for the autoNode field so "provisioner(e.g.
Karpenter)" becomes "provisioner (e.g., Karpenter)" (add a space before the
parenthesis and a comma after "e. g.") in the CRD manifest where the autoNode
description is defined (look for the autoNode description block in the generated
CRD YAML).
In `@docs/content/reference/aggregated-docs.md`:
- Around line 37629-37859: The IAM JSON examples (the roleARN description and
the policy document example block) are currently embedded in paragraph HTML and
being mangled (HTML entities and <em> tags), so replace those paragraph-wrapped
JSON snippets with literal fenced code blocks (rendered as preformatted text),
unescape HTML entities (e.g. “/” to straight quotes), and preserve
placeholders (e.g. <oidc-provider-arn>, <oidc-provider-name>, arn:aws:...)
without introducing <em> or other inline tags; update the "roleARN" description
and the large policy example to use these fenced code blocks so the JSON is
copy-safe and valid.
- Around line 29066-29067: The prose for autoNode's provisioner example has a
punctuation/spacing regression: replace instances of "provisioner(e.g.
Karpenter)" with "provisioner (e.g., Karpenter)" in the aggregated docs text for
autoNode; update both occurrences referenced (the two places noted around
autoNode) so there is a space before the parenthesis and a comma after "e.g."
for consistency and readability.
In `@docs/content/reference/api.md`:
- Around line 9345-9346: Update the documentation text for the platform field to
explicitly state that the platform enum currently only supports AWS: locate the
platform field description (the paragraph starting "platform specifies the
infrastructure platform that Karpenter should provision nodes on.") and change
it to note that the enum is constrained to "AWS" in this PR (e.g., "platform
specifies the infrastructure platform that Karpenter should provision nodes on;
currently this field only supports the AWS value"). Ensure the wording clearly
mentions the AWS-only limitation and keeps the existing context about
provisioning nodes.
- Around line 520-521: Replace the missing space in the phrase "provisioner(e.g.
Karpenter)" with "provisioner (e.g. Karpenter)" in the docs: locate each
occurrence of the exact string "provisioner(e.g. Karpenter)" (appearing in the
paragraph describing autoNode) and update both instances to include the space
after "provisioner" so it reads "provisioner (e.g. Karpenter)" for improved
readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7997d8c1-4bff-48db-bf82-784498016706
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*
📒 Files selected for processing (15)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlcmd/cluster/aws/create.gocmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlcontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.godocs/content/reference/aggregated-docs.mddocs/content/reference/api.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.gosupport/karpenter/karpenter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
- karpenter-operator/controllers/karpenterignition/karpenterignition_controller_test.go
| </p> | ||
| <p> | ||
| <p>We expose here internal configuration knobs that won’t be exposed to the service.</p> | ||
| <p>AutoNode specifies the configuration for automatic node provisioning and lifecycle management.</p> |
Contributor
There was a problem hiding this comment.
Remove nested <p> tags to avoid invalid HTML output.
These sections currently render as <p><p>...</p></p>, which is invalid markup and can cause unpredictable doc rendering. Keep a single paragraph wrapper.
Suggested doc fix
-<p>
-<p>AutoNode specifies the configuration for automatic node provisioning and lifecycle management.</p>
-</p>
+<p>AutoNode specifies the configuration for automatic node provisioning and lifecycle management.</p>Also applies to: 37611-37611, 37870-37871, 41733-41734, 41753-41754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/reference/aggregated-docs.md` at line 31397, The AutoNode
paragraph (and other affected sections) contains nested <p> tags producing
invalid HTML; open the aggregated-docs.md entry for the "AutoNode" section (and
the other reported occurrences) and remove the inner <p> wrapper so only a
single <p> surrounds the text (e.g., replace <p><p>...</p></p> with <p>...</p>),
ensuring any adjacent markup or headings remain intact; update the same fix for
the other referenced occurrences (lines noted in the review) to eliminate nested
paragraph tags across the file.
Member
Author
|
/test verify |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
- Add +union, +unionDiscriminator, +unionMember markers to ProvisionerConfig and KarpenterConfig discriminated unions - Add CEL validation rules enforcing union member presence matches the discriminator value - Add field-level +kubebuilder:validation:Enum=AWS on KarpenterConfig.Platform to restrict to supported platforms - Change AutoNode.Provisioner from pointer to value (required field) with omitzero JSON tag - Add omitempty to all fields per current conventions - Use CEL XValidation instead of Pattern for roleARN validation for better error messages - Improve type-level and field-level Godoc on AutoNode, ProvisionerConfig, KarpenterConfig, and KarpenterAWSConfig - Add IAM trust and permissions policy documentation on roleARN Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
enxebre
force-pushed
the
fix-autonode-api-conventions
branch
from
5776511 to
7c31664
Compare
JoelSpeed
reviewed
Mar 6, 2026
Contributor
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
No further feedback on the API changes from me here
Contributor
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
Member
Author
|
/test e2e-aks |
Member
Author
|
/test e2e-aws-4-21 |
Contributor
|
@enxebre: 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. |
Member
Author
|
/verified by e2e @enxebre |
openshift-ci-robot
added
the
verified
|
@enxebre: This PR has been marked as verified by 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Documentation
Validation
Refactor / API