Skip to content

Testing changes for MCS Firstboot Pivot Failure Reporting v2 #6124

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

Draft
CourtCourt521 wants to merge 1 commit into
base: main
Choose a base branch
from
+73 −42
Draft

Testing changes for MCS Firstboot Pivot Failure Reporting v2 #6124

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions manifests/machineconfigserver/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ rules:
- apiGroups: ["route.openshift.io"]
resources: ["routes"]
verbs: ["get", "list"]
- apiGroups: ["config.openshift.io"]
resources: ["infrastructures"]
verbs: ["get", "list", "watch"]
Comment on lines +28 to +30
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for Infrastructure informer/lister usage that requires list+watch

# Search for Infrastructure informer/lister initialization
rg -nP --type=go -C3 '\bInfrastructure(Informer|Lister)\b'

# Search for watch operations on Infrastructure
rg -nP --type=go -C3 'Infrastructures\(\)\.Watch'

# Search for list operations on Infrastructure  
rg -nP --type=go -C3 'Infrastructures\(\)\.List'

Repository: openshift/machine-config-operator

Length of output: 18993

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Find where infraInformer is actually used (excluding vendor)
rg -n --type=go --glob '!vendor/**' '\binfraInformer\b' pkg

# 2) Look for starting/starting informers via factory or .Informer().Run/.HasSynced (excluding vendor)
rg -n --type=go --glob '!vendor/**' '\.Informer\(\)\.Run|\bRun\(.*infra|Start.*infra|WaitForCacheSync|HasSynced|informerFactory' pkg

# 3) Check if there are any direct API calls for Infrastructures().List/Watch in pkg (excluding vendor)
rg -n --type=go --glob '!vendor/**' 'Infrastructures\(\)\.(List|Watch)\b' pkg

Repository: openshift/machine-config-operator

Length of output: 23552

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for where the config informer factory is created/started (excluding vendor)
rg -n --type=go --glob '!vendor/**' '\bNewSharedInformerFactory\([^)]*configClient|\bStart\([^)]*\)' pkg/operator pkg/controller pkg/server pkg/daemon || true

# Specifically search for starting shared informer factories named/configure on configClient
rg -n --type=go --glob '!vendor/**' 'configinformers\.NewSharedInformerFactory|configinformers.*NewSharedInformerFactory' pkg/operator pkg/controller pkg/server pkg/daemon || true

# Find where infraInformer.Informer() comes from in operator.go to trace factory start
rg -n --type=go --glob '!vendor/**' 'infraInformer\s*[:=]' pkg/operator/operator.go || true
rg -n --type=go --glob '!vendor/**' 'Infrastructures\(\)\s*\.Informer\(\)' pkg/operator/operator.go || true

Repository: openshift/machine-config-operator

Length of output: 6087

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Trace config informer factory creation for infraInformer and whether it is started
rg -n --type=go --glob '!vendor/**' 'configinformers\.NewSharedInformerFactory\(' pkg/operator/operator.go pkg/server/cluster_server.go pkg/controller pkg/daemon pkg/osimagestream || true

# Find the sharedInformerFactory variable name used in pkg/server/cluster_server.go and see Start call scope
sed -n '1,220p' pkg/server/cluster_server.go | cat -n

# Show relevant section of pkg/operator/operator.go around where informer factories are started (look near Start())
rg -n --type=go --glob '!vendor/**' '\.Start\(stopCh\)|SharedInformerFactory\.Start' pkg/operator/operator.go
sed -n '430,560p' pkg/operator/operator.go | cat -n

Repository: openshift/machine-config-operator

Length of output: 16197

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search only in pkg/server for infra informer/lister usage and list/watch
rg -n --type=go --glob '!vendor/**' 'Infrastructures\(\)\.(Get|List|Watch)|infrastructure|infraInformer|infraLister' pkg/server || true

# Also quickly check for any shared informer factory involving config client in server
rg -n --type=go --glob '!vendor/**' 'configinformers|ConfigV1\(\)|ConfigClientOrDie\(' pkg/server || true

Repository: openshift/machine-config-operator

Length of output: 694

Tighten RBAC: drop list/watch for config.openshift.io/infrastructures

manifests/machineconfigserver/clusterrole.yaml should not grant list/watch: pkg/server/cluster_server.go only uses configClient.ConfigV1().Infrastructures().Get(ctx, "cluster", ...) to compute the MCS URL, and pkg/server contains no Infrastructures().List/Watch usage (or infra informer usage).

🔒 Tighten RBAC to minimum necessary verbs 
 - apiGroups: ["config.openshift.io"]
   resources: ["infrastructures"]
