Skip to content

CORS-4005: refactor worker manifest serialization #10407

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

Open
patrickdillon wants to merge 1 commit into
base: main
Choose a base branch
from
+51 −59
Open

CORS-4005: refactor worker manifest serialization #10407

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
110 changes: 51 additions & 59 deletions pkg/asset/machines/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import (

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
meta "k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/pointer"
capz "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
ipamv1 "sigs.k8s.io/cluster-api/api/ipam/v1beta1" //nolint:staticcheck //CORS-3563
capz "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" //nolint:staticcheck //CORS-3563
"sigs.k8s.io/yaml"

configv1 "github.com/openshift/api/config/v1"
Expand Down Expand Up @@ -325,14 +325,11 @@ func (w *Worker) Generate(ctx context.Context, dependencies asset.Parents) error
dependencies.Get(clusterID, installConfig, rhcosImage, rhcosRelease, wign)

workerUserDataSecretName := "worker-user-data"

machines := []machinev1beta1.Machine{}
machineConfigs := []*mcfgv1.MachineConfig{}
machineSets := []runtime.Object{}
var ipClaims []ipamv1.IPAddressClaim
var ipAddrs []ipamv1.IPAddress
var ipClaims, ipAddrs, machines, machineSets []runtime.Object
var err error
ic := installConfig.Config

for _, pool := range ic.Compute {
pool := pool // this makes golint happy... G601: Implicit memory aliasing in for loop. (gosec)
if pool.Hyperthreading == types.HyperthreadingDisabled {
Expand Down Expand Up @@ -727,9 +724,15 @@ func (w *Worker) Generate(ctx context.Context, dependencies asset.Parents) error
return errors.Wrap(err, "failed to create worker machine objects")
}

machines = data.Machines
ipClaims = data.IPClaims
ipAddrs = data.IPAddresses
for _, m := range data.Machines {
machines = append(machines, &m)
}
for _, c := range data.IPClaims {
ipClaims = append(ipClaims, &c)
}
for _, a := range data.IPAddresses {
ipAddrs = append(ipAddrs, &a)
}
Comment on lines +727 to +735
Copy link

@coderabbitai coderabbitai bot Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the relevant loop + append sites.
rg -n -C2 'for _, m := range data\.Machines|machines = append\(machines, &m\)|for _, c := range data\.IPClaims|ipClaims = append\(ipClaims, &c\)|for _, a := range data\.IPAddresses|ipAddrs = append\(ipAddrs, &a\)' pkg/asset/machines/worker.go

# After the fix, this should produce no matches.
rg -n 'append\((machines|ipClaims|ipAddrs), &[mca]\)' pkg/asset/machines/worker.go || true

Repository: openshift/installer

Length of output: 483

Fix pointer aliasing in range loops before serialization

At Lines 731, 734, and 737, taking &m, &c, and &a captures reused range variables. All appended pointers will reference the same memory location, ending up with slices containing repeated pointers to the last iterated object instead of distinct elements.

Proposed fix 
-				for _, m := range data.Machines {
-					machines = append(machines, &m)
-				}
-				for _, c := range data.IPClaims {
-					ipClaims = append(ipClaims, &c)
-				}
-				for _, a := range data.IPAddresses {
-					ipAddrs = append(ipAddrs, &a)
-				}
+				for i := range data.Machines {
+					machines = append(machines, &data.Machines[i])
+				}
+				for i := range data.IPClaims {
+					ipClaims = append(ipClaims, &data.IPClaims[i])
+				}
+				for i := range data.IPAddresses {
+					ipAddrs = append(ipAddrs, &data.IPAddresses[i])
+				}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/asset/machines/worker.go` around lines 730 - 738, The loops that append
pointers currently capture the reused range variables (for _, m := range
data.Machines, for _, c := range data.IPClaims, for _, a := range
data.IPAddresses) causing pointer aliasing; fix by creating a new local copy
inside each loop (e.g., mCopy := m / cCopy := c / aCopy := a) and append the
address of that copy to machines, ipClaims, and ipAddrs respectively so each
slice element points to a distinct struct instance.

Copy link
Contributor Author

@patrickdillon patrickdillon Mar 18, 2026

Choose a reason for hiding this comment

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

No fix needed. The module uses Go 1.25, and since Go 1.22, range variables are scoped per iteration — each m, c, and a is a fresh variable, so &m, &c, and &a point to distinct instances. There's no aliasing bug here.

Copy link
Member

@tthvo tthvo Mar 18, 2026

Choose a reason for hiding this comment

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

See https://go.dev/doc/go1.22#language for release note that contains the fix for pointer aliasing.

Copy link

@coderabbitai coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


logrus.Debugf("Generated %v worker machines.", len(machines))

Expand Down Expand Up @@ -801,58 +804,17 @@ func (w *Worker) Generate(ctx context.Context, dependencies asset.Parents) error
return errors.Wrap(err, "failed to create MachineConfig manifests for worker machines")
}

w.MachineSetFiles = make([]*asset.File, len(machineSets))
padFormat := fmt.Sprintf("%%0%dd", len(fmt.Sprintf("%d", len(machineSets))))
for i, machineSet := range machineSets {
data, err := yaml.Marshal(machineSet)
if err != nil {
return errors.Wrapf(err, "marshal worker %d", i)
}

padded := fmt.Sprintf(padFormat, i)
w.MachineSetFiles[i] = &asset.File{
Filename: filepath.Join(directory, fmt.Sprintf(workerMachineSetFileName, padded)),
Data: data,
}
if w.MachineSetFiles, err = serialize(machineSets, workerMachineSetFileName, false); err != nil {
return fmt.Errorf("failed to serialize worker machine sets: %w", err)
}

w.IPClaimFiles = make([]*asset.File, len(ipClaims))
for i, claim := range ipClaims {
data, err := yaml.Marshal(claim)
if err != nil {
return errors.Wrapf(err, "marshal ip claim %v", claim.Name)
}

w.IPClaimFiles[i] = &asset.File{
Filename: filepath.Join(directory, fmt.Sprintf(ipClaimFileName, claim.Name)),
Data: data,
}
if w.IPClaimFiles, err = serialize(ipClaims, ipClaimFileName, true); err != nil {
return fmt.Errorf("failed to serialize worker ip claims: %w", err)
}

w.IPAddrFiles = make([]*asset.File, len(ipAddrs))
for i, address := range ipAddrs {
data, err := yaml.Marshal(address)
if err != nil {
return errors.Wrapf(err, "marshal ip claim %v", address.Name)
}

w.IPAddrFiles[i] = &asset.File{
Filename: filepath.Join(directory, fmt.Sprintf(ipAddressFileName, address.Name)),
Data: data,
}
if w.IPAddrFiles, err = serialize(ipAddrs, ipAddressFileName, true); err != nil {
return fmt.Errorf("failed to serialize worker ip addresses: %w", err)
}
w.MachineFiles = make([]*asset.File, len(machines))
for i, machineDef := range machines {
data, err := yaml.Marshal(machineDef)
if err != nil {
return errors.Wrapf(err, "marshal master %d", i)
}

padded := fmt.Sprintf(padFormat, i)
w.MachineFiles[i] = &asset.File{
Filename: filepath.Join(directory, fmt.Sprintf(workerMachineFileName, padded)),
Data: data,
}
if w.MachineFiles, err = serialize(machines, workerMachineFileName, false); err != nil {
return fmt.Errorf("failed to serialize worker machines: %w", err)
}
return nil
}
Expand Down Expand Up @@ -966,3 +928,33 @@ func (w *Worker) MachineSets() ([]machinev1beta1.MachineSet, error) {

return machineSets, nil
}

// serialize marshals a list of runtime.Object manifests into asset files.
// When useObjectName is true, the object's metadata name is used in the filename,
// e.g. "99_openshift-machine-api_claim-cluster-worker-0-claim-0-0.yaml".
// When false, a zero-padded index is used instead,
// e.g. "99_openshift-cluster-api_worker-machineset-0.yaml".
func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) {
Copy link
Member

@tthvo tthvo Mar 18, 2026

Choose a reason for hiding this comment

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

Suggested change
func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) {
func serialize(manifests []runtime.Object, fileNameTemplate string, useObjectName bool) ([]*asset.File, error) {

nit: It's actually a name template, right? Probably clearer to use it this way.

files := make([]*asset.File, len(manifests))
padFormat := fmt.Sprintf("%%0%dd", len(fmt.Sprintf("%d", len(manifests))))
for i, m := range manifests {
data, err := yaml.Marshal(m)
if err != nil {
return nil, fmt.Errorf("marshaling %s: %w", fmt.Sprintf(fileName, i), err)
}

key := fmt.Sprintf(padFormat, i)
if useObjectName {
accessor, err := meta.Accessor(m)
if err != nil {
return nil, fmt.Errorf("accessing object metadata: %w", err)
}
key = accessor.GetName()
}
files[i] = &asset.File{
Filename: filepath.Join(directory, fmt.Sprintf(fileName, key)),
Comment on lines +937 to +955
Copy link

@coderabbitai coderabbitai bot Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "serialize() error path:"
sed -n '937,945p' pkg/asset/machines/worker.go

echo
echo "Filename format definitions used by the changed call sites:"
rg -n -C1 'workerMachineSetFileName =|workerMachineFileName =|ipClaimFileName\s*=|ipAddressFileName\s*=' pkg/asset/machines

Repository: openshift/installer

Length of output: 1544

🏁 Script executed:

cat -n pkg/asset/machines/worker.go | sed -n '937,960p'

Repository: openshift/installer

Length of output: 974

Move key and filename computation before the marshal call to fix the error message.

Line 943 uses fmt.Sprintf(fileName, i) where fileName is a format string with %s placeholder (e.g., "99_openshift-cluster-api_worker-machines-%s.yaml"), but i is an int. This produces malformed error output like marshaling 99_openshift-cluster-api_worker-machines-%!s(int=0).yaml: ... instead of the actual filename. Build key and filename first, then reuse filename in both the error message and the File assignment.

Suggested fix 
 func serialize(manifests []runtime.Object, fileName string, useObjectName bool) ([]*asset.File, error) {
 	files := make([]*asset.File, len(manifests))
 	padFormat := fmt.Sprintf("%%0%dd", len(fmt.Sprintf("%d", len(manifests))))
 	for i, m := range manifests {
-		data, err := yaml.Marshal(m)
-		if err != nil {
-			return nil, fmt.Errorf("marshaling %s: %w", fmt.Sprintf(fileName, i), err)
-		}
-
 		key := fmt.Sprintf(padFormat, i)
 		if useObjectName {
 			accessor, err := meta.Accessor(m)
 			if err != nil {
 				return nil, fmt.Errorf("accessing object metadata: %w", err)
 			}
 			key = accessor.GetName()
 		}
+		filename := filepath.Join(directory, fmt.Sprintf(fileName, key))
+
+		data, err := yaml.Marshal(m)
+		if err != nil {
+			return nil, fmt.Errorf("marshaling %s: %w", filename, err)
+		}
 		files[i] = &asset.File{
-			Filename: filepath.Join(directory, fmt.Sprintf(fileName, key)),
+			Filename: filename,
 			Data:     data,
 		}
 	}
 	return files, nil
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/asset/machines/worker.go` around lines 937 - 955, In serialize, compute
the key and final filename before calling yaml.Marshal so error messages use the
actual filename: generate key (using padFormat or meta.Accessor when
useObjectName is true) and build filename := filepath.Join(directory,
fmt.Sprintf(fileName, key)) prior to marshaling, then call yaml.Marshal(m) and
on marshal error return fmt.Errorf("marshaling %s: %w", filename, err); finally
use that same filename when creating the asset.File (asset.File{Filename:
filename, ...}) to ensure consistent, correct error output and file assignment.

Copy link
Member

@tthvo tthvo Mar 18, 2026

Choose a reason for hiding this comment

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

+1 here. It should only affect error message, but makes sense to fix 👍

Copy link

@coderabbitai coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

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

@tthvo, thanks for the confirmation! Here's the planned fix:

  • Move key computation before yaml.Marshal in serialize()
  • Build filename from key before marshaling
  • Use filename in both the error message and the asset.File assignment

Failed to handle agent chat message. Please try again.

Data: data,
}
}
return files, nil
}