Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions hack/build-coreos-manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ func mergeMarketplaceStream(streamJSON []byte) ([]byte, error) {
arch.RHELCoreOSExtensions = &rhcos.Extensions{}
}
arch.RHELCoreOSExtensions.Marketplace = mkt
stream.Architectures[name] = arch
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/asset/manifests/openshift.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ func (o *Openshift) Dependencies() []asset.Asset {
&openshift.BaremetalConfig{},
new(rhcos.Image),
&openshift.AzureCloudProviderSecret{},
&OSImageStream{},
}
}

Expand Down Expand Up @@ -260,13 +261,15 @@ func (o *Openshift) Generate(ctx context.Context, dependencies asset.Parents) er
roleCloudCredsSecretReader := &openshift.RoleCloudCredsSecretReader{}
baremetalConfig := &openshift.BaremetalConfig{}
rhcosImage := new(rhcos.Image)
osImageStream := &OSImageStream{}

dependencies.Get(
cloudCredsSecret,
kubeadminPasswordSecret,
roleCloudCredsSecretReader,
baremetalConfig,
rhcosImage)
rhcosImage,
osImageStream)

assetData := map[string][]byte{
"99_kubeadmin-password-secret.yaml": applyTemplateData(kubeadminPasswordSecret.Files()[0].Data, templateData),
Expand Down Expand Up @@ -299,6 +302,7 @@ func (o *Openshift) Generate(ctx context.Context, dependencies asset.Parents) er

o.FileList = append(o.FileList, openshiftInstall.Files()...)
o.FileList = append(o.FileList, featureGate.Files()...)
o.FileList = append(o.FileList, osImageStream.Files()...)

asset.SortFiles(o.FileList)

Expand Down
92 changes: 92 additions & 0 deletions pkg/asset/manifests/osimagestream.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package manifests

import (
"context"
"github.com/openshift/installer/pkg/rhcos"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"path"

"github.com/openshift/api/features"
"github.com/pkg/errors"
"sigs.k8s.io/yaml"

mcfgv1alpha "github.com/openshift/api/machineconfiguration/v1alpha1"
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/installconfig"
)

var osImageStreamFileName = path.Join(openshiftManifestDir, "99_osimagestream.yaml")

// OSImageStream generates the OSImageStream manifest.
type OSImageStream struct {
FileList []*asset.File
}

var _ asset.WritableAsset = (*OSImageStream)(nil)

// Name returns a human-friendly name for the asset.
func (*OSImageStream) Name() string {
return "OSImageStream"
}

// Dependencies returns all of the dependencies directly needed to generate
// the asset.
func (*OSImageStream) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.InstallConfig{},
}
}