-  verbs: ["get", "list", "watch"]
+  verbs: ["get"]
🧰 Tools
🪛 Checkov (3.2.530)

[medium] 1-30: Minimize wildcard use in Roles and ClusterRoles

(CKV_K8S_49)

🤖 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 `@manifests/machineconfigserver/clusterrole.yaml` around lines 28 - 30, The
ClusterRole currently grants unnecessary verbs for the
config.openshift.io/infrastructures resource; remove "list" and "watch" and
retain only the "get" verb since pkg/server/cluster_server.go only calls
ConfigV1().Infrastructures().Get(...). Locate the rule that lists apiGroups:
["config.openshift.io"] and resources: ["infrastructures"] in
manifests/machineconfigserver/clusterrole.yaml and update the verbs array to
only include "get" so RBAC is tightened to the minimum required permission.

3 changes: 2 additions & 1 deletion pkg/daemon/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ const (
CurrentImageAnnotationKey = "machineconfiguration.openshift.io/currentImage"
// DesiredImageAnnotationKey is used to specify the desired OS image pullspec for a machine
DesiredImageAnnotationKey = "machineconfiguration.openshift.io/desiredImage"

// MachineConfigServerURLAnnotationKey stores the MCS base URL for status reporting
MachineConfigServerURLAnnotationKey = "machineconfiguration.openshift.io/machineConfigServerURL"
// CurrentMachineConfigAnnotationKey is used to fetch current MachineConfig for a machine
CurrentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig"
// DesiredMachineConfigAnnotationKey is used to specify the desired MachineConfig for a machine
Expand Down
33 changes: 1 addition & 32 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,8 @@ import (
"encoding/pem"
"errors"
"fmt"
"net"
"net/url"
"os"
"reflect"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -620,7 +617,7 @@ func (optr *Operator) syncRenderConfig(_ *renderConfig, _ *configv1.ClusterOpera
templatectrl.DockerRegistryKey: imgs.DockerRegistry,
}

ignitionHost, err := getIgnitionHost(&infra.Status)
ignitionHost, err := server.GetIgnitionHost(&infra.Status)
if err != nil {
return err
}
Expand Down Expand Up @@ -698,34 +695,6 @@ func (optr *Operator) getTrustedBundle(proxy *configv1.Proxy) ([]byte, error) {
return trustBundle, nil
}

func getIgnitionHost(infraStatus *configv1.InfrastructureStatus) (string, error) {
internalURL := infraStatus.APIServerInternalURL
internalURLParsed, err := url.Parse(internalURL)
if err != nil {
return "", err
}
securePortStr := strconv.Itoa(server.SecurePort)
ignitionHost := fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr)
if infraStatus.PlatformStatus != nil {
switch infraStatus.PlatformStatus.Type {
case configv1.BareMetalPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs[0], securePortStr)
case configv1.OpenStackPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs[0], securePortStr)
case configv1.OvirtPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs[0], securePortStr)
case configv1.VSpherePlatformType:
if infraStatus.PlatformStatus.VSphere != nil {
if len(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs) > 0 {
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs[0], securePortStr)
}
}
}
}

return ignitionHost, nil
}

func (optr *Operator) syncMachineConfigPools(config *renderConfig, _ *configv1.ClusterOperator) error {
generatedPools, err := manifests.GetMachineConfigPools()
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/bootstrap_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (bsc *bootstrapServer) GetConfig(cr poolRequest) (*runtime.RawExtension, er
addDataAndMaybeAppendToIgnition(cloudProviderCAPath, cc.Spec.CloudProviderCAData, &ignConf)

appenders := newAppendersBuilder(nil, bsc.kubeconfigFunc, bsc.certs, bsc.serverBaseDir).
WithNodeAnnotations(currConf, "").
WithNodeAnnotations(currConf, "", "").
build()

for _, a := range appenders {
Expand Down
22 changes: 21 additions & 1 deletion pkg/server/cluster_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/informers"
clientset "k8s.io/client-go/kubernetes"
corelisterv1 "k8s.io/client-go/listers/core/v1"
Expand Down Expand Up @@ -56,6 +57,7 @@ type clusterServer struct {

kubeconfigFunc kubeconfigFunc
apiserverURL string
mcsURL string
}

const minResyncPeriod = 20 * time.Minute
Expand Down Expand Up @@ -85,6 +87,7 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) {
machineConfigClient := clientsBuilder.MachineConfigClientOrDie("machine-config-shared-informer")
kubeClient := clientsBuilder.KubeClientOrDie("kube-client-shared-informer")
routeClient := clientsBuilder.RouteClientOrDie("route-client")
configClient := clientsBuilder.ConfigClientOrDie("config-client")
sharedInformerFactory := mcfginformers.NewSharedInformerFactory(machineConfigClient, resyncPeriod()())
kubeNamespacedSharedInformer := informers.NewSharedInformerFactoryWithOptions(kubeClient, resyncPeriod()(), informers.WithNamespace("openshift-machine-config-operator"))

Expand Down Expand Up @@ -112,6 +115,22 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) {
return nil, errors.New("failed to wait for cache sync")
}

// Fetch Infrastructure to compute MCS URL
// Non-fatal: if this fails, continue with empty mcsURL
var mcsURL string
ctx := context.Background()
infra, err := configClient.ConfigV1().Infrastructures().Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
klog.Warningf("Failed to get cluster infrastructure, continuing without MCS URL: %v", err)
} else {
ignitionHost, err := GetIgnitionHost(&infra.Status)
if err != nil {
klog.Warningf("Failed to compute ignition host, continuing without MCS URL: %v", err)
} else {
mcsURL = fmt.Sprintf("https://%s", ignitionHost)
}
}

return &clusterServer{
machineConfigPoolLister: mcpLister,
machineConfigLister: mcLister,
Expand All @@ -123,6 +142,7 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) {
routeclient: routeClient,
kubeconfigFunc: func() ([]byte, []byte, error) { return kubeconfigFromSecret(bootstrapTokenDir, apiserverURL, nil) },
apiserverURL: apiserverURL,
mcsURL: mcsURL,
}, nil
}

