Skip to content
Closed
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
40 changes: 26 additions & 14 deletions pkg/cmd/render/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -803,20 +803,32 @@ func getFeatureGatesStatus(installConfig map[string]any) (sets.Set[configv1.Feat
necessaryFeatureGates := []configv1.FeatureGateName{"ShortCertRotation"}
disabled.Insert(necessaryFeatureGates...)

featureGates, found := installConfig["featureGates"]
if !found {
return enabled, disabled
}

for _, featureGate := range featureGates.([]any) {
key := strings.Split(featureGate.(string), "=")[0]
value := strings.Split(featureGate.(string), "=")[1]
if value == "true" {
enabled = enabled.Insert(configv1.FeatureGateName(key))
disabled = disabled.Delete(configv1.FeatureGateName(key))
} else if value == "false" {
enabled = enabled.Delete(configv1.FeatureGateName(key))
disabled = disabled.Insert(configv1.FeatureGateName(key))
if featureSetRaw, found := installConfig["featureSet"]; found {
featureSet := configv1.FeatureSet(featureSetRaw.(string))
Comment on lines +806 to +807

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Unchecked type assertion on featureSetRaw.

featureSetRaw.(string) will panic if installConfig["featureSet"] is present but not a string (e.g., malformed YAML producing an int/bool/nil). This runs on the bootstrap render path, so a malformed install-config would crash rendering instead of degrading gracefully.

As per path instructions, Go code should "Never ignore error returns"; the same principle applies to unchecked type assertions on values sourced from external config here.

🛡️ Proposed fix using comma-ok assertion
 	if featureSetRaw, found := installConfig["featureSet"]; found {
-		featureSet := configv1.FeatureSet(featureSetRaw.(string))
-		if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
+		featureSetStr, ok := featureSetRaw.(string)
+		if !ok {
+			return enabled, disabled
+		}
+		featureSet := configv1.FeatureSet(featureSetStr)
+		if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
📝 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 featureSetRaw, found := installConfig["featureSet"]; found {
featureSet := configv1.FeatureSet(featureSetRaw.(string))
if featureSetRaw, found := installConfig["featureSet"]; found {
featureSetStr, ok := featureSetRaw.(string)
if !ok {
return enabled, disabled
}
featureSet := configv1.FeatureSet(featureSetStr)
if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/render/render.go` around lines 806 - 807, The featureSet handling in
render.go uses an unchecked type assertion on installConfig["featureSet"], which
can panic on malformed external config. Update the featureSetRaw handling in the
render path to use a comma-ok string assertion (and skip or error out gracefully
when the value is not a string) so the bootstrap rendering flow does not crash.
Use the existing installConfig["featureSet"] lookup and configv1.FeatureSet
assignment location to apply the fix.

Source: Path instructions

if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
for _, fg := range resolved.Enabled {
enabled.Insert(fg.FeatureGateAttributes.Name)
disabled.Delete(fg.FeatureGateAttributes.Name)
}
for _, fg := range resolved.Disabled {
if !enabled.Has(fg.FeatureGateAttributes.Name) {
disabled.Insert(fg.FeatureGateAttributes.Name)
}
}
}
}

if featureGates, found := installConfig["featureGates"]; found {
for _, featureGate := range featureGates.([]any) {
key := strings.Split(featureGate.(string), "=")[0]

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.

Could do key, value, found := strings.Cut(featureGate.(string), "=") and that would address CodeRabbit's complaint

value := strings.Split(featureGate.(string), "=")[1]
if value == "true" {
enabled = enabled.Insert(configv1.FeatureGateName(key))
disabled = disabled.Delete(configv1.FeatureGateName(key))
} else if value == "false" {
enabled = enabled.Delete(configv1.FeatureGateName(key))
disabled = disabled.Insert(configv1.FeatureGateName(key))
}
}
}
Comment on lines +821 to 833

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Unbounded index access on strings.Split result.

strings.Split(featureGate.(string), "=")[1] panics with an index-out-of-range if the entry has no = (e.g., a bare "FooBar" in the list). Since this is bootstrap-critical rendering, a single malformed entry crashes the whole render rather than being reported as a config error.

🛡️ Proposed fix using SplitN with a length check
 		for _, featureGate := range featureGates.([]any) {
-			key := strings.Split(featureGate.(string), "=")[0]
-			value := strings.Split(featureGate.(string), "=")[1]
+			parts := strings.SplitN(featureGate.(string), "=", 2)
+			if len(parts) != 2 {
+				continue
+			}
+			key, value := parts[0], parts[1]
 			if value == "true" {
📝 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 featureGates, found := installConfig["featureGates"]; found {
for _, featureGate := range featureGates.([]any) {
key := strings.Split(featureGate.(string), "=")[0]
value := strings.Split(featureGate.(string), "=")[1]
if value == "true" {
enabled = enabled.Insert(configv1.FeatureGateName(key))
disabled = disabled.Delete(configv1.FeatureGateName(key))
} else if value == "false" {
enabled = enabled.Delete(configv1.FeatureGateName(key))
disabled = disabled.Insert(configv1.FeatureGateName(key))
}
}
}
if featureGates, found := installConfig["featureGates"]; found {
for _, featureGate := range featureGates.([]any) {
parts := strings.SplitN(featureGate.(string), "=", 2)
if len(parts) != 2 {
continue
}
key, value := parts[0], parts[1]
if value == "true" {
enabled = enabled.Insert(configv1.FeatureGateName(key))
disabled = disabled.Delete(configv1.FeatureGateName(key))
} else if value == "false" {
enabled = enabled.Delete(configv1.FeatureGateName(key))
disabled = disabled.Insert(configv1.FeatureGateName(key))
}
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/render/render.go` around lines 821 - 833, The featureGates parsing in
the render logic is indexing the result of strings.Split without validation,
which can panic on malformed entries. Update the feature gate handling in
render.go to parse each entry safely in the same block that iterates
featureGates, using a bounded split or equivalent validation before reading
key/value. If an entry does not contain a valid key=value pair, report it as a
configuration error instead of crashing, and keep the enabled/disabled set
updates in the existing render flow.


Expand Down
156 changes: 156 additions & 0 deletions pkg/cmd/render/render_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,64 @@ data:
ecdsa:
curve: P521
`

clusterConfigMapWithCustomPKIAndFeatureSet = `
apiVersion: v1
kind: ConfigMap
metadata:
name: cluster-config-v1
namespace: kube-system
data:
install-config: |
apiVersion: v1
baseDomain: gcp.devcluster.openshift.com
compute:
- architecture: amd64
hyperthreading: Enabled
name: worker
platform: {}
replicas: 3
controlPlane:
architecture: amd64
hyperthreading: Enabled
name: master
platform:
gcp:
osDisk:
DiskSizeGB: 128
DiskType: pd-ssd
type: n1-standard-4
zones:
- us-east1-b
- us-east1-c
- us-east1-d
replicas: 3
metadata:
creationTimestamp: null
name: my-cluster
networking:
clusterNetwork:
- cidr: 10.128.0.0/14
hostPrefix: 23
machineCIDR: 10.0.0.0/16
machineNetwork:
- cidr: 10.0.0.0/16
networkType: OpenShiftSDN
serviceNetwork:
- 172.30.0.0/16
platform:
gcp:
projectID: openshift
region: us-east1
publish: External
featureSet: TechPreviewNoUpgrade
pki:
signerCertificates:
key:
algorithm: ECDSA
ecdsa:
curve: P521
`
)

type testConfig struct {
Expand Down Expand Up @@ -607,6 +665,53 @@ func TestTemplateDataWithCustomPKI(t *testing.T) {
testTemplateData(t, config, validateECDSAP521Signer)
}

func TestTemplateDataWithCustomPKIFeatureSet(t *testing.T) {
validateECDSAP521Signer := func(t *testing.T, td *TemplateData) {
if len(td.certificates) == 0 {
t.Fatal("no certificates generated")
}

var signerCert []byte
for _, cert := range td.certificates {
if cert.Name == "etcd-signer" {
signerCert = cert.Data["tls.crt"]
break
}
}

if signerCert == nil {
t.Fatal("etcd-signer certificate not found in generated certificates")
}

block, _ := pem.Decode(signerCert)
if block == nil {
t.Fatal("failed to decode PEM block from etcd-signer certificate")
}

x509Cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
t.Fatalf("failed to parse x509 certificate: %v", err)
}

ecdsaKey, ok := x509Cert.PublicKey.(*ecdsa.PublicKey)
if !ok {
t.Fatalf("expected ECDSA public key, got %T", x509Cert.PublicKey)
}

if ecdsaKey.Curve != elliptic.P521() {
t.Errorf("expected P521 curve, got %v", ecdsaKey.Curve.Params().Name)
}
}

config := &testConfig{
clusterNetworkConfig: networkConfigIpv4,
infraConfig: infraConfig,
clusterConfigMap: clusterConfigMapWithCustomPKIAndFeatureSet,
}

testTemplateData(t, config, validateECDSAP521Signer)
}

func hasClusterCIDR(expected ...string) func(*testing.T, *TemplateData) {
return func(t *testing.T, td *TemplateData) {
if len(td.ClusterCIDR) != len(expected) {
Expand Down Expand Up @@ -1020,6 +1125,27 @@ featureGates: [ShortCertRotation=true, UpgradeStatus=foobar]
expectedEnabled: sets.New(features.FeatureShortCertRotation),
expectedDisabled: sets.New[configv1.FeatureGateName](),
},
"featureSet TechPreviewNoUpgrade": {
installConfig: `
apiVersion: v1
metadata:
name: my-cluster
featureSet: TechPreviewNoUpgrade
`,
expectedEnabled: techPreviewEnabledGates(),
expectedDisabled: techPreviewDisabledGates(),
},
"featureSet with featureGates override": {
installConfig: `
apiVersion: v1
metadata:
name: my-cluster
featureSet: TechPreviewNoUpgrade
featureGates: [ConfigurablePKI=false]
`,
expectedEnabled: techPreviewEnabledGatesWithout(features.FeatureGateConfigurablePKI),
expectedDisabled: techPreviewDisabledGatesWith(features.FeatureGateConfigurablePKI),
},
}

for name, test := range tests {
Expand Down Expand Up @@ -1197,3 +1323,33 @@ pki:
})
}
}

func techPreviewEnabledGates() sets.Set[configv1.FeatureGateName] {
resolved := features.FeatureSets(0, features.SelfManaged, configv1.TechPreviewNoUpgrade)
s := sets.New[configv1.FeatureGateName]()
for _, fg := range resolved.Enabled {
s.Insert(fg.FeatureGateAttributes.Name)
}
return s
}

func techPreviewDisabledGates() sets.Set[configv1.FeatureGateName] {
resolved := features.FeatureSets(0, features.SelfManaged, configv1.TechPreviewNoUpgrade)
s := sets.New[configv1.FeatureGateName]()
for _, fg := range resolved.Disabled {
s.Insert(fg.FeatureGateAttributes.Name)
}
return s
}

func techPreviewEnabledGatesWithout(gates ...configv1.FeatureGateName) sets.Set[configv1.FeatureGateName] {
s := techPreviewEnabledGates()
s.Delete(gates...)
return s
}

func techPreviewDisabledGatesWith(gates ...configv1.FeatureGateName) sets.Set[configv1.FeatureGateName] {
s := techPreviewDisabledGates()
s.Insert(gates...)
return s
}