diff --git a/manifests/machineconfigserver/clusterrole.yaml b/manifests/machineconfigserver/clusterrole.yaml index c8f406d8c6..76ca884564 100644 --- a/manifests/machineconfigserver/clusterrole.yaml +++ b/manifests/machineconfigserver/clusterrole.yaml @@ -9,6 +9,9 @@ rules: - apiGroups: ["machineconfiguration.openshift.io"] resources: ["controllerconfigs"] verbs: ["get", "watch", "list"] +- apiGroups: ["machineconfiguration.openshift.io"] + resources: ["machineosconfigs", "machineosbuilds"] + verbs: ["get", "list", "watch"] - apiGroups: ["security.openshift.io"] resourceNames: ["hostnetwork"] resources: ["securitycontextconstraints"] @@ -16,3 +19,9 @@ rules: - apiGroups: [""] resources: ["configmaps"] verbs: ["get", "list", "watch"] +- apiGroups: [""] + resources: ["services"] + verbs: ["get", "list"] +- apiGroups: ["route.openshift.io"] + resources: ["routes"] + verbs: ["get", "list"] diff --git a/pkg/controller/build/reconciler.go b/pkg/controller/build/reconciler.go index 90f9faf7df..50182b2828 100644 --- a/pkg/controller/build/reconciler.go +++ b/pkg/controller/build/reconciler.go @@ -999,7 +999,7 @@ func (b *buildReconciler) deleteMOSBImage(ctx context.Context, mosb *mcfgv1.Mach } image := string(mosb.Spec.RenderedImagePushSpec) - isOpenShiftRegistry, err := b.isOpenShiftRegistry(image) + isOpenShiftRegistry, err := ctrlcommon.IsOpenShiftRegistry(ctx, image, b.kubeclient, b.routeclient) if err != nil { return err } @@ -1039,53 +1039,6 @@ func (b *buildReconciler) deleteMOSBImage(ctx context.Context, mosb *mcfgv1.Mach return nil } -// getInternalRegistryHostnames discovers OpenShift internal registry hostnames -func (b *buildReconciler) getInternalRegistryHostnames(ctx context.Context) ([]string, error) { - var hostnames []string - - // Get the list of services in the openshift-image-registry namespace (cluster-local) - services, err := b.kubeclient.CoreV1().Services("openshift-image-registry").List(ctx, metav1.ListOptions{}) - if err != nil { - return nil, err - } - for _, svc := range services.Items { - clusterHostname := fmt.Sprintf("%s.%s.svc", svc.Name, svc.Namespace) - if len(svc.Spec.Ports) > 0 { - port := svc.Spec.Ports[0].Port - hostnames = append(hostnames, fmt.Sprintf("%s:%d", clusterHostname, port)) - } else { - hostnames = append(hostnames, clusterHostname) - } - } - - // Get the list of routes in the openshift-image-registry namespace (external access) - routes, err := b.routeclient.RouteV1().Routes("openshift-image-registry").List(ctx, metav1.ListOptions{}) - if err != nil { - return nil, err - } - for _, route := range routes.Items { - if route.Spec.Host != "" { - hostnames = append(hostnames, route.Spec.Host) - } - } - - return hostnames, nil -} - -// isOpenShiftRegistry checks if the imageRef points to one of the known internal hostnames -func (b *buildReconciler) isOpenShiftRegistry(imageRef string) (bool, error) { - registryHosts, err := b.getInternalRegistryHostnames(context.TODO()) - if err != nil { - return false, err - } - for _, host := range registryHosts { - if strings.HasPrefix(imageRef, host) { - return true, nil - } - } - return false, nil -} - // Finds and deletes any other running builds for a given MachineOSConfig. func (b *buildReconciler) deleteOtherBuildsForMachineOSConfig(ctx context.Context, newMosb *mcfgv1.MachineOSBuild, mosc *mcfgv1.MachineOSConfig) error { mosbList, err := b.getMachineOSBuildsForMachineOSConfig(mosc) diff --git a/pkg/controller/common/registry_utils.go b/pkg/controller/common/registry_utils.go new file mode 100644 index 0000000000..08eadbc844 --- /dev/null +++ b/pkg/controller/common/registry_utils.go @@ -0,0 +1,61 @@ +package common + +import ( + "context" + "fmt" + "strings" + + routeclientset "github.com/openshift/client-go/route/clientset/versioned" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientset "k8s.io/client-go/kubernetes" +) + +// GetInternalRegistryHostnames discovers OpenShift internal registry hostnames +// by querying Services and Routes in the openshift-image-registry namespace. +func GetInternalRegistryHostnames(ctx context.Context, kubeclient clientset.Interface, routeclient routeclientset.Interface) ([]string, error) { + var hostnames []string + + // Get the list of services in the openshift-image-registry namespace (cluster-local) + services, err := kubeclient.CoreV1().Services("openshift-image-registry").List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, err + } + for _, svc := range services.Items { + clusterHostname := fmt.Sprintf("%s.%s.svc", svc.Name, svc.Namespace) + if len(svc.Spec.Ports) > 0 { + port := svc.Spec.Ports[0].Port + hostnames = append(hostnames, fmt.Sprintf("%s:%d", clusterHostname, port)) + } else { + hostnames = append(hostnames, clusterHostname) + } + } + + // Get the list of routes in the openshift-image-registry namespace (external access) + routes, err := routeclient.RouteV1().Routes("openshift-image-registry").List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, err + } + for _, route := range routes.Items { + if route.Spec.Host != "" { + hostnames = append(hostnames, route.Spec.Host) + } + } + + return hostnames, nil +} + +// IsOpenShiftRegistry checks if the imageRef points to one of the known internal registry hostnames +func IsOpenShiftRegistry(ctx context.Context, imageRef string, kubeclient clientset.Interface, routeclient routeclientset.Interface) (bool, error) { + registryHosts, err := GetInternalRegistryHostnames(ctx, kubeclient, routeclient) + if err != nil { + return false, err + } + + for _, host := range registryHosts { + if strings.HasPrefix(imageRef, host) { + return true, nil + } + } + + return false, nil +} diff --git a/pkg/controller/common/registry_utils_test.go b/pkg/controller/common/registry_utils_test.go new file mode 100644 index 0000000000..168e4d71eb --- /dev/null +++ b/pkg/controller/common/registry_utils_test.go @@ -0,0 +1,267 @@ +package common + +import ( + "context" + "testing" + + routev1 "github.com/openshift/api/route/v1" + routefake "github.com/openshift/client-go/route/clientset/versioned/fake" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" +) + +func TestGetInternalRegistryHostnames(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + services []runtime.Object + routes []runtime.Object + expectedHostnames []string + errExpected bool + }{ + { + name: "Registry with service and route", + services: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 5000}, + }, + }, + }, + }, + routes: []runtime.Object{ + &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default-route", + Namespace: "openshift-image-registry", + }, + Spec: routev1.RouteSpec{ + Host: "default-route-openshift-image-registry.apps.example.com", + }, + }, + }, + expectedHostnames: []string{ + "image-registry.openshift-image-registry.svc:5000", + "default-route-openshift-image-registry.apps.example.com", + }, + }, + { + name: "Registry with service without port", + services: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{}, + }, + }, + }, + routes: []runtime.Object{}, + expectedHostnames: []string{ + "image-registry.openshift-image-registry.svc", + }, + }, + { + name: "Registry not deployed (empty namespace)", + services: []runtime.Object{}, + routes: []runtime.Object{}, + expectedHostnames: []string{}, + }, + { + name: "Multiple services and routes", + services: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 5000}, + }, + }, + }, + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry-internal", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 5001}, + }, + }, + }, + }, + routes: []runtime.Object{ + &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "external-route", + Namespace: "openshift-image-registry", + }, + Spec: routev1.RouteSpec{ + Host: "external-route.apps.example.com", + }, + }, + }, + expectedHostnames: []string{ + "image-registry.openshift-image-registry.svc:5000", + "image-registry-internal.openshift-image-registry.svc:5001", + "external-route.apps.example.com", + }, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + kubeclient := fake.NewSimpleClientset(testCase.services...) + routeclient := routefake.NewSimpleClientset(testCase.routes...) + + hostnames, err := GetInternalRegistryHostnames(context.TODO(), kubeclient, routeclient) + + if testCase.errExpected { + assert.Error(t, err) + return + } + + require.NoError(t, err) + assert.ElementsMatch(t, testCase.expectedHostnames, hostnames) + }) + } +} + +func TestIsOpenShiftRegistry(t *testing.T) { + t.Parallel() + + // Set up a standard registry environment + services := []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 5000}, + }, + }, + }, + } + + routes := []runtime.Object{ + &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default-route", + Namespace: "openshift-image-registry", + }, + Spec: routev1.RouteSpec{ + Host: "default-route-openshift-image-registry.apps.example.com", + }, + }, + } + + testCases := []struct { + name string + imageRef string + isInternal bool + errExpected bool + }{ + { + name: "Internal registry via service hostname", + imageRef: "image-registry.openshift-image-registry.svc:5000/openshift/custom-image:latest", + isInternal: true, + }, + { + name: "Internal registry via route hostname", + imageRef: "default-route-openshift-image-registry.apps.example.com/openshift/custom-image:latest", + isInternal: true, + }, + { + name: "External registry (Quay)", + imageRef: "quay.io/openshift/custom-image:latest", + isInternal: false, + }, + { + name: "External registry (Docker Hub)", + imageRef: "docker.io/library/nginx:latest", + isInternal: false, + }, + { + name: "External private registry", + imageRef: "my-private-registry.example.com:5000/app/image:v1", + isInternal: false, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + kubeclient := fake.NewSimpleClientset(services...) + routeclient := routefake.NewSimpleClientset(routes...) + + isInternal, err := IsOpenShiftRegistry(context.TODO(), testCase.imageRef, kubeclient, routeclient) + + if testCase.errExpected { + assert.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, testCase.isInternal, isInternal) + }) + } +} + +func TestIsOpenShiftRegistryWhenRegistryNotDeployed(t *testing.T) { + t.Parallel() + + // Empty clients - simulating registry not deployed + kubeclient := fake.NewSimpleClientset() + routeclient := routefake.NewSimpleClientset() + + testCases := []struct { + name string + imageRef string + }{ + { + name: "Image that would be internal if registry existed", + imageRef: "image-registry.openshift-image-registry.svc:5000/openshift/custom-image:latest", + }, + { + name: "External registry image", + imageRef: "quay.io/openshift/custom-image:latest", + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + isInternal, err := IsOpenShiftRegistry(context.TODO(), testCase.imageRef, kubeclient, routeclient) + + // Should not error when registry is not deployed + require.NoError(t, err) + + // Should return false (not internal) when registry not deployed + assert.False(t, isInternal, "Should treat as external registry when OpenShift registry is not deployed") + }) + } +} diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 85e13b2b04..084a42cc67 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -2174,7 +2174,12 @@ func (dn *Daemon) checkStateOnFirstRun() error { // Bootstrapping state is when we have the node annotations file if state.bootstrapping { - targetOSImageURL := state.currentConfig.Spec.OSImageURL + // During bootstrap, prefer the image from node annotations (if set) over the MC from cluster lister + targetOSImageURL := state.currentImage + if targetOSImageURL == "" { + // Fall back to MC's OSImageURL if no image annotation was provided + targetOSImageURL = state.currentConfig.Spec.OSImageURL + } osMatch := dn.checkOS(targetOSImageURL) if !osMatch { logSystem("Bootstrap pivot required to: %s", targetOSImageURL) diff --git a/pkg/server/cluster_server.go b/pkg/server/cluster_server.go index c42e89f418..280d834c69 100644 --- a/pkg/server/cluster_server.go +++ b/pkg/server/cluster_server.go @@ -1,6 +1,7 @@ package server import ( + "context" "encoding/json" "errors" "fmt" @@ -9,14 +10,19 @@ import ( "path/filepath" "time" + ign3types "github.com/coreos/ignition/v2/config/v3_5/types" yaml "github.com/ghodss/yaml" + mcfgv1 "github.com/openshift/api/machineconfiguration/v1" mcfginformers "github.com/openshift/client-go/machineconfiguration/informers/externalversions" + routeclientset "github.com/openshift/client-go/route/clientset/versioned" "github.com/openshift/machine-config-operator/internal/clients" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/informers" + clientset "k8s.io/client-go/kubernetes" corelisterv1 "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" clientcmdv1 "k8s.io/client-go/tools/clientcmd/api/v1" @@ -42,6 +48,11 @@ type clusterServer struct { machineConfigLister v1.MachineConfigLister controllerConfigLister v1.ControllerConfigLister configMapLister corelisterv1.ConfigMapLister + machineOSConfigLister v1.MachineOSConfigLister + machineOSBuildLister v1.MachineOSBuildLister + + kubeclient clientset.Interface + routeclient routeclientset.Interface kubeconfigFunc kubeconfigFunc apiserverURL string @@ -73,26 +84,31 @@ 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") sharedInformerFactory := mcfginformers.NewSharedInformerFactory(machineConfigClient, resyncPeriod()()) kubeNamespacedSharedInformer := informers.NewFilteredSharedInformerFactory(kubeClient, resyncPeriod()(), "openshift-machine-config-operator", nil) - mcpInformer, mcInformer, ccInformer, cmInformer := + mcpInformer, mcInformer, ccInformer, cmInformer, moscInformer, mosbInformer := sharedInformerFactory.Machineconfiguration().V1().MachineConfigPools(), sharedInformerFactory.Machineconfiguration().V1().MachineConfigs(), sharedInformerFactory.Machineconfiguration().V1().ControllerConfigs(), - kubeNamespacedSharedInformer.Core().V1().ConfigMaps() - mcpLister, mcLister, ccLister, cmLister := mcpInformer.Lister(), mcInformer.Lister(), ccInformer.Lister(), cmInformer.Lister() - mcpListerHasSynced, mcListerHasSynced, ccListerHasSynced, cmListerHasSynced := + kubeNamespacedSharedInformer.Core().V1().ConfigMaps(), + sharedInformerFactory.Machineconfiguration().V1().MachineOSConfigs(), + sharedInformerFactory.Machineconfiguration().V1().MachineOSBuilds() + mcpLister, mcLister, ccLister, cmLister, moscLister, mosbLister := mcpInformer.Lister(), mcInformer.Lister(), ccInformer.Lister(), cmInformer.Lister(), moscInformer.Lister(), mosbInformer.Lister() + mcpListerHasSynced, mcListerHasSynced, ccListerHasSynced, cmListerHasSynced, moscListerHasSynced, mosbListerHasSynced := mcpInformer.Informer().HasSynced, mcInformer.Informer().HasSynced, ccInformer.Informer().HasSynced, - cmInformer.Informer().HasSynced + cmInformer.Informer().HasSynced, + moscInformer.Informer().HasSynced, + mosbInformer.Informer().HasSynced var informerStopCh chan struct{} go sharedInformerFactory.Start(informerStopCh) go kubeNamespacedSharedInformer.Start(informerStopCh) - if !cache.WaitForCacheSync(informerStopCh, mcpListerHasSynced, mcListerHasSynced, ccListerHasSynced, cmListerHasSynced) { + if !cache.WaitForCacheSync(informerStopCh, mcpListerHasSynced, mcListerHasSynced, ccListerHasSynced, cmListerHasSynced, moscListerHasSynced, mosbListerHasSynced) { return nil, errors.New("failed to wait for cache sync") } @@ -101,6 +117,10 @@ func NewClusterServer(kubeConfig, apiserverURL string) (Server, error) { machineConfigLister: mcLister, controllerConfigLister: ccLister, configMapLister: cmLister, + machineOSConfigLister: moscLister, + machineOSBuildLister: mosbLister, + kubeclient: kubeClient, + routeclient: routeClient, kubeconfigFunc: func() ([]byte, []byte, error) { return kubeconfigFromSecret(bootstrapTokenDir, apiserverURL, nil) }, apiserverURL: apiserverURL, }, nil @@ -163,7 +183,27 @@ func (cs *clusterServer) GetConfig(cr poolRequest) (*runtime.RawExtension, error addDataAndMaybeAppendToIgnition(caBundleFilePath, cc.Spec.KubeAPIServerServingCAData, &ignConf) addDataAndMaybeAppendToIgnition(cloudProviderCAPath, cc.Spec.CloudProviderCAData, &ignConf) - appenders := getAppenders(currConf, cr.version, cs.kubeconfigFunc, []string{}, "") + + desiredImage := cs.resolveDesiredImageForPool(mp) + klog.Infof("desiredImage is found to be %s", desiredImage) + + appenders := []appenderFunc{ + func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { + return appendNodeAnnotations(cfg, currConf, desiredImage) + }, + func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { + return appendKubeConfig(cfg, cs.kubeconfigFunc) + }, + appendInitialMachineConfig, + func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { return appendCerts(cfg, []string{}, "") }, + // Inject desired image into the MC + appendDesiredOSImage(desiredImage), + // Must be last + func(cfg *ign3types.Config, mc *mcfgv1.MachineConfig) error { + return appendEncapsulated(cfg, mc, cr.version) + }, + } + for _, a := range appenders { if err := a(&ignConf, mc); err != nil { return nil, err @@ -224,3 +264,129 @@ func kubeconfigFromSecret(secretDir, apiserverURL string, caData []byte) ([]byte } return kcData, caData, nil } + +// Finds the MOSC that targets this MCO and verfies that the MOSC has an image in status +// locates the matching MOSB for the MCP's current or next rendered MC and confirms build succeeded +// and returns the image pullspec when its ready +func (cs *clusterServer) resolveDesiredImageForPool(pool *mcfgv1.MachineConfigPool) string { + // If listers are not initialized (e.g., in tests or clusters without layering), return empty + if cs.machineOSConfigLister == nil || cs.machineOSBuildLister == nil { + return "" + } + + // Find MachineOSConfig for this pool + moscList, err := cs.machineOSConfigLister.List(labels.Everything()) + if err != nil { + // MOSC resources not available + klog.Infof("Could not list MachineOSConfigs for pool %s: %v", pool.Name, err) + return "" + } + + // TODO(dkhater): Simplify to directly get MOSC using pool name with admin_ack enforcing MOSC name == pool name. + // Versions 4.18.21-4.18.23 lack OCPBUGS-60904 validation (MOSCs could have non-matching names). + // Can use admin_ack (e.g., at 4.23) to block upgrades until MOSCs are corrected, then replace List+filter with: mosc, err := cs.machineOSConfigLister.Get(pool.Name) + // See: https://issues.redhat.com/browse/OCPBUGS-60904 for validation bug. + // See: https://issues.redhat.com/browse/MCO-1935 for cleanup story. + + var mosc *mcfgv1.MachineOSConfig + for _, config := range moscList { + if config.Spec.MachineConfigPool.Name == pool.Name { + mosc = config + break + } + } + + // No MOSC for this pool - not layered + if mosc == nil { + return "" + } + + // Check if image is ready in MOSC status + moscState := ctrlcommon.NewMachineOSConfigState(mosc) + if !moscState.HasOSImage() { + klog.Infof("Pool %s has MachineOSConfig but image not ready yet", pool.Name) + return "" + } + + // Also verify the corresponding MOSB is successful to ensure we don't serve an image that hasn't been built yet + // + // Note: We cannot directly use the MOSB reference from mosc.Status.MachineOSBuild here because + // there can be multiple MOSBs for a single MOSC (one per rendered MachineConfig). The MOSC status + // points to the latest successful build, but we need the MOSB matching the pool's rollout state. + // We use pool.Status.Configuration.Name (old config) when UpdatedMachineCount == 0, and + // pool.Spec.Configuration.Name (new config) when UpdatedMachineCount > 0. This matches the existing + // MCS behavior for non-layered nodes and ensures we serve the correct image during rollouts. + // + // TODO(dkhater): For added safety during node scale-up, we should only serve the new build if it's + // both successful AND at least one node has already updated to it. This would more closely match + // the existing MCS rollout behavior and provide better safety, though it would result in additional + // reboots when scaling nodes during an update. + // See https://issues.redhat.com/browse/MCO-1940 for tracking this improvement. + mosbList, err := cs.machineOSBuildLister.List(labels.Everything()) + if err != nil { + klog.Infof("Could not list MachineOSBuilds for pool %s: %v", pool.Name, err) + return "" + } + + var currentConf string + if pool.Status.UpdatedMachineCount > 0 { + currentConf = pool.Spec.Configuration.Name + } else { + currentConf = pool.Status.Configuration.Name + } + + var mosb *mcfgv1.MachineOSBuild + for _, build := range mosbList { + if build.Spec.MachineOSConfig.Name == mosc.Name && + build.Spec.MachineConfig.Name == currentConf { + mosb = build + break + } + } + + if mosb == nil { + klog.Infof("Pool %s has MachineOSConfig but no matching MachineOSBuild for MC %s", pool.Name, currentConf) + return "" + } + + // Check build is successful + mosbState := ctrlcommon.NewMachineOSBuildState(mosb) + if !mosbState.IsBuildSuccess() { + klog.Infof("Pool %s has MachineOSBuild but build not successful yet", pool.Name) + return "" + } + + imageSpec := moscState.GetOSImage() + + // Don't serve internal registry images during bootstrap because cluster DNS is not available yet + // New nodes will bootstrap with base image, then update to layered image after joining cluster (2 reboots) + // External registries (Quay, etc.) will still get the 1-reboot optimization + isInternal, err := ctrlcommon.IsOpenShiftRegistry(context.TODO(), imageSpec, cs.kubeclient, cs.routeclient) + if err != nil { + klog.Errorf("Failed to check if image is internal registry for pool %s: %v", pool.Name, err) + return "" + } + + if isInternal { + klog.V(4).Infof("New nodes will bootstrap with base image, then update to layered image after joining cluster") + return "" + } + + klog.Infof("Resolved layered image for pool %s: %s", pool.Name, imageSpec) + return imageSpec +} + +// appendDesiredOSImage mutates the MC to include the desired OS image. +// This runs appendEncapsulated so mcd-firstboot sees the image on first boot. +func appendDesiredOSImage(desired string) appenderFunc { + return func(_ *ign3types.Config, mc *mcfgv1.MachineConfig) error { + if desired == "" { + return nil + } + if mc.Spec.OSImageURL != desired { + klog.Infof("overriding MC OSImageURL: %q -> %q", mc.Spec.OSImageURL, desired) + mc.Spec.OSImageURL = desired + } + return nil + } +} diff --git a/pkg/server/server.go b/pkg/server/server.go index 4d730f0fbb..4f122ad3f0 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -50,7 +50,7 @@ func getAppenders(currMachineConfig string, version *semver.Version, f kubeconfi appenders := []appenderFunc{ // append machine annotations file. func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { - return appendNodeAnnotations(cfg, currMachineConfig) + return appendNodeAnnotations(cfg, currMachineConfig, "") }, // append kubeconfig. func(cfg *ign3types.Config, _ *mcfgv1.MachineConfig) error { return appendKubeConfig(cfg, f) }, @@ -153,8 +153,8 @@ func appendKubeConfig(conf *ign3types.Config, f kubeconfigFunc) error { return nil } -func appendNodeAnnotations(conf *ign3types.Config, currConf string) error { - anno, err := getNodeAnnotation(currConf) +func appendNodeAnnotations(conf *ign3types.Config, currConf, image string) error { + anno, err := getNodeAnnotation(currConf, image) if err != nil { return err } @@ -165,12 +165,17 @@ func appendNodeAnnotations(conf *ign3types.Config, currConf string) error { return nil } -func getNodeAnnotation(conf string) (string, error) { +func getNodeAnnotation(conf, image string) (string, error) { nodeAnnotations := map[string]string{ daemonconsts.CurrentMachineConfigAnnotationKey: conf, daemonconsts.DesiredMachineConfigAnnotationKey: conf, daemonconsts.MachineConfigDaemonStateAnnotationKey: daemonconsts.MachineConfigDaemonStateDone, } + // If image is provided, include image annotations + if image != "" { + nodeAnnotations[daemonconsts.CurrentImageAnnotationKey] = image + nodeAnnotations[daemonconsts.DesiredImageAnnotationKey] = image + } contents, err := json.Marshal(nodeAnnotations) if err != nil { return "", fmt.Errorf("could not marshal node annotations, err: %w", err) diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 6a3538ed6d..04d03b797a 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -21,8 +21,12 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + routev1 "github.com/openshift/api/route/v1" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" + routefake "github.com/openshift/client-go/route/clientset/versioned/fake" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants" ) @@ -159,7 +163,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) + anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "") if err != nil { t.Fatalf("unexpected error while creating annotations err: %v", err) } @@ -354,7 +358,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) + anno, err := getNodeAnnotation(mp.Status.Configuration.Name, "") if err != nil { t.Fatalf("unexpected error while creating annotations err: %v", err) } @@ -534,3 +538,245 @@ func TestKubeconfigFromSecret(t *testing.T) { }) } } + +// TestResolveDesiredImageForPool tests the 1-reboot vs 2-reboot behavior for layered images. +// This verifies that: +// - External registry images are served during bootstrap (1 reboot optimization) +// - Internal registry images are NOT served during bootstrap (2 reboots - safe fallback) +// - Registry detection failures default to safe fallback (2 reboots) +func TestResolveDesiredImageForPool(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + poolName string + moscExists bool + mosbExists bool + mosbSuccessful bool + registryInternal bool + expectedImageURL string + expectedRebootPath string + }{ + { + name: "External registry image - 1 reboot path", + poolName: "worker", + moscExists: true, + mosbExists: true, + mosbSuccessful: true, + registryInternal: false, + expectedImageURL: "quay.io/openshift/custom-layered-image@sha256:abc123", + expectedRebootPath: "1-reboot (layered image served during bootstrap)", + }, + { + name: "Internal registry image - 2 reboot path", + poolName: "worker", + moscExists: true, + mosbExists: true, + mosbSuccessful: true, + registryInternal: true, + expectedImageURL: "", + expectedRebootPath: "2-reboot (base image served, then update to layered after joining cluster)", + }, + { + name: "No MOSC - no layering", + poolName: "worker", + moscExists: false, + expectedImageURL: "", + expectedRebootPath: "Base image only (pool not using layering)", + }, + { + name: "MOSB not successful - wait for build", + poolName: "worker", + moscExists: true, + mosbExists: true, + mosbSuccessful: false, + expectedImageURL: "", + expectedRebootPath: "Base image (wait for build to complete)", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Create test pool + pool := &mcfgv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.poolName, + }, + Spec: mcfgv1.MachineConfigPoolSpec{ + Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ + ObjectReference: corev1.ObjectReference{ + Name: "rendered-worker-new", + }, + }, + }, + Status: mcfgv1.MachineConfigPoolStatus{ + Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ + ObjectReference: corev1.ObjectReference{ + Name: "rendered-worker-old", + }, + }, + UpdatedMachineCount: 1, // At least one node updated, so use new config + }, + } + + // Create mock listers + var moscList []*mcfgv1.MachineOSConfig + var mosbList []*mcfgv1.MachineOSBuild + + if tc.moscExists { + mosc := &mcfgv1.MachineOSConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.poolName, + }, + Spec: mcfgv1.MachineOSConfigSpec{ + MachineConfigPool: mcfgv1.MachineConfigPoolReference{ + Name: tc.poolName, + }, + }, + Status: mcfgv1.MachineOSConfigStatus{ + CurrentImagePullSpec: mcfgv1.ImageDigestFormat("quay.io/openshift/custom-layered-image@sha256:abc123"), + }, + } + + if tc.registryInternal { + mosc.Status.CurrentImagePullSpec = mcfgv1.ImageDigestFormat("image-registry.openshift-image-registry.svc:5000/openshift/custom-layered-image@sha256:abc123") + } + + moscList = append(moscList, mosc) + } + + if tc.mosbExists { + mosb := &mcfgv1.MachineOSBuild{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.poolName + "-" + pool.Spec.Configuration.ObjectReference.Name, + }, + Spec: mcfgv1.MachineOSBuildSpec{ + MachineOSConfig: mcfgv1.MachineOSConfigReference{ + Name: tc.poolName, + }, + MachineConfig: mcfgv1.MachineConfigReference{ + Name: pool.Spec.Configuration.ObjectReference.Name, + }, + }, + Status: mcfgv1.MachineOSBuildStatus{}, + } + + if tc.mosbSuccessful { + // Set conditions to indicate build success + mosb.Status.Conditions = []metav1.Condition{ + { + Type: "Succeeded", + Status: metav1.ConditionTrue, + }, + } + + if tc.registryInternal { + mosb.Status.DigestedImagePushSpec = mcfgv1.ImageDigestFormat("image-registry.openshift-image-registry.svc:5000/openshift/custom-layered-image@sha256:abc123") + } else { + mosb.Status.DigestedImagePushSpec = mcfgv1.ImageDigestFormat("quay.io/openshift/custom-layered-image@sha256:abc123") + } + } + + mosbList = append(mosbList, mosb) + } + + moscLister := &mockMOSCLister{configs: moscList} + mosbLister := &mockMOSBLister{builds: mosbList} + + // Create fake k8s clients for registry detection + kubeclient, routeclient := createFakeRegistryClients(tc.registryInternal) + + // Create cluster server + cs := &clusterServer{ + machineOSConfigLister: moscLister, + machineOSBuildLister: mosbLister, + kubeclient: kubeclient, + routeclient: routeclient, + } + + // Call resolveDesiredImageForPool + result := cs.resolveDesiredImageForPool(pool) + + // Verify result + assert.Equal(t, tc.expectedImageURL, result, "Image URL should match expected for %s", tc.expectedRebootPath) + + // Log the reboot path for clarity + t.Logf("✓ Verified %s", tc.expectedRebootPath) + }) + } +} + +// Mock listers for MachineOSConfig and MachineOSBuild +type mockMOSCLister struct { + configs []*mcfgv1.MachineOSConfig +} + +func (m *mockMOSCLister) List(selector labels.Selector) ([]*mcfgv1.MachineOSConfig, error) { + return m.configs, nil +} + +func (m *mockMOSCLister) Get(name string) (*mcfgv1.MachineOSConfig, error) { + for _, config := range m.configs { + if config.Name == name { + return config, nil + } + } + return nil, fmt.Errorf("MachineOSConfig %s not found", name) +} + +type mockMOSBLister struct { + builds []*mcfgv1.MachineOSBuild +} + +func (m *mockMOSBLister) List(selector labels.Selector) ([]*mcfgv1.MachineOSBuild, error) { + return m.builds, nil +} + +func (m *mockMOSBLister) Get(name string) (*mcfgv1.MachineOSBuild, error) { + for _, build := range m.builds { + if build.Name == name { + return build, nil + } + } + return nil, fmt.Errorf("MachineOSBuild %s not found", name) +} + +// Helper function to create fake clients with or without internal registry +func createFakeRegistryClients(withInternalRegistry bool) (*fake.Clientset, *routefake.Clientset) { + var services []runtime.Object + var routes []runtime.Object + + if withInternalRegistry { + // Create fake internal registry service + services = append(services, &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "image-registry", + Namespace: "openshift-image-registry", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + {Port: 5000}, + }, + }, + }) + + // Create fake internal registry route + routes = append(routes, &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default-route", + Namespace: "openshift-image-registry", + }, + Spec: routev1.RouteSpec{ + Host: "default-route-openshift-image-registry.apps.example.com", + }, + }) + } + + kubeclient := fake.NewSimpleClientset(services...) + routeclient := routefake.NewSimpleClientset(routes...) + + return kubeclient, routeclient +}