Skip to content

OCPBUGS-76350: Honor AWS AMI override in NodePool token generation #7675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 2 commits into from
Feb 25, 2026
Merged

OCPBUGS-76350: Honor AWS AMI override in NodePool token generation #7675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29f2981
fix(operator): honor AWS AMI override in NodePool token generation
Feb 6, 2026
1223fd7
test: add tests for AWS AMI override in token generation
Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,9 @@ func isAutoscalingEnabled(nodePool *hyperv1.NodePool) bool {
}

func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) {
if releaseImage.StreamMetadata == nil {
return "", fmt.Errorf("release image stream metadata is nil")
}
arch, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]
if !foundArch {
return "", fmt.Errorf("couldn't find OS metadata for architecture %q", specifiedArch)
Expand Down
16 changes: 15 additions & 1 deletion hypershift-operator/controllers/nodepool/nodepool_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,15 @@ func TestDefaultNodePoolAMI(t *testing.T) {
specifiedArch: "arm64",
expectedImage: "",
},
{
name: "fail because stream metadata is nil",
region: "us-east-1",
specifiedArch: "amd64",
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: nil,
},
expectedImage: "",
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -605,7 +614,9 @@ func TestDefaultNodePoolAMI(t *testing.T) {
}

ctx := t.Context()
tc.releaseImage = fakereleaseprovider.GetReleaseImage(ctx, hc, client, releaseProvider)
if tc.releaseImage == nil {
tc.releaseImage = fakereleaseprovider.GetReleaseImage(ctx, hc, client, releaseProvider)
}

tc.image, tc.err = defaultNodePoolAMI(tc.region, tc.specifiedArch, tc.releaseImage)
if strings.Contains(tc.name, "successfully") {
Expand All @@ -617,6 +628,9 @@ func TestDefaultNodePoolAMI(t *testing.T) {
} else if strings.Contains(tc.name, "fail because architecture") {
g.Expect(tc.image).To(BeEmpty())
g.Expect(tc.err.Error()).To(Equal("couldn't find OS metadata for architecture \"" + tc.specifiedArch + "\""))
} else if strings.Contains(tc.name, "stream metadata is nil") {
g.Expect(tc.image).To(BeEmpty())
g.Expect(tc.err.Error()).To(Equal("release image stream metadata is nil"))
} else {
g.Expect(tc.image).To(BeEmpty())
g.Expect(tc.err.Error()).To(Equal("release image metadata has no image for region \"" + tc.region + "\""))
Expand Down
16 changes: 5 additions & 11 deletions hypershift-operator/controllers/nodepool/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ type userData struct {
caCert []byte
ignitionServerEndpoint string
proxy *configv1.Proxy
ami string
}

// NewToken is the contract to create a new Token struct.
Expand Down Expand Up @@ -134,19 +133,10 @@ func NewToken(ctx context.Context, configGenerator *ConfigGenerator, cpoCapabili
proxy := globalconfig.ProxyConfig()
globalconfig.ReconcileProxyConfigWithStatusFromHostedCluster(proxy, configGenerator.hostedCluster)

ami := ""
if configGenerator.hostedCluster.Spec.Platform.AWS != nil {
ami, err = defaultNodePoolAMI(configGenerator.hostedCluster.Spec.Platform.AWS.Region, configGenerator.nodePool.Spec.Arch, configGenerator.releaseImage)
if err != nil {
return nil, err
}
}

token.userData = &userData{
ignitionServerEndpoint: ignEndpoint,
caCert: caCert,
proxy: proxy,
ami: ami,
}

return token, nil
Expand Down Expand Up @@ -377,7 +367,11 @@ func (t *Token) reconcileUserDataSecret(userDataSecret *corev1.Secret, token str
if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) {
npLabels := t.nodePool.GetLabels()
if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" {
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = t.userData.ami
ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
if err != nil {
return fmt.Errorf("failed to get default node pool AMI: %w", err)
}
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami
userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true"
Comment on lines 367 to 375

@coderabbitai coderabbitai Bot Feb 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

rg -n "defaultNodePoolAMI" --type=go -C3

Repository: openshift/hypershift

Length of output: 3778

🏁 Script executed:

sed -n '60,80p' hypershift-operator/controllers/nodepool/aws.go

Repository: openshift/hypershift

Length of output: 866

🏁 Script executed:

sed -n '300,320p' hypershift-operator/controllers/nodepool/aws.go

Repository: openshift/hypershift

Length of output: 1132

🏁 Script executed:

rg "Platform\.AWS\.AMI" --type=go -B2 -A2

Repository: openshift/hypershift

Length of output: 1187

🏁 Script executed:

sed -n '45,90p' hypershift-operator/controllers/nodepool/aws.go

Repository: openshift/hypershift

Length of output: 1793

🏁 Script executed:

sed -n '360,380p' hypershift-operator/controllers/nodepool/token.go

Repository: openshift/hypershift

Length of output: 991

Honor NodePool AMI override before falling back to defaultNodePoolAMI.

Lines 370–374 compute the AMI solely via defaultNodePoolAMI(...), ignoring any user-specified nodePool.Spec.Platform.AWS.AMI. This creates an inconsistency: the awsMachineTemplateSpec function in aws.go checks for the override first, but the Karpenter path in token.go does not. The UserDataAMILabel will therefore be set to the default AMI even when a custom AMI is specified, breaking consistency for Karpenter and failing in ISO/air-gapped environments that require custom AMIs.

✅ Proposed fix 
-			ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
-			if err != nil {
-				return fmt.Errorf("failed to get default node pool AMI: %w", err)
-			}
+			ami := ""
+			if t.nodePool.Spec.Platform.AWS != nil && t.nodePool.Spec.Platform.AWS.AMI != "" {
+				ami = t.nodePool.Spec.Platform.AWS.AMI
+			} else {
+				var err error
+				ami, err = defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
+				if err != nil {
+					return fmt.Errorf("failed to get default node pool AMI: %w", err)
+				}
+			}
 			userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami
📝 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
if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) {
npLabels := t.nodePool.GetLabels()
if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" {
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = t.userData.ami
ami, err := defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
if err != nil {
return fmt.Errorf("failed to get default node pool AMI: %w", err)
}
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami
userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true"
if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) {
npLabels := t.nodePool.GetLabels()
if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" {
ami := ""
if t.nodePool.Spec.Platform.AWS != nil && t.nodePool.Spec.Platform.AWS.AMI != "" {
ami = t.nodePool.Spec.Platform.AWS.AMI
} else {
var err error
ami, err = defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region, t.nodePool.Spec.Arch, t.releaseImage)
if err != nil {
return fmt.Errorf("failed to get default node pool AMI: %w", err)
}
}
userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel] = ami
userDataSecret.Labels[karpenterutil.ManagedByKarpenterLabel] = "true"
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@hypershift-operator/controllers/nodepool/token.go` around lines 367 - 375,
The Karpenter path always calls defaultNodePoolAMI and ignores any
user-specified AMI override; update the block guarded by
karpenterutil.IsKarpenterEnabled (and the t.nodePool label check) to first
inspect t.nodePool.Spec.Platform?.AWS?.AMI and, if non-empty, use that AMI value
for userDataSecret.Labels[hyperkarpenterv1.UserDataAMILabel], otherwise fall
back to calling defaultNodePoolAMI(t.hostedCluster.Spec.Platform.AWS.Region,
t.nodePool.Spec.Arch, t.releaseImage); ensure you safely handle nil Platform/AWS
pointers and preserve setting karpenterutil.ManagedByKarpenterLabel="true".

@hypershift-jira-solve-ci hypershift-jira-solve-ci Bot Feb 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the analysis, but this is intentional. The human reviewer (@muraee) has explicitly and repeatedly clarified that Karpenter does not use NodePool spec fields — including nodePool.Spec.Platform.AWS.AMI. The Karpenter NodePool is a system-managed resource, so checking for a user-specified AMI override on it is unnecessary.

The current code correctly calls defaultNodePoolAMI() directly, as requested by the reviewer.

AI-assisted response via Claude Code

@coderabbitai coderabbitai Bot Feb 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped: comment is from another GitHub bot.

}
}
Expand Down