Expand Down Expand Up @@ -187,7 +207,7 @@ func (cs *clusterServer) GetConfig(cr poolRequest) (*runtime.RawExtension, error
desiredImage := cs.resolveDesiredImageForPool(mp)

appenders := newAppendersBuilder(cr.version, cs.kubeconfigFunc, []string{}, "").
WithNodeAnnotations(currConf, desiredImage).
WithNodeAnnotations(currConf, desiredImage, cs.mcsURL).
WithCustomAppender(appendDesiredOSImage(desiredImage)).
build()

Expand Down
48 changes: 43 additions & 5 deletions pkg/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package server

import (
"fmt"
"net"
"net/url"
"os"
"path/filepath"
"strconv"
"strings"

"github.com/clarketm/json"
Expand All @@ -14,6 +16,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/klog/v2"

configv1 "github.com/openshift/api/config/v1"
mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
build "github.com/openshift/machine-config-operator/pkg/controller/build/constants"
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
Expand Down Expand Up @@ -70,9 +73,9 @@ func newAppendersBuilder(version *semver.Version, kubeconfigFn kubeconfigFunc, c
}

// WithNodeAnnotations adds the node annotations appender with the specified config and image.
func (ab *appendersBuilder) WithNodeAnnotations(currMachineConfig, image string) *appendersBuilder {
func (ab *appendersBuilder) WithNodeAnnotations(currMachineConfig, image, mcsURL string) *appendersBuilder {
ab.customAppenders = append(ab.customAppenders, func(cfg *ign3types.Config, mc *mcfgv1.MachineConfig) error {
return appendNodeAnnotations(cfg, currMachineConfig, image, mc)
return appendNodeAnnotations(cfg, currMachineConfig, image, mcsURL, mc)
})
return ab
}
Expand Down Expand Up @@ -196,8 +199,8 @@ func appendKubeConfig(conf *ign3types.Config, f kubeconfigFunc) error {
return nil
}

func appendNodeAnnotations(conf *ign3types.Config, currConf, image string, mc *mcfgv1.MachineConfig) error {
anno, err := getNodeAnnotation(currConf, image, mc)
func appendNodeAnnotations(conf *ign3types.Config, currConf, image, mcsURL string, mc *mcfgv1.MachineConfig) error {
anno, err := getNodeAnnotation(currConf, image, mcsURL, mc)
if err != nil {
return err
}
Expand All @@ -208,14 +211,18 @@ func appendNodeAnnotations(conf *ign3types.Config, currConf, image string, mc *m
return nil
}

func getNodeAnnotation(conf, image string, mc *mcfgv1.MachineConfig) (string, error) {
func getNodeAnnotation(conf, image, mcsURL string, mc *mcfgv1.MachineConfig) (string, error) {
nodeAnnotations := map[string]string{
daemonconsts.CurrentMachineConfigAnnotationKey: conf,
daemonconsts.DesiredMachineConfigAnnotationKey: conf,
daemonconsts.FirstPivotMachineConfigAnnotationKey: conf,
daemonconsts.MachineConfigDaemonStateAnnotationKey: daemonconsts.MachineConfigDaemonStateDone,
}

if mcsURL != "" {
nodeAnnotations[daemonconsts.MachineConfigServerURLAnnotationKey] = mcsURL
}

// Determine which image to use:
// 1. Pre-built image from MC annotations (install-time hybrid OCL) takes priority
// 2. Dynamically resolved image parameter (runtime scaling)
Expand Down Expand Up @@ -308,3 +315,34 @@ func addDataAndMaybeAppendToIgnition(path string, data []byte, ignConf *ign3type
appendFileToIgnition(ignConf, path, (string(data)))
}
}

// GetIgnitionHost computes the MCS hostname:port for serving Ignition configs
// from the cluster's Infrastructure status. Platform-specific IP addresses
// take precedence over the parsed APIServerInternalURL hostname.
func GetIgnitionHost(infraStatus *configv1.InfrastructureStatus) (string, error) {
internalURL := infraStatus.APIServerInternalURL
internalURLParsed, err := url.Parse(internalURL)
if err != nil {
return "", err
}
securePortStr := strconv.Itoa(SecurePort)
ignitionHost := fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr)
if infraStatus.PlatformStatus != nil {
switch infraStatus.PlatformStatus.Type {
case configv1.BareMetalPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs[0], securePortStr)
case configv1.OpenStackPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs[0], securePortStr)
case configv1.OvirtPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs[0], securePortStr)
case configv1.VSpherePlatformType:
if infraStatus.PlatformStatus.VSphere != nil {
if len(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs) > 0 {
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.VSphere.APIServerInternalIPs[0], securePortStr)
}
}
}
}

return ignitionHost, nil
Comment on lines +322 to +347
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Harden GetIgnitionHost (IPv6 host:port + panic safety)

  • Replace the fallback fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr) with net.JoinHostPort(...) so IPv6 literals get valid brackets.
  • Add infraStatus == nil handling and check len(APIServerInternalIPs) > 0 before indexing [0] in the BareMetal/OpenStack/Ovirt/VSphere branches to avoid panics.
🤖 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/server/server.go` around lines 322 - 347, GetIgnitionHost currently uses
fmt.Sprintf("%s:%s", internalURLParsed.Hostname(), securePortStr) which breaks
for IPv6 and assumes infraStatus and API IP slices are non-nil/non-empty; change
the fallback to use net.JoinHostPort(internalURLParsed.Hostname(),
securePortStr) and add an initial nil check for infraStatus to return an error
if nil, then in each platform branch (BareMetal/APIServerInternalIPs,
OpenStack/APIServerInternalIPs, Ovirt/APIServerInternalIPs,
VSphere.APIServerInternalIPs) check that the slice exists and len(...)>0 before
indexing [0] to avoid panics, keeping the SecurePort and ignitionHost assignment
logic otherwise the same.

}
4 changes: 2 additions & 2 deletions pkg/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func TestBootstrapServer(t *testing.T) {
if err != nil {
t.Fatalf("unexpected error while appending file to ignition: %v", err)
}
anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", mc)
anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", "https://api-int.test.example.com:22623", mc)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 2, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

These tests still don’t verify the new MCS URL annotation.

The hardcoded URL only goes into a local mcIgnCfg that never drives the final assertions, and the servers under test are not configured to emit that URL. These cases will keep passing even if MachineConfigServerURLAnnotationKey is never written.

Also applies to: 383-383

🤖 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/server/server_test.go` at line 188, The tests currently call
getNodeAnnotation and build a local mcIgnCfg but never assert that
MachineConfigServerURLAnnotationKey was written or configure the test servers to
emit that MCS URL; update the tests to (1) configure the test server(s) used in
these cases to return/emit the expected MCS URL, (2) set the expected URL value
and call getNodeAnnotation(mp.Status.Configuration.Name, "", "<expected-url>",
mc) as you already do, and (3) add explicit assertions that the returned
annotation map contains MachineConfigServerURLAnnotationKey with the expected
URL value (use the same unique symbols: getNodeAnnotation,
MachineConfigServerURLAnnotationKey, and mcIgnCfg to locate where to change
behavior and where to add the assertion). Ensure both occurrences (around lines
shown) are updated consistently.

if err != nil {
t.Fatalf("unexpected error while creating annotations err: %v", err)
}
Expand Down Expand Up @@ -380,7 +380,7 @@ func TestClusterServer(t *testing.T) {
if err != nil {
t.Fatalf("unexpected error while appending file to ignition: %v", err)
}
anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", mc)
anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "", "https://api-int.test.example.com:22623", mc)
if err != nil {
t.Fatalf("unexpected error while creating annotations err: %v", err)
}
Expand Down