// Generate generates the OSImageStream CRD.
func (f *OSImageStream) Generate(_ context.Context, dependencies asset.Parents) error {
installConfig := &installconfig.InstallConfig{}
dependencies.Get(installConfig)

// If one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is CentOS Stream CoreOS
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
Comment on lines +45 to +48
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Comment mentions "OKD" but code checks IsSCOS().

The comment on line 47 says "The target is OKD" but the code calls IsSCOS(). While SCOS (CentOS Stream CoreOS) is used for OKD builds, the comment should align with the method name for clarity and maintainability.

📝 Suggested clarification
 	// If one of the following are true the OSImageStream CR is not generated
 	// 1. The feature is not enabled
-	// 2. The target is OKD
+	// 2. The target is SCOS (used by OKD)
 	if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
📝 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 one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is OKD
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
// If one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is SCOS (used by OKD)
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/asset/manifests/osimagestream.go` around lines 45 - 48, Update the
misleading comment above the OSImageStream generation check so it refers to
IsSCOS() rather than "OKD": clarify that the OSImageStream CR is not generated
when the feature gate features.FeatureGateOSStreams is disabled or when
installConfig.Config.IsSCOS() (SCOS/OKD target) is true; keep the logic using
installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams)
and installConfig.Config.IsSCOS() unchanged, only change the comment text to
match the IsSCOS() method name (or explicitly mention both SCOS and OKD for
clarity).

// FG disabled or not OCP
return nil
}

// If no stream was set just report the default one for the current version
stream := installConfig.Config.OSImageStream
if stream == "" {
stream = rhcos.DefaultOSImageStream
}

osImageStream := &mcfgv1alpha.OSImageStream{
TypeMeta: metav1.TypeMeta{
APIVersion: mcfgv1alpha.SchemeGroupVersion.String(),
Kind: "OSImageStream",
},
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
Spec: &mcfgv1alpha.OSImageStreamSpec{
DefaultStream: string(stream),
},
}
Comment on lines +45 to +70
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Skip OSImageStream generation when no stream was selected.

This currently emits 99_osimagestream.yaml whenever the feature gate is on, even if installConfig.Config.OSImageStream is empty. That produces a CR with an empty spec.defaultStream, which does not match the PR objective of generating the manifest only when a stream was chosen.

Suggested fix
-	if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
+	if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) ||
+		installConfig.Config.IsSCOS() ||
+		installConfig.Config.OSImageStream == "" {
 		// FG disabled or not OCP
 		return nil
 	}
@@
 		Spec: &mcfgv1alpha.OSImageStreamSpec{
-			DefaultStream: string(installConfig.Config.OSImageStream),
+			DefaultStream: installConfig.Config.OSImageStream,
 		},
 	}
📝 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 one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is OKD
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) || installConfig.Config.IsSCOS() {
// FG disabled or not OCP
return nil
}
osImageStream := &mcfgv1alpha.OSImageStream{
TypeMeta: metav1.TypeMeta{
APIVersion: mcfgv1alpha.SchemeGroupVersion.String(),
Kind: "OSImageStream",
},
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
Spec: &mcfgv1alpha.OSImageStreamSpec{
DefaultStream: string(installConfig.Config.OSImageStream),
},
}
// If one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is OKD
if !installConfig.Config.EnabledFeatureGates().Enabled(features.FeatureGateOSStreams) ||
installConfig.Config.IsSCOS() ||
installConfig.Config.OSImageStream == "" {
// FG disabled or not OCP
return nil
}
osImageStream := &mcfgv1alpha.OSImageStream{
TypeMeta: metav1.TypeMeta{
APIVersion: mcfgv1alpha.SchemeGroupVersion.String(),
Kind: "OSImageStream",
},
ObjectMeta: metav1.ObjectMeta{
Name: "cluster",
},
Spec: &mcfgv1alpha.OSImageStreamSpec{
DefaultStream: installConfig.Config.OSImageStream,
},
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/asset/manifests/osimagestream.go` around lines 44 - 63, The OSImageStream
CR is being created even when no stream was chosen; update the generation logic
that currently checks features.FeatureGateOSStreams and
installConfig.Config.IsSCOS to also skip when installConfig.Config.OSImageStream
is empty (or only whitespace). Specifically, before constructing the
mcfgv1alpha.OSImageStream (the osImageStream variable and its
Spec.DefaultStream), add a guard that returns nil if
installConfig.Config.OSImageStream is blank so no 99_osimagestream.yaml is
emitted with an empty spec.defaultStream.


osImageStreamData, err := yaml.Marshal(osImageStream)
if err != nil {
return errors.Wrapf(err, "failed to create %s manifests from InstallConfig", f.Name())
}

f.FileList = append(f.FileList, &asset.File{
Filename: osImageStreamFileName,
Data: osImageStreamData,
})
return nil
}

// Files returns the files generated by the asset.
func (f *OSImageStream) Files() []*asset.File {
return f.FileList
}

// Load loads the already-rendered files back from disk.
func (f *OSImageStream) Load(_ asset.FileFetcher) (bool, error) {
return false, nil
}
6 changes: 6 additions & 0 deletions pkg/rhcos/stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

package rhcos

import "github.com/openshift/installer/pkg/types"

// DefaultOSImageStream is the OS image stream used when the install-config
// does not specify one.
const DefaultOSImageStream = types.OSImageStreamRHCOS9

func getStreamFileName() string {
return "coreos/rhcos.json"
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/rhcos/stream_scos.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

package rhcos

import "github.com/openshift/installer/pkg/types"

// DefaultOSImageStream Not used in SCOS
const DefaultOSImageStream types.OSImageStream = ""

func getStreamFileName() string {
return "coreos/scos.json"
}
Expand Down