diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go index f927c78be3aa..596b693ab0b8 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go @@ -8,30 +8,42 @@ import ( "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" "github.com/openshift/hypershift/support/thirdparty/kubernetes/pkg/credentialprovider" "github.com/openshift/hypershift/support/upsert" + "github.com/openshift/hypershift/support/util" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/utils/ptr" + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" ctrl "sigs.k8s.io/controller-runtime" crclient "sigs.k8s.io/controller-runtime/pkg/client" crreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( - ControllerName = "globalps" + ControllerName = "globalps" + configSeedLabelKey = "hypershift.openshift.io/globalps-config-hash" + globalPSLabelKey = "hypershift.openshift.io/nodepool-globalps-enabled" ) +// GlobalPullSecretPodConfig encapsulates the configuration for GlobalPullSecret DaemonSet pods +type GlobalPullSecretPodConfig struct { + VolumeMounts []corev1.VolumeMount + Volumes []corev1.Volume +} + type Reconciler struct { cpClient crclient.Client kubeSystemSecretClient crclient.Client + nodeClient crclient.Client hcUncachedClient crclient.Client hcpNamespace string hccoImage string + upsert.CreateOrUpdateProvider } @@ -49,18 +61,36 @@ func (r *Reconciler) Reconcile(ctx context.Context, req crreconcile.Request) (cr // reconcileGlobalPullSecret reconciles the original pull secret given by HCP and merges it with a new pull secret provided by the user. // The new pull secret is only stored in the DataPlane side so, it's not exposed in the API. It lives in the kube-system namespace of the DataPlane. -// If that PS exists, the HCCO deploys a DaemonSet which mounts the whole Root FS of the node, and merges the new PS with the original one. +// - If that PS is created, the HCCO deploys a DaemonSet which mounts the node's kubeconfig's file, and merges the new PS with the original one. +// - If the PS doesn't exist, the HCCO doesn't do anything. +// - If at some point the user deletes the additional pull secret, the daemonSet will not be removed // If the PS doesn't exist, the HCCO doesn't do anything. +// +// IMPORTANT: The DaemonSet is ONLY deployed to nodes that are explicitly labeled as eligible. +// Nodes belonging to NodePools using InPlace upgrade strategy are NOT labeled, preventing +// conflicts between the DaemonSet's kubelet config modifications and Machine Config Daemon operations. func (r *Reconciler) reconcileGlobalPullSecret(ctx context.Context) error { var ( userProvidedPullSecretBytes []byte originalPullSecretBytes []byte globalPullSecretBytes []byte err error + ok bool ) log := ctrl.LoggerFrom(ctx) log.Info("reconciling global pull secret") + // Get the original pull secret once at the beginning (used in both scenarios) + originalPullSecret := manifests.PullSecret(r.hcpNamespace) + if err := r.cpClient.Get(ctx, crclient.ObjectKeyFromObject(originalPullSecret), originalPullSecret); err != nil { + return fmt.Errorf("failed to get original pull secret: %w", err) + } + + originalPullSecretBytes, ok = originalPullSecret.Data[corev1.DockerConfigJsonKey] + if !ok || len(originalPullSecretBytes) == 0 { + return fmt.Errorf("original pull secret does not contain %s key", corev1.DockerConfigJsonKey) + } + // Get the user provided pull secret exists, additionalPullSecret, err := additionalPullSecretExists(ctx, r.kubeSystemSecretClient) if err != nil { @@ -68,18 +98,49 @@ func (r *Reconciler) reconcileGlobalPullSecret(ctx context.Context) error { } if !exists || additionalPullSecret.Data == nil { + // Delete global pull secret if it exists secret := manifests.GlobalPullSecret() if err := r.kubeSystemSecretClient.Delete(ctx, secret); err != nil { if !apierrors.IsNotFound(err) { return fmt.Errorf("failed to delete global pull secret: %w", err) } } + + // Create/Update original pull secret in the DataPlane's kube-system namespace + originalSecret := manifests.OriginalPullSecret() + if _, err := r.CreateOrUpdate(ctx, r.kubeSystemSecretClient, originalSecret, func() error { + originalSecret.Data = map[string][]byte{ + corev1.DockerConfigJsonKey: originalPullSecretBytes, + } + return nil + }); err != nil { + return fmt.Errorf("failed to create original pull secret: %w", err) + } + + // Generate a hash of the original pull secret content to trigger pod recreation + configSeed := util.HashSimple(originalPullSecretBytes) + + // Label nodes that should have GlobalPullSecret DaemonSet (non-InPlace nodes) + // This is critical to prevent DaemonSet deployment conflicts with Machine Config Daemon + log.Info("labeling nodes eligible for GlobalPullSecret DaemonSet") + if err := r.labelNodesForGlobalPullSecret(ctx); err != nil { + return fmt.Errorf("failed to label nodes for GlobalPullSecret: %w", err) + } + + // Reconcile DaemonSet with only original pull secret (global-pull-secret will be optional and empty) + daemonSet := manifests.GlobalPullSecretDaemonSet() + if err := reconcileDaemonSet(ctx, daemonSet, "", originalSecret.Name, configSeed, r.hcUncachedClient, r.CreateOrUpdate, r.hccoImage); err != nil { + return fmt.Errorf("failed to reconcile global pull secret daemon set: %w", err) + } + return nil } - // Reconcile the RBAC for the Global Pull Secret - if err := reconcileGlobalPullSecretRBAC(ctx, r.hcUncachedClient, r.CreateOrUpdate, "kube-system", "openshift-config"); err != nil { - return fmt.Errorf("failed to reconcile global pull secret RBAC: %w", err) + // Label nodes that should have GlobalPullSecret DaemonSet (non-InPlace nodes) + // This is critical to prevent DaemonSet deployment conflicts with Machine Config Daemon + log.Info("labeling nodes eligible for GlobalPullSecret DaemonSet") + if err := r.labelNodesForGlobalPullSecret(ctx); err != nil { + return fmt.Errorf("failed to label nodes for GlobalPullSecret: %w", err) } if userProvidedPullSecretBytes, err = validateAdditionalPullSecret(additionalPullSecret); err != nil { @@ -88,21 +149,23 @@ func (r *Reconciler) reconcileGlobalPullSecret(ctx context.Context) error { log.Info("Valid additional pull secret found in the DataPlane, reconciling global pull secret") - // Get the original pull secret - originalPullSecret := manifests.PullSecret(r.hcpNamespace) - if err := r.cpClient.Get(ctx, crclient.ObjectKeyFromObject(originalPullSecret), originalPullSecret); err != nil { - return fmt.Errorf("failed to get original pull secret: %w", err) - } - - // Asumming hcp pull secret is valid - originalPullSecretBytes = originalPullSecret.Data[corev1.DockerConfigJsonKey] - // Merge the additional pull secret with the original pull secret if globalPullSecretBytes, err = mergePullSecrets(ctx, originalPullSecretBytes, userProvidedPullSecretBytes); err != nil { return fmt.Errorf("failed to merge pull secrets: %w", err) } - // Create secret in the DataPlane + // Create original pull secret in the DataPlane's kube-system namespace + originalSecret := manifests.OriginalPullSecret() + if _, err := r.CreateOrUpdate(ctx, r.kubeSystemSecretClient, originalSecret, func() error { + originalSecret.Data = map[string][]byte{ + corev1.DockerConfigJsonKey: originalPullSecretBytes, + } + return nil + }); err != nil { + return fmt.Errorf("failed to create original pull secret: %w", err) + } + + // Create global pull secret in the DataPlane secret := manifests.GlobalPullSecret() if _, err := r.CreateOrUpdate(ctx, r.kubeSystemSecretClient, secret, func() error { secret.Data = map[string][]byte{ @@ -113,15 +176,95 @@ func (r *Reconciler) reconcileGlobalPullSecret(ctx context.Context) error { return fmt.Errorf("failed to create global pull secret: %w", err) } + // Generate a hash of the global pull secret content to trigger pod recreation when content changes + configSeed := util.HashSimple(globalPullSecretBytes) daemonSet := manifests.GlobalPullSecretDaemonSet() - if err := reconcileDaemonSet(ctx, daemonSet, secret.Name, r.hcUncachedClient, r.CreateOrUpdate, r.hccoImage); err != nil { + if err := reconcileDaemonSet(ctx, daemonSet, secret.Name, originalSecret.Name, configSeed, r.hcUncachedClient, r.CreateOrUpdate, r.hccoImage); err != nil { return fmt.Errorf("failed to reconcile global pull secret daemon set: %w", err) } return nil } -func reconcileDaemonSet(ctx context.Context, daemonSet *appsv1.DaemonSet, globalPullSecretName string, c crclient.Client, createOrUpdate upsert.CreateOrUpdateFN, hccoImage string) error { +// labelNodesForGlobalPullSecret labels nodes that should receive the GlobalPullSecret DaemonSet. +// Only nodes that do NOT belong to InPlace NodePools are labeled with hypershift.openshift.io/nodepool-globalps-enabled. +// This ensures the DaemonSet only deploys on nodes where it won't conflict with Machine Config Daemon. +func (r *Reconciler) labelNodesForGlobalPullSecret(ctx context.Context) error { + log := ctrl.LoggerFrom(ctx) + + // Get all nodes from the hosted cluster + nodeList := &corev1.NodeList{} + if err := r.nodeClient.List(ctx, nodeList); err != nil { + return fmt.Errorf("failed to list nodes: %w", err) + } + + // Get all MachineSets to identify which use InPlace upgrade strategy + machineSetList := &capiv1.MachineSetList{} + if err := r.cpClient.List(ctx, machineSetList, &crclient.ListOptions{ + Namespace: r.hcpNamespace, + }); err != nil { + return fmt.Errorf("failed to list MachineSets: %w", err) + } + + // Create set of nodes that should be labeled (from Replace NodePools) + nodesToLabel := make(map[string]bool) + + for _, ms := range machineSetList.Items { + // Check if this MachineSet belongs to a NodePool with InPlace strategy + // This can be identified by the presence of InPlace-specific annotations + _, hasTargetConfig := ms.Annotations["hypershift.openshift.io/nodePoolTargetConfigVersion"] + _, hasCurrentConfig := ms.Annotations["hypershift.openshift.io/nodePoolCurrentConfigVersion"] + + if hasTargetConfig || hasCurrentConfig { + // This is InPlace MachineSet - skip its nodes + continue + } + + // This is Replace MachineSet - include its nodes for labeling + machines := &capiv1.MachineList{} + if err := r.cpClient.List(ctx, machines, &crclient.ListOptions{ + Namespace: ms.Namespace, + LabelSelector: labels.SelectorFromSet(ms.Spec.Selector.MatchLabels), + }); err != nil { + return fmt.Errorf("failed to list machines for Replace MachineSet %s: %w", ms.Name, err) + } + + // Mark nodes from this Replace MachineSet for labeling + for _, machine := range machines.Items { + if machine.Status.NodeRef != nil { + nodesToLabel[machine.Status.NodeRef.Name] = true + } + } + } + + // Update labels only on nodes from Replace NodePools + // These nodes are eligible for GlobalPullSecret DaemonSet scheduling + for _, node := range nodeList.Items { + if nodesToLabel[node.Name] { + // Node belongs to a Replace NodePool, so it's eligible for GlobalPS + nodeCopy := node.DeepCopy() + + if nodeCopy.Labels == nil { + nodeCopy.Labels = make(map[string]string) + } + + currentLabel := nodeCopy.Labels[globalPSLabelKey] + + if currentLabel != "true" { + nodeCopy.Labels[globalPSLabelKey] = "true" + log.Info("labeling node as eligible for GlobalPullSecret DaemonSet", "node", node.Name) + + if err := r.nodeClient.Update(ctx, nodeCopy); err != nil { + return fmt.Errorf("failed to update node labels for GlobalPullSecret eligibility on node %s: %w", node.Name, err) + } + } + } + } + + return nil +} + +func reconcileDaemonSet(ctx context.Context, daemonSet *appsv1.DaemonSet, globalPullSecretName string, originalPullSecretName string, configSeed string, c crclient.Client, createOrUpdate upsert.CreateOrUpdateFN, hccoImage string) error { log := ctrl.LoggerFrom(ctx) log.Info("Reconciling global pull secret daemon set") @@ -135,15 +278,19 @@ func reconcileDaemonSet(ctx context.Context, daemonSet *appsv1.DaemonSet, global Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - "name": manifests.GlobalPullSecretDSName, + "name": manifests.GlobalPullSecretDSName, + configSeedLabelKey: configSeed, }, }, Spec: corev1.PodSpec{ - ServiceAccountName: manifests.GlobalPullSecretDSName, - AutomountServiceAccountToken: ptr.To(true), + AutomountServiceAccountToken: ptr.To(false), SecurityContext: &corev1.PodSecurityContext{}, DNSPolicy: corev1.DNSDefault, Tolerations: []corev1.Toleration{{Operator: corev1.TolerationOpExists}}, + // Use nodeSelector to only include nodes that are explicitly enabled for GlobalPullSecret + NodeSelector: map[string]string{ + globalPSLabelKey: "true", + }, Containers: []corev1.Container{ { Name: manifests.GlobalPullSecretDSName, @@ -154,50 +301,27 @@ func reconcileDaemonSet(ctx context.Context, daemonSet *appsv1.DaemonSet, global }, Args: []string{ "sync-global-pullsecret", - fmt.Sprintf("--global-pull-secret-name=%s", globalPullSecretName), }, SecurityContext: &corev1.SecurityContext{ + // Privileged mode is required for the following operations: + // 1. Write access to /var/lib/kubelet/config.json (kubelet configuration file) + // 2. DBus connection to systemd for kubelet service management + // 3. Restart kubelet.service via systemd (requires root privileges) + // These operations cannot be performed with specific capabilities due to + // the combination of file system access and systemd service management. Privileged: ptr.To(true), }, - VolumeMounts: []corev1.VolumeMount{ - { - Name: "kubelet-config", - MountPath: "/var/lib/kubelet", - }, - { - Name: "dbus", - MountPath: "/var/run/dbus", - }, - }, + VolumeMounts: buildGlobalPSVolumeMounts(globalPullSecretName), TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("50Mi"), - corev1.ResourceCPU: resource.MustParse("40m"), - }, - }, - }, - }, - Volumes: []corev1.Volume{ - { - Name: "kubelet-config", - VolumeSource: corev1.VolumeSource{ - HostPath: &corev1.HostPathVolumeSource{ - Path: "/var/lib/kubelet", - Type: ptr.To(corev1.HostPathDirectory), - }, - }, - }, - { - Name: "dbus", - VolumeSource: corev1.VolumeSource{ - HostPath: &corev1.HostPathVolumeSource{ - Path: "/var/run/dbus", - Type: ptr.To(corev1.HostPathDirectory), + corev1.ResourceMemory: resource.MustParse("35Mi"), + corev1.ResourceCPU: resource.MustParse("5m"), }, }, }, }, + Volumes: buildGlobalPSVolumes(globalPullSecretName, originalPullSecretName), }, }, } @@ -234,20 +358,23 @@ func validateAdditionalPullSecret(pullSecret *corev1.Secret) ([]byte, error) { // MergePullSecrets merges two pull secrets into a single pull secret. // The additional pull secret is merged with the original pull secret. -// If an auth entry already exists, it will be overwritten. -// The resulting pull secret is returned as a JSON string. +// If an auth entry already exists, the original pull secret will be kept. +// If there is somekind on conflict with the original pull secret, the user could +// try to use a namespaced entry, to avoid the limitation on the original pull secret. // Not using credentialprovider.DockerConfigJSON because it does not support // marshaling the auth field. func mergePullSecrets(ctx context.Context, originalPullSecret, userProvidedPullSecret []byte) ([]byte, error) { var ( originalAuths map[string]any userProvidedAuths map[string]any + finalAuths map[string]any originalJSON map[string]any userProvidedJSON map[string]any globalPullSecretBytes []byte err error ) + log := ctrl.LoggerFrom(ctx) // Unmarshal original pull secret if err = json.Unmarshal(originalPullSecret, &originalJSON); err != nil { return nil, fmt.Errorf("invalid original pull secret format: %w", err) @@ -260,14 +387,17 @@ func mergePullSecrets(ctx context.Context, originalPullSecret, userProvidedPullS } userProvidedAuths = userProvidedJSON["auths"].(map[string]any) - // Merge auths - for k, v := range userProvidedAuths { - originalAuths[k] = v + for k, v := range originalAuths { + if _, ok := userProvidedAuths[k]; ok { + log.Info("The registry provided in the additional-pull-secret secret already exists in the original pull secret, this is not allowed. Keeping the original pull secret registry authentication", "registry", k) + } + userProvidedAuths[k] = v } + finalAuths = userProvidedAuths // Create final JSON finalJSON := map[string]any{ - "auths": originalAuths, + "auths": finalAuths, } globalPullSecretBytes, err = json.Marshal(finalJSON) @@ -278,104 +408,131 @@ func mergePullSecrets(ctx context.Context, originalPullSecret, userProvidedPullS return globalPullSecretBytes, nil } -func reconcileGlobalPullSecretRBAC(ctx context.Context, c crclient.Client, createOrUpdate upsert.CreateOrUpdateFN, kubeSystemNS, openshiftConfigNS string) error { - // Remove the RBAC resources if the user provided pull secret is not present - log := ctrl.LoggerFrom(ctx) - log.Info("reconciling global pull secret RBAC") - - // Create ServiceAccount - sa := manifests.GlobalPullSecretSyncerServiceAccount() - if _, err := createOrUpdate(ctx, c, sa, func() error { return nil }); err != nil { - return fmt.Errorf("failed to reconcile service account: %w", err) - } - - // Create Role and RoleBinding for kube-system namespace - globalPullSecretRole := manifests.GlobalPullSecretSyncerRole(kubeSystemNS) - if _, err := createOrUpdate(ctx, c, globalPullSecretRole, func() error { - globalPullSecretRole.Rules = []rbacv1.PolicyRule{ - { - APIGroups: []string{""}, - Resources: []string{"secrets"}, - Verbs: []string{"list", "watch"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"secrets"}, - ResourceNames: []string{"additional-pull-secret", "global-pull-secret"}, - Verbs: []string{"get"}, - }, +func additionalPullSecretExists(ctx context.Context, c crclient.Client) (bool, *corev1.Secret, error) { + additionalPullSecret := manifests.AdditionalPullSecret() + if err := c.Get(ctx, crclient.ObjectKeyFromObject(additionalPullSecret), additionalPullSecret); err != nil { + if apierrors.IsNotFound(err) { + return false, nil, nil } - return nil - }); err != nil { - return fmt.Errorf("failed to reconcile global pull secret syncer role in kube-system: %w", err) + return false, nil, err } + return true, additionalPullSecret, nil +} - // Create RoleBinding for kube-system namespace - globalPullSecretRoleBinding := manifests.GlobalPullSecretSyncerRoleBinding(kubeSystemNS) - if _, err := createOrUpdate(ctx, c, globalPullSecretRoleBinding, func() error { - globalPullSecretRoleBinding.RoleRef = rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: globalPullSecretRole.Name, - } - globalPullSecretRoleBinding.Subjects = []rbacv1.Subject{ - { - Kind: "ServiceAccount", - Name: sa.Name, - Namespace: sa.Namespace, - }, - } - return nil - }); err != nil { - return fmt.Errorf("failed to reconcile global pull secret syncer role binding in kube-system: %w", err) - } - - // Create Role and RoleBinding for openshift-config namespace - globalPullSecretOpenshiftConfigRole := manifests.GlobalPullSecretSyncerRole(openshiftConfigNS) - if _, err := createOrUpdate(ctx, c, globalPullSecretOpenshiftConfigRole, func() error { - globalPullSecretOpenshiftConfigRole.Rules = []rbacv1.PolicyRule{ - { - APIGroups: []string{""}, - Resources: []string{"secrets"}, - ResourceNames: []string{"pull-secret"}, - Verbs: []string{"get"}, - }, - } - return nil - }); err != nil { - return fmt.Errorf("failed to reconcile global pull secret syncer role in openshift-config: %w", err) +// Volume build functions for GlobalPullSecret DaemonSet +// buildGlobalPSVolumeMounts builds the volume mounts for the GlobalPullSecret DaemonSet +func buildGlobalPSVolumeMounts(globalPullSecretName string) []corev1.VolumeMount { + var volumeMounts []corev1.VolumeMount + + volumeMounts = append(volumeMounts, globalPSVolumeMountKubeletConfig()) + volumeMounts = append(volumeMounts, globalPSVolumeMountDbus()) + volumeMounts = append(volumeMounts, globalPSVolumeMountOriginalPullSecret()) + + if globalPullSecretName != "" { + volumeMounts = append(volumeMounts, globalPSVolumeMountGlobalPullSecret()) + } + + return volumeMounts +} + +// buildGlobalPSVolumes creates volumes for the GlobalPullSecret DaemonSet using util.BuildVolume pattern +func buildGlobalPSVolumes(globalPullSecretName string, originalPullSecretName string) []corev1.Volume { + var volumes []corev1.Volume + + volumes = append(volumes, util.BuildVolume(globalPSVolumeKubeletConfig(), buildGlobalPSVolumeKubeletConfig)) + volumes = append(volumes, util.BuildVolume(globalPSVolumeDbus(), buildGlobalPSVolumeDbus)) + volumes = append(volumes, util.BuildVolume(globalPSVolumeOriginalPullSecret(), buildGlobalPSVolumeOriginalPullSecret(originalPullSecretName))) + + if globalPullSecretName != "" { + volumes = append(volumes, util.BuildVolume(globalPSVolumeGlobalPullSecret(), buildGlobalPSVolumeGlobalPullSecret(globalPullSecretName))) } - // Create RoleBinding for openshift-config namespace - globalPullSecretOpenshiftConfigRoleBinding := manifests.GlobalPullSecretSyncerRoleBinding(openshiftConfigNS) - if _, err := createOrUpdate(ctx, c, globalPullSecretOpenshiftConfigRoleBinding, func() error { - globalPullSecretOpenshiftConfigRoleBinding.RoleRef = rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: globalPullSecretOpenshiftConfigRole.Name, + return volumes +} + +func globalPSVolumeKubeletConfig() *corev1.Volume { + return &corev1.Volume{ + Name: "kubelet-config", + } +} + +func globalPSVolumeDbus() *corev1.Volume { + return &corev1.Volume{ + Name: "dbus", + } +} + +func globalPSVolumeOriginalPullSecret() *corev1.Volume { + return &corev1.Volume{ + Name: "original-pull-secret", + } +} + +func globalPSVolumeGlobalPullSecret() *corev1.Volume { + return &corev1.Volume{ + Name: "global-pull-secret", + } +} + +// Volume builder functions +func buildGlobalPSVolumeKubeletConfig(v *corev1.Volume) { + v.HostPath = &corev1.HostPathVolumeSource{ + Path: "/var/lib/kubelet", + Type: ptr.To(corev1.HostPathDirectory), + } +} + +func buildGlobalPSVolumeDbus(v *corev1.Volume) { + v.HostPath = &corev1.HostPathVolumeSource{ + Path: "/var/run/dbus", + Type: ptr.To(corev1.HostPathDirectory), + } +} + +func buildGlobalPSVolumeOriginalPullSecret(secretName string) func(v *corev1.Volume) { + return func(v *corev1.Volume) { + v.Secret = &corev1.SecretVolumeSource{ + SecretName: secretName, } - globalPullSecretOpenshiftConfigRoleBinding.Subjects = []rbacv1.Subject{ - { - Kind: "ServiceAccount", - Name: sa.Name, - Namespace: sa.Namespace, - }, + } +} + +func buildGlobalPSVolumeGlobalPullSecret(secretName string) func(v *corev1.Volume) { + return func(v *corev1.Volume) { + v.Secret = &corev1.SecretVolumeSource{ + SecretName: secretName, + Optional: ptr.To(true), } - return nil - }); err != nil { - return fmt.Errorf("failed to reconcile global pull secret syncer role binding in openshift-config: %w", err) } +} - return nil +// Volume mount functions for GlobalPullSecret DaemonSet +func globalPSVolumeMountKubeletConfig() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: globalPSVolumeKubeletConfig().Name, + MountPath: "/var/lib/kubelet", + } } -func additionalPullSecretExists(ctx context.Context, c crclient.Client) (bool, *corev1.Secret, error) { - additionalPullSecret := manifests.AdditionalPullSecret() - if err := c.Get(ctx, crclient.ObjectKeyFromObject(additionalPullSecret), additionalPullSecret); err != nil { - if apierrors.IsNotFound(err) { - return false, nil, nil - } - return false, nil, err +func globalPSVolumeMountDbus() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: globalPSVolumeDbus().Name, + MountPath: "/var/run/dbus", + } +} + +func globalPSVolumeMountOriginalPullSecret() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: globalPSVolumeOriginalPullSecret().Name, + MountPath: "/etc/original-pull-secret", + ReadOnly: true, + } +} + +func globalPSVolumeMountGlobalPullSecret() corev1.VolumeMount { + return corev1.VolumeMount{ + Name: globalPSVolumeGlobalPullSecret().Name, + MountPath: "/etc/global-pull-secret", + ReadOnly: true, } - return true, additionalPullSecret, nil } diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go index 6121e39ea47a..ce9a5ad759ae 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go @@ -10,7 +10,9 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -107,10 +109,24 @@ func TestMergePullSecrets(t *testing.T) { wantErr: false, }, { - name: "overwrite existing registry", + name: "conflict resolution - original always wins", originalSecret: composePullSecretBytes(map[string]string{"registry1": oldAuth}), additionalSecret: composePullSecretBytes(map[string]string{"registry1": validAuth}), - expectedResult: composePullSecretBytes(map[string]string{"registry1": validAuth}), + expectedResult: composePullSecretBytes(map[string]string{"registry1": oldAuth}), + wantErr: false, + }, + { + name: "precedence test - original always has precedence", + originalSecret: composePullSecretBytes(map[string]string{"registry1": oldAuth, "registry2": oldAuth}), + additionalSecret: composePullSecretBytes(map[string]string{"registry1": validAuth, "registry3": validAuth}), + expectedResult: composePullSecretBytes(map[string]string{"registry1": oldAuth, "registry2": oldAuth, "registry3": validAuth}), + wantErr: false, + }, + { + name: "multiple conflicts - original always wins", + originalSecret: composePullSecretBytes(map[string]string{"registry1": oldAuth, "registry2": oldAuth}), + additionalSecret: composePullSecretBytes(map[string]string{"registry1": validAuth, "registry2": validAuth, "registry3": validAuth}), + expectedResult: composePullSecretBytes(map[string]string{"registry1": oldAuth, "registry2": oldAuth, "registry3": validAuth}), wantErr: false, }, { @@ -279,3 +295,304 @@ func TestAdditionalPullSecretExists(t *testing.T) { }) } } + +func TestLabelNodesForGlobalPullSecret(t *testing.T) { + tests := []struct { + name string + nodes []corev1.Node + machineSets []capiv1.MachineSet + machines []capiv1.Machine + expectedLabeled []string // names of nodes that should have the label + }{ + { + name: "Replace-InPlace-Replace scenario: only Replace nodes should be labeled", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-node-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-node-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-node-2", + }, + }, + }, + machineSets: []capiv1.MachineSet{ + // First NodePool: Replace strategy (no InPlace annotations) + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machineset-1", + Namespace: "test-namespace", + }, + Spec: capiv1.MachineSetSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machineset": "replace-1", + }, + }, + }, + }, + // Second NodePool: InPlace strategy (has InPlace annotations) + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-machineset-1", + Namespace: "test-namespace", + Annotations: map[string]string{ + "hypershift.openshift.io/nodePoolTargetConfigVersion": "config-hash-123", + "hypershift.openshift.io/nodePoolCurrentConfigVersion": "config-hash-456", + }, + }, + Spec: capiv1.MachineSetSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machineset": "inplace-1", + }, + }, + }, + }, + // Third NodePool: Replace strategy (no InPlace annotations) - this should work after InPlace + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machineset-2", + Namespace: "test-namespace", + }, + Spec: capiv1.MachineSetSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machineset": "replace-2", + }, + }, + }, + }, + }, + machines: []capiv1.Machine{ + // Machine for first Replace NodePool + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machine-1", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "replace-1", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "replace-node-1", + }, + }, + }, + // Machine for InPlace NodePool + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-machine-1", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "inplace-1", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "inplace-node-1", + }, + }, + }, + // Machine for second Replace NodePool (created after InPlace) + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machine-2", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "replace-2", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "replace-node-2", + }, + }, + }, + }, + expectedLabeled: []string{"replace-node-1", "replace-node-2"}, // Both Replace nodes should be labeled + }, + { + name: "Only InPlace NodePools: no nodes should be labeled", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-node-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-node-2", + }, + }, + }, + machineSets: []capiv1.MachineSet{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-machineset-1", + Namespace: "test-namespace", + Annotations: map[string]string{ + "hypershift.openshift.io/nodePoolTargetConfigVersion": "config-hash-123", + }, + }, + Spec: capiv1.MachineSetSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machineset": "inplace-1", + }, + }, + }, + }, + }, + machines: []capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "inplace-machine-1", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "inplace-1", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "inplace-node-1", + }, + }, + }, + }, + expectedLabeled: []string{}, // No nodes should be labeled + }, + { + name: "Only Replace NodePools: all nodes should be labeled", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-node-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-node-2", + }, + }, + }, + machineSets: []capiv1.MachineSet{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machineset-1", + Namespace: "test-namespace", + // No InPlace annotations + }, + Spec: capiv1.MachineSetSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machineset": "replace-1", + }, + }, + }, + }, + }, + machines: []capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machine-1", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "replace-1", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "replace-node-1", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "replace-machine-2", + Namespace: "test-namespace", + Labels: map[string]string{ + "machineset": "replace-1", + }, + }, + Status: capiv1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: "replace-node-2", + }, + }, + }, + }, + expectedLabeled: []string{"replace-node-1", "replace-node-2"}, // All Replace nodes should be labeled + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // Create runtime scheme and add required types + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = capiv1.AddToScheme(scheme) + + // Convert to client.Object slices + var objects []client.Object + for i := range tt.nodes { + objects = append(objects, &tt.nodes[i]) + } + for i := range tt.machineSets { + objects = append(objects, &tt.machineSets[i]) + } + for i := range tt.machines { + objects = append(objects, &tt.machines[i]) + } + + // Create fake clients + cpClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + kubeSystemSecretClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + nodeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + hcUncachedClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + // Create reconciler + reconciler := &Reconciler{ + cpClient: cpClient, + kubeSystemSecretClient: kubeSystemSecretClient, + nodeClient: nodeClient, + hcUncachedClient: hcUncachedClient, + hcpNamespace: "test-namespace", + } + + // Execute the function under test + err := reconciler.labelNodesForGlobalPullSecret(context.Background()) + g.Expect(err).NotTo(HaveOccurred()) + + // Check that only expected nodes have the label + nodeList := &corev1.NodeList{} + err = nodeClient.List(context.Background(), nodeList) + g.Expect(err).NotTo(HaveOccurred()) + + labeledNodes := make(map[string]bool) + for _, node := range nodeList.Items { + if node.Labels != nil && node.Labels[globalPSLabelKey] == "true" { + labeledNodes[node.Name] = true + } + } + + // Verify expected nodes are labeled + for _, expectedNode := range tt.expectedLabeled { + g.Expect(labeledNodes[expectedNode]).To(BeTrue(), "Node %s should be labeled but wasn't", expectedNode) + } + + // Verify no unexpected nodes are labeled + g.Expect(len(labeledNodes)).To(Equal(len(tt.expectedLabeled)), "Number of labeled nodes doesn't match expected") + }) + } +} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go index 798f79d982ad..59a8b8fb213d 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go @@ -8,13 +8,16 @@ import ( "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/operator" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/cache" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" + crreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" ) @@ -35,7 +38,18 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig return fmt.Errorf("failed to create kube-system cache: %w", err) } - // Create a crclient from new cache + // Create a cache for nodes (cluster-scoped) + nodeCache, err := cache.New(opts.Manager.GetConfig(), cache.Options{ + Scheme: opts.Manager.GetScheme(), + DefaultNamespaces: map[string]cache.Config{ + "": {}, // Empty string for cluster-scoped resources like nodes + }, + }) + if err != nil { + return fmt.Errorf("failed to create node cache: %w", err) + } + + // Create a crclient from kube-system cache kubeSystemClient, err := crclient.New(opts.Manager.GetConfig(), crclient.Options{ Scheme: opts.Manager.GetScheme(), Cache: &crclient.CacheOptions{Reader: kubeSystemCache}, @@ -44,12 +58,26 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig return fmt.Errorf("failed to create kube-system client: %w", err) } - // Get the informer for secrets from new cache + // Create a crclient from node cache + nodeClient, err := crclient.New(opts.Manager.GetConfig(), crclient.Options{ + Scheme: opts.Manager.GetScheme(), + Cache: &crclient.CacheOptions{Reader: nodeCache}, + }) + if err != nil { + return fmt.Errorf("failed to create node client: %w", err) + } + + // Get the informers for Watch usage only (not for hybrid approach) kubeSystemSecretInformer, err := kubeSystemCache.GetInformer(ctx, &corev1.Secret{}) if err != nil { return fmt.Errorf("failed to get kube-system secret informer: %w", err) } + nodeInformer, err := nodeCache.GetInformer(ctx, &corev1.Node{}) + if err != nil { + return fmt.Errorf("failed to get node informer: %w", err) + } + uncachedClientRestConfig := opts.Manager.GetConfig() uncachedClientRestConfig.WarningHandler = rest.NoWarnings{} uncachedClient, err := crclient.New(uncachedClientRestConfig, crclient.Options{ @@ -69,15 +97,19 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig cpClient: opts.CPCluster.GetClient(), hcUncachedClient: uncachedClient, kubeSystemSecretClient: kubeSystemClient, + nodeClient: nodeClient, hcpNamespace: opts.Namespace, hccoImage: hccoImage, CreateOrUpdateProvider: opts.TargetCreateOrUpdateProvider, } - // Add the cache to the manager + // Add the caches to the manager if err := opts.Manager.Add(kubeSystemCache); err != nil { return fmt.Errorf("failed to add kube-system cache: %w", err) } + if err := opts.Manager.Add(nodeCache); err != nil { + return fmt.Errorf("failed to add node cache: %w", err) + } // Create a controller c, err := controller.New(ControllerName, opts.Manager, controller.Options{Reconciler: r}) @@ -96,5 +128,33 @@ func Setup(ctx context.Context, opts *operator.HostedClusterConfigOperatorConfig return fmt.Errorf("failed to watch kube-system secrets: %w", err) } + // Watch for nodes - when nodes are created, we need to reconcile global pull secret + if err := c.Watch(&source.Informer{ + Informer: nodeInformer, + Handler: handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o crclient.Object) []crreconcile.Request { + // Trigger reconciliation for node creation using the node's name for better observability + // The reconciler ignores the NamespacedName but this helps with logging and debugging + return []crreconcile.Request{{NamespacedName: types.NamespacedName{Name: o.GetName(), Namespace: ""}}} + }), + Predicates: []predicate.Predicate{ + predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + // Only reconcile when new nodes are created + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + // Ignore node updates + return false + }, + DeleteFunc: func(e event.DeleteEvent) bool { + // Ignore node deletions + return false + }, + }, + }, + }); err != nil { + return fmt.Errorf("failed to watch nodes: %w", err) + } + return nil } diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go index da33e264c576..675050e8ff08 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go @@ -3,7 +3,6 @@ package manifests import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -47,39 +46,21 @@ func GlobalPullSecretDaemonSet() *appsv1.DaemonSet { } } -func GlobalPullSecret() *corev1.Secret { +func OriginalPullSecret() *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "global-pull-secret", + Name: "original-pull-secret", Namespace: GlobalPullSecretNamespace, }, - Type: corev1.SecretTypeDockerConfigJson, } } -func GlobalPullSecretSyncerServiceAccount() *corev1.ServiceAccount { - return &corev1.ServiceAccount{ +func GlobalPullSecret() *corev1.Secret { + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "global-pull-secret-syncer", + Name: "global-pull-secret", Namespace: GlobalPullSecretNamespace, }, - } -} - -func GlobalPullSecretSyncerRole(ns string) *rbacv1.Role { - return &rbacv1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: "global-pull-secret-syncer", - Namespace: ns, - }, - } -} - -func GlobalPullSecretSyncerRoleBinding(ns string) *rbacv1.RoleBinding { - return &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: "global-pull-secret-syncer", - Namespace: ns, - }, + Type: corev1.SecretTypeDockerConfigJson, } } diff --git a/docs/content/how-to/common/global-pull-secret.md b/docs/content/how-to/common/global-pull-secret.md index cd424461e5cd..a7eb1c599a6f 100644 --- a/docs/content/how-to/common/global-pull-secret.md +++ b/docs/content/how-to/common/global-pull-secret.md @@ -43,13 +43,17 @@ Your `.dockerconfigjson` should follow this structure: "registry.example.com": { "auth": "base64-encoded-credentials" }, - "quay.io": { + "quay.io/mycompany": { "auth": "base64-encoded-credentials" } } } ``` +!!! tip "Using Namespace-Specific Registry Entries" + + For registries like Quay.io that support organization/namespace-specific authentication, you can specify the full path in your registry entry (e.g., `quay.io/mycompany` instead of just `quay.io`). This allows you to provide different credentials for different namespaces within the same registry, and helps avoid conflicts with existing registry entries in the original pull secret. + ### 3. Apply the secret ```bash @@ -90,13 +94,23 @@ The Global Pull Secret functionality operates through a multi-component system: - The system validates that your secret contains a proper DockerConfigJSON format - It retrieves the original pull secret from the HostedControlPlane - Your additional pull secret is merged with the original one -- If there are conflicting registry entries, your additional pull secret takes precedence +- **If there are conflicting registry entries, the original pull secret takes precedence** (the additional pull secret entry is ignored for conflicting registries) +- The system supports namespace-specific registry entries (e.g., `quay.io/namespace`) for better credential specificity ### Deployment Process - A `global-pull-secret` is created in the `kube-system` namespace containing the merged result - RBAC resources (ServiceAccount, Role, RoleBinding) are created for the DaemonSet in both `kube-system` and `openshift-config` namespaces - We use Role and RoleBinding in both namespaces to access secrets in `kube-system` and `openshift-config` namespaces -- A DaemonSet named `global-pull-secret-syncer` is deployed to all nodes +- A DaemonSet named `global-pull-secret-syncer` is deployed to eligible nodes + +!!! warning "NodePool InPlace Strategy Restriction" + + The Global Pull Secret DaemonSet is **not deployed** to nodes that belong to NodePools using the **InPlace upgrade strategy**. This restriction prevents conflicts between the DaemonSet's modifications to `/var/lib/kubelet/config.json` and the Machine Config Daemon (MCD) during InPlace upgrades. + + - **Nodes with Replace strategy**: ✅ Receive Global Pull Secret DaemonSet + - **Nodes with InPlace strategy**: ❌ Do not receive Global Pull Secret DaemonSet + + This ensures that MCD operations during InPlace upgrades do not fail due to unexpected changes in kubelet configuration files. ### Node-Level Synchronization - Each DaemonSet pod runs a controller that watches the secrets under kube-system namespace @@ -105,9 +119,69 @@ The Global Pull Secret functionality operates through a multi-component system: - If the restart fails after 3 attempts, the system rolls back the file changes ### Automatic Cleanup -- If you delete the `additional-pull-secret`, the HCCO automatically removes the globalPullSecret secret -- The DaemonSet is deleted from all nodes -- RBAC resources (ServiceAccount, Role, RoleBinding) in both namespaces are cleaned up by the HCCO +- If you delete the `additional-pull-secret`, the HCCO automatically removes the `global-pull-secret` secret +- The system reverts to using only the original pull secret from the HostedControlPlane +- The DaemonSet continues running but now syncs only the original pull secret to nodes + +## Registry Precedence and Conflict Resolution + +The Global Pull Secret system uses a specific precedence model when merging your additional pull secret with the original one: + +### Merge Behavior +- **Original pull secret entries always take precedence** over additional pull secret entries for the same registry +- If both secrets contain an entry for `quay.io`, the original pull secret's credentials will be used +- Your additional pull secret entries are only added if they don't conflict with existing entries +- Warnings are logged when conflicts are detected + +### Recommended Approach +To avoid conflicts and ensure your credentials are used, consider these strategies: + +1. **Use namespace-specific entries**: Instead of `quay.io`, use `quay.io/your-namespace` +2. **Target specific registries**: Add entries only for registries not already in the original pull secret +3. **Check existing entries**: Review what registries are already configured in the HostedControlPlane + +### Example Merge Scenario + +**Original Pull Secret:** +```json +{ + "auths": { + "quay.io": { + "auth": "original-credentials" + } + } +} +``` + +**Your Additional Pull Secret:** +```json +{ + "auths": { + "quay.io": { + "auth": "your-credentials" + }, + "quay.io/mycompany": { + "auth": "your-namespace-credentials" + } + } +} +``` + +**Resulting Merged Pull Secret:** +```json +{ + "auths": { + "quay.io": { + "auth": "original-credentials" + }, + "quay.io/mycompany": { + "auth": "your-namespace-credentials" + } + } +} +``` + +Note how the `quay.io` entry keeps the original credentials, but `quay.io/mycompany` is added from your additional secret. ## Implementation details @@ -120,6 +194,7 @@ The implementation consists of several key components working together: - Manages the merging logic between original and additional pull secrets - Creates and manages RBAC resources - Deploys and manages the DaemonSet + - **Node eligibility assessment**: Labels nodes from InPlace NodePools and configures DaemonSet scheduling restrictions 2. **Sync Global Pull Secret Command** (`sync-global-pullsecret` package) - Runs as a DaemonSet on each node @@ -232,7 +307,46 @@ graph TB - **Efficiency** - Only updates when there are actual changes - The globalPullSecret implementation has their own controller so it cannot interfere with the HCCO reonciliation -- **Minimal privileges**: Specific RBAC for only the required resources in each namespace +- **Security considerations**: Uses specific RBAC for only the required resources in each namespace. The DaemonSet containers run in privileged mode due to the need to: + - Write to `/var/lib/kubelet/config.json` (kubelet configuration file) + - Connect to systemd via DBus for service management + - Restart kubelet.service, which requires root privileges +- **Smart node targeting**: Automatically excludes nodes from InPlace NodePools to prevent MCD conflicts + +### InPlace NodePool Handling + +To prevent conflicts with Machine Config Daemon operations, the implementation includes intelligent node targeting: + +#### Node Labeling Process +1. **MachineSets Discovery**: The controller queries the management cluster for MachineSets with InPlace-specific annotations (`hypershift.openshift.io/nodePoolTargetConfigVersion`) +2. **Machine Enumeration**: For each InPlace MachineSets, it lists all associated Machines +3. **Node Identification**: Maps Machine objects to their corresponding nodes via `machine.Status.NodeRef.Name` +4. **Labeling**: Applies `hypershift.openshift.io/nodepool-inplace-strategy=true` label to identified nodes + +#### DaemonSet Scheduling Configuration +The DaemonSet uses NodeAffinity to exclude InPlace nodes: + +```yaml +spec: + template: + spec: + affinity: + nodeAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + nodeSelectorTerms: + - matchExpressions: + - key: hypershift.openshift.io/nodepool-inplace-strategy + operator: DoesNotExist +``` + +This ensures that: +- **Nodes without the label**: ✅ Are eligible for DaemonSet scheduling +- **Nodes with the label** (any value): ❌ Are excluded from DaemonSet scheduling + +#### Conflict Prevention Benefits +- **Prevents MCD failures**: Avoids conflicts when MCD expects specific kubelet configuration during InPlace upgrades +- **Maintains upgrade reliability**: InPlace upgrade processes are not interrupted by Global Pull Secret modifications +- **Automatic detection**: No manual intervention required - the system automatically identifies and handles InPlace nodes ### Error Handling diff --git a/support/util/util.go b/support/util/util.go index aa695b314ce7..e8330b02eb5d 100644 --- a/support/util/util.go +++ b/support/util/util.go @@ -729,3 +729,27 @@ func HostFromURL(addr string) (string, error) { func EnableIfCustomKubeconfig(hcp *hyperv1.HostedControlPlane) bool { return len(hcp.Spec.KubeAPIServerDNSName) > 0 } + +// CountAvailableNodes counts the number of available nodes in the cluster. +// Available nodes are defined as Ready nodes that are not cordoned (Unschedulable). +func CountAvailableNodes(ctx context.Context, client client.Client) (int32, error) { + var nodeList corev1.NodeList + if err := client.List(ctx, &nodeList); err != nil { + return 0, fmt.Errorf("failed to list nodes: %w", err) + } + + // Count only available nodes (Ready nodes that are not cordoned) + availableNodesCount := int32(0) + for _, node := range nodeList.Items { + if !node.Spec.Unschedulable { + for _, condition := range node.Status.Conditions { + if condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue { + availableNodesCount++ + break + } + } + } + } + + return availableNodesCount, nil +} diff --git a/support/util/util_test.go b/support/util/util_test.go index 10939f4972f5..502a25a92d12 100644 --- a/support/util/util_test.go +++ b/support/util/util_test.go @@ -1,6 +1,7 @@ package util import ( + "context" "testing" "unicode/utf8" @@ -11,10 +12,12 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" apiversion "k8s.io/apimachinery/pkg/version" fakediscovery "k8s.io/client-go/discovery/fake" fakekubeclient "k8s.io/client-go/kubernetes/fake" + "sigs.k8s.io/controller-runtime/pkg/client" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -892,3 +895,118 @@ func TestHostFromURL(t *testing.T) { }) } } + +func TestCountAvailableNodes(t *testing.T) { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + + tests := []struct { + name string + nodes []corev1.Node + expected int32 + expectErr bool + }{ + { + name: "all nodes ready and schedulable", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Spec: corev1.NodeSpec{Unschedulable: false}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "node2"}, + Spec: corev1.NodeSpec{Unschedulable: false}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + }, + }, + expected: 2, + }, + { + name: "one node cordoned", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Spec: corev1.NodeSpec{Unschedulable: false}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "node2"}, + Spec: corev1.NodeSpec{Unschedulable: true}, // cordoned + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + }, + }, + expected: 1, + }, + { + name: "one node not ready", + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Spec: corev1.NodeSpec{Unschedulable: false}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "node2"}, + Spec: corev1.NodeSpec{Unschedulable: false}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionFalse}, // not ready + }, + }, + }, + }, + expected: 1, + }, + { + name: "no nodes", + nodes: []corev1.Node{}, + expected: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objects := make([]client.Object, len(tt.nodes)) + for i := range tt.nodes { + objects[i] = &tt.nodes[i] + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + Build() + + result, err := CountAvailableNodes(context.Background(), fakeClient) + if tt.expectErr && err == nil { + t.Errorf("expected error, got none") + } + if !tt.expectErr && err != nil { + t.Errorf("unexpected error: %v", err) + } + if result != tt.expected { + t.Errorf("expected %d available nodes, got %d", tt.expected, result) + } + }) + } +} diff --git a/sync-global-pullsecret/sync-global-pullsecret.go b/sync-global-pullsecret/sync-global-pullsecret.go index 1f21ac897f72..57a178a9a75f 100644 --- a/sync-global-pullsecret/sync-global-pullsecret.go +++ b/sync-global-pullsecret/sync-global-pullsecret.go @@ -4,91 +4,89 @@ package syncglobalpullsecret import ( "context" + "encoding/json" "fmt" "os" + "os/signal" + "path/filepath" + "syscall" "time" - hyperapi "github.com/openshift/hypershift/support/api" - - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/rest" - - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" - crclient "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/coreos/go-systemd/dbus" + "github.com/go-logr/logr" + "github.com/go-logr/zapr" "github.com/spf13/cobra" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" ) // syncGlobalPullSecretOptions contains the configuration options for the sync-global-pullsecret command type syncGlobalPullSecretOptions struct { kubeletConfigJsonPath string - globalPSSecretName string } -const ( - defaultKubeletConfigJsonPath = "/var/lib/kubelet/config.json" - defaultGlobalPSSecretName = "global-pull-secret" - additionalPullSecretName = "additional-pull-secret" - defaultGlobalPullSecretNamespace = "kube-system" - originalPullSecretName = "pull-secret" - originalPullSecretNamespace = "openshift-config" - dbusRestartUnitMode = "replace" - kubeletServiceUnit = "kubelet.service" - - // systemd job completion state as documented in go-systemd/dbus - systemdJobDone = "done" // Job completed successfully -) - //go:generate ../hack/tools/bin/mockgen -destination=sync-global-pullsecret_mock.go -package=syncglobalpullsecret . dbusConn type dbusConn interface { RestartUnit(name string, mode string, ch chan<- string) (int, error) Close() } -// writeFileFunc is a variable that holds the function used to write files. -// This allows tests to inject custom write functions for testing rollback scenarios. -var writeFileFunc = os.WriteFile - -// GlobalPullSecretReconciler reconciles a Secret object -type GlobalPullSecretReconciler struct { - cachedClient crclient.Client - uncachedClient crclient.Client - Scheme *runtime.Scheme +// GlobalPullSecretSyncer handles the synchronization of pull secrets +type GlobalPullSecretSyncer struct { kubeletConfigJsonPath string - globalPSSecretName string - globalPSSecretNS string + log logr.Logger } +const ( + defaultKubeletConfigJsonPath = "/var/lib/kubelet/config.json" + dbusRestartUnitMode = "replace" + kubeletServiceUnit = "kubelet.service" + + // Mounted secret file paths + originalPullSecretFilePath = "/etc/original-pull-secret/.dockerconfigjson" + globalPullSecretFilePath = "/etc/global-pull-secret/.dockerconfigjson" + + tickerPace = 30 * time.Second + + // systemd job completion state as documented in go-systemd/dbus + systemdJobDone = "done" // Job completed successfully +) + +var ( + // writeFileFunc is a variable that holds the function used to write files. + // This allows tests to inject custom write functions for testing rollback scenarios. + writeFileFunc = writeAtomic + + // readFileFunc is a variable that holds the function used to read files. + // This allows tests to inject custom read functions for testing. + readFileFunc = os.ReadFile +) + // NewRunCommand creates a new cobra.Command for the sync-global-pullsecret command func NewRunCommand() *cobra.Command { cmd := &cobra.Command{ Use: "sync-global-pullsecret", - Short: "Syncs a mixture between the user provided pull secret in DataPlane and the HostedCluster PullSecret to be deployed in the nodes of the HostedCluster", - Long: `Syncs a mixture between the user provided pull secret in DataPlane and the HostedCluster PullSecret to be deployed in the nodes of the HostedCluster. The resulting pull secret is deployed in a DaemonSet in the DataPlane that updates the kubelet.config.json file with the new pull secret. If there are conflicting entries in the resulting global pull secret, the user provided pull secret will prevail.`, + Short: "Syncs a mixture between the user original pull secret in DataPlane and the HostedCluster PullSecret to be deployed in the nodes of the HostedCluster", + Long: `Syncs a mixture between the user original pull secret in DataPlane and the HostedCluster PullSecret to be deployed in the nodes of the HostedCluster. The resulting pull secret is deployed in a DaemonSet in the DataPlane that updates the kubelet.config.json file with the new pull secret. If there are conflicting entries in the resulting global pull secret, the original pull secret entries will prevail to ensure the well functioning of the nodes.`, } opts := syncGlobalPullSecretOptions{ kubeletConfigJsonPath: defaultKubeletConfigJsonPath, } - cmd.Flags().StringVar(&opts.globalPSSecretName, "global-pull-secret-name", defaultGlobalPSSecretName, "The name of the global pullSecret secret in the DataPlane.") cmd.Run = func(cmd *cobra.Command, args []string) { - setupLog := ctrl.Log.WithName("global-pullsecret") - zapOpts := zap.Options{ - Development: true, - } - ctrl.SetLogger(zap.New(zap.UseFlagOptions(&zapOpts))) - ctx := ctrl.SetupSignalHandler() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Handle SIGINT and SIGTERM + sigChan := make(chan os.Signal, 1) + signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) + go func() { + <-sigChan + cancel() + }() + if err := opts.run(ctx); err != nil { - setupLog.Error(err, "unable to start manager") + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } } @@ -98,177 +96,145 @@ func NewRunCommand() *cobra.Command { // run executes the main logic of the sync-global-pullsecret command func (o *syncGlobalPullSecretOptions) run(ctx context.Context) error { - // Create manager - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ - Scheme: hyperapi.Scheme, - Cache: cache.Options{ - DefaultNamespaces: map[string]cache.Config{defaultGlobalPullSecretNamespace: {}}, - }, - }) + // Setup logger using zap with logr interface + config := zap.NewProductionConfig() + config.EncoderConfig.TimeKey = "timestamp" + config.EncoderConfig.EncodeTime = zapcore.RFC3339TimeEncoder + zapLogger, err := config.Build() if err != nil { - return fmt.Errorf("failed to create manager: %w", err) + return fmt.Errorf("failed to create logger: %w", err) } + logger := zapr.NewLogger(zapLogger) - uncachedClientRestConfig := mgr.GetConfig() - uncachedClientRestConfig.WarningHandler = rest.NoWarnings{} - uncachedClient, err := crclient.New(uncachedClientRestConfig, crclient.Options{ - Scheme: mgr.GetScheme(), - Mapper: mgr.GetRESTMapper(), - }) - if err != nil { - return fmt.Errorf("failed to create uncached client: %w", err) - } - - // Create reconciler - r := &GlobalPullSecretReconciler{ - cachedClient: mgr.GetClient(), - uncachedClient: uncachedClient, - Scheme: mgr.GetScheme(), + // Create syncer + syncer := &GlobalPullSecretSyncer{ kubeletConfigJsonPath: o.kubeletConfigJsonPath, - globalPSSecretName: o.globalPSSecretName, - globalPSSecretNS: defaultGlobalPullSecretNamespace, + log: logger, } - // Create controller - if err := ctrl.NewControllerManagedBy(mgr). - For(&corev1.Secret{}). - WithEventFilter(predicate.Funcs{ - // Adding filters to avoid processing events that are not relevant - CreateFunc: func(e event.CreateEvent) bool { - return o.isTargetSecret(e.Object) - }, - UpdateFunc: func(e event.UpdateEvent) bool { - return o.isTargetSecret(e.ObjectNew) - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return o.isTargetSecret(e.Object) - }, - GenericFunc: func(e event.GenericEvent) bool { - return false - }, - }). - Complete(r); err != nil { - return fmt.Errorf("failed to create controller: %w", err) - } + // Start the sync loop + return syncer.runSyncLoop(ctx) +} - // Start manager - if err := mgr.Start(ctx); err != nil { - return fmt.Errorf("failed to start manager: %w", err) - } +// runSyncLoop runs the main synchronization loop with backoff +func (s *GlobalPullSecretSyncer) runSyncLoop(ctx context.Context) error { + s.log.Info("Starting global pull secret sync loop") - return nil -} + // Initial sync + if err := s.syncPullSecret(); err != nil { + s.log.Error(err, "Initial sync failed") + } -// isTargetSecret checks if the given object is the target secret we want to watch -func (o *syncGlobalPullSecretOptions) isTargetSecret(obj crclient.Object) bool { - // Check if it's a Secret and has the correct name and namespace - if secret, ok := obj.(*corev1.Secret); ok { - return secret.GetNamespace() == defaultGlobalPullSecretNamespace && - secret.GetName() == o.globalPSSecretName + // Sync loop with backoff + ticker := time.NewTicker(tickerPace) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + s.log.Info("Context canceled, stopping sync loop") + return nil + case <-ticker.C: + if err := s.syncPullSecret(); err != nil { + s.log.Error(err, "Sync failed") + // Continue the loop even if sync fails + } + } } - return false } -// Reconcile handles the reconciliation logic for the GlobalPullSecret -func (r *GlobalPullSecretReconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { - var ( - log = ctrl.LoggerFrom(ctx) - globalPullSecret = &corev1.Secret{} - originalPullSecret = &corev1.Secret{} - err error - chosenPullSecret *corev1.Secret - ) - - log.Info("Reconciling GlobalPullSecret") - err = r.uncachedClient.Get(ctx, types.NamespacedName{ - Name: originalPullSecretName, - Namespace: originalPullSecretNamespace, - }, originalPullSecret) - if err != nil { - return reconcile.Result{}, fmt.Errorf("failed to get original pull secret: %w", err) - } +// syncPullSecret handles the synchronization logic for the GlobalPullSecret +func (s *GlobalPullSecretSyncer) syncPullSecret() error { + s.log.Info("Syncing global pull secret") - err = r.cachedClient.Get(ctx, req.NamespacedName, globalPullSecret) + // Try to read the global pull secret from mounted file first + globalPullSecretBytes, err := readPullSecretFromFile(globalPullSecretFilePath) if err != nil { - if !apierrors.IsNotFound(err) { - return reconcile.Result{}, fmt.Errorf("failed to get global pull secret: %w", err) + if !os.IsNotExist(err) { + return fmt.Errorf("failed to read global pull secret from file: %w", err) + } + // If global pull secret file doesn't exist, fall back to original pull secret + s.log.Info("Global pull secret file not found, using original pull secret") + originalPullSecretBytes, err := readPullSecretFromFile(originalPullSecretFilePath) + if err != nil { + return fmt.Errorf("failed to read original pull secret from file: %w", err) } - log.Info("Global pull secret not found, using original pull secret") - chosenPullSecret = originalPullSecret.DeepCopy() + globalPullSecretBytes = originalPullSecretBytes } else { - log.Info("Global pull secret found, using it") - chosenPullSecret = globalPullSecret.DeepCopy() + if len(globalPullSecretBytes) == 0 { + s.log.Info("Global pull secret file is empty, using original pull secret") + originalPullSecretBytes, err := readPullSecretFromFile(originalPullSecretFilePath) + if err != nil { + return fmt.Errorf("failed to read original pull secret from file: %w", err) + } + globalPullSecretBytes = originalPullSecretBytes + } else { + s.log.Info("Global pull secret content found, using it") + } } - // Normal reconciliation - if err := r.checkAndFixFile(ctx, chosenPullSecret); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to check and fix file: %w", err) + if err := s.checkAndFixFile(globalPullSecretBytes); err != nil { + return fmt.Errorf("failed to check and fix file: %w", err) } - return reconcile.Result{}, nil + return nil } // checkAndFixFile reads the current file content and updates it if it differs from the desired content (global pull secret content). -// Have in mind the logic which do the merge of the pull secret is in the globalpullsecret package under the HCCO. -func (r *GlobalPullSecretReconciler) checkAndFixFile(ctx context.Context, pullSecret *corev1.Secret) error { - log := ctrl.LoggerFrom(ctx) - log.Info("Checking and fixing file") - - // Validate pullSecret is not nil - if pullSecret == nil { - return fmt.Errorf("pullSecret cannot be nil") - } - - // Validate pullSecret.Data is not nil - if pullSecret.Data == nil { - return fmt.Errorf("pullSecret.Data cannot be nil") - } +func (s *GlobalPullSecretSyncer) checkAndFixFile(pullSecretBytes []byte) error { + s.log.Info("Checking Kubelet's config.json file content") - log.Info("DEBUG: Pass Data check") - // Validate the required key exists - pullSecretBytes, exists := pullSecret.Data[corev1.DockerConfigJsonKey] - if !exists { - return fmt.Errorf("pullSecret does not contain required key: %s", corev1.DockerConfigJsonKey) + // Basic sanity check + if err := validateDockerConfigJSON(pullSecretBytes); err != nil { + return fmt.Errorf("invalid docker config.json content: %w", err) } // Read existing content if file exists - existingContent, err := os.ReadFile(r.kubeletConfigJsonPath) + existingContent, err := readFileFunc(s.kubeletConfigJsonPath) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to read existing file: %w", err) } + // Preserve trailing newline if it exists in the original file + contentToWrite := pullSecretBytes + if len(existingContent) > 0 && existingContent[len(existingContent)-1] == '\n' { + if len(pullSecretBytes) == 0 || pullSecretBytes[len(pullSecretBytes)-1] != '\n' { + contentToWrite = append(pullSecretBytes, '\n') + } + } + // If file content is different, write the desired content - if string(existingContent) != string(pullSecretBytes) { - log.Info("file content is different, updating it") + if string(existingContent) != string(contentToWrite) { + s.log.Info("file content is different, updating it") // Save original content for potential rollback originalContent := existingContent // Write the new content - if err := writeFileFunc(r.kubeletConfigJsonPath, pullSecretBytes, 0600); err != nil { + if err := writeFileFunc(s.kubeletConfigJsonPath, contentToWrite, 0600); err != nil { return fmt.Errorf("failed to write file: %w", err) } - log.Info("Pull secret updated", "file", r.kubeletConfigJsonPath) + s.log.Info("Pull secret updated", "file", s.kubeletConfigJsonPath) // Attempt to restart Kubelet with retries maxRetries := 3 var lastErr error for attempt := 1; attempt <= maxRetries; attempt++ { - if err := signalKubeletToRestartProcess(ctx); err != nil { + if err := signalKubeletToRestartProcess(); err != nil { lastErr = err if attempt < maxRetries { - log.Info(fmt.Sprintf("Attempt %d failed, retrying...: %v", attempt, err)) + s.log.Info(fmt.Sprintf("Attempt %d failed, retrying...: %v", attempt, err)) time.Sleep(time.Duration(attempt) * time.Second) continue } } else { - log.Info("Successfully restarted Kubelet", "attempt", attempt) + s.log.Info("Successfully restarted Kubelet", "attempt", attempt) return nil } } // If we reach this point, all retries failed - perform rollback - log.Info("Failed to restart Kubelet after some attempts, executing rollback", "maxRetries", maxRetries, "lastErr", lastErr) - if err := writeFileFunc(r.kubeletConfigJsonPath, originalContent, 0600); err != nil { + s.log.Info("Failed to restart Kubelet after some attempts, executing rollback", "maxRetries", maxRetries, "error", lastErr) + if err := writeFileFunc(s.kubeletConfigJsonPath, originalContent, 0600); err != nil { return fmt.Errorf("2 errors happened: the kubelet restart failed after %d attempts and it failed to rollback the file: %w", maxRetries, err) } return fmt.Errorf("failed to restart kubelet after %d attempts, rolled back changes: %w", maxRetries, lastErr) @@ -279,20 +245,17 @@ func (r *GlobalPullSecretReconciler) checkAndFixFile(ctx context.Context, pullSe // signalKubeletToRestartProcess signals Kubelet to reload the config by restarting the kubelet.service. // This is done by sending a signal to systemd via dbus. -func signalKubeletToRestartProcess(ctx context.Context) error { - log := ctrl.LoggerFrom(ctx) - log.Info("Signaling Kubelet to reload config") +func signalKubeletToRestartProcess() error { conn, err := dbus.New() if err != nil { return fmt.Errorf("failed to connect to dbus: %w", err) } defer conn.Close() - return restartKubelet(ctx, conn) + return restartKubelet(conn) } -func restartKubelet(ctx context.Context, conn dbusConn) error { - log := ctrl.LoggerFrom(ctx) +func restartKubelet(conn dbusConn) error { ch := make(chan string) if _, err := conn.RestartUnit(kubeletServiceUnit, dbusRestartUnitMode, ch); err != nil { return fmt.Errorf("failed to restart kubelet: %w", err) @@ -304,6 +267,51 @@ func restartKubelet(ctx context.Context, conn dbusConn) error { return fmt.Errorf("failed to restart kubelet, result: %s", result) } - log.Info("Successfully signaled Kubelet to reload config") return nil } + +// readPullSecretFromFile reads a pull secret from a mounted file path +func readPullSecretFromFile(filePath string) ([]byte, error) { + content, err := readFileFunc(filePath) + if err != nil { + return nil, err + } + return content, nil +} + +func validateDockerConfigJSON(b []byte) error { + var m map[string]any + if err := json.Unmarshal(b, &m); err != nil { + return err + } + if _, ok := m["auths"]; !ok { + return fmt.Errorf("missing 'auths' key") + } + return nil +} + +func writeAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + f, err := os.CreateTemp(dir, ".config.json.tmp-*") + if err != nil { + return err + } + tmp := f.Name() + defer os.Remove(tmp) + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + if err := f.Chmod(perm); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + return os.Rename(tmp, path) +} diff --git a/sync-global-pullsecret/sync-global-pullsecret_test.go b/sync-global-pullsecret/sync-global-pullsecret_test.go index 8ec874cdd5fe..62f7f86d501c 100644 --- a/sync-global-pullsecret/sync-global-pullsecret_test.go +++ b/sync-global-pullsecret/sync-global-pullsecret_test.go @@ -1,7 +1,6 @@ package syncglobalpullsecret import ( - "context" "fmt" "os" "path/filepath" @@ -9,12 +8,7 @@ import ( . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - + "github.com/go-logr/logr" "go.uber.org/mock/gomock" ) @@ -92,6 +86,68 @@ func TestCheckAndFixFile(t *testing.T) { expectedFinalContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, expectError: true, }, + { + name: "preserve trailing newline when original file has one", + description: "file has trailing newline, new content doesn't, should preserve newline", + initialContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + rollbackShouldFail: false, + expectedErrorContains: []string{ + "failed to restart kubelet after 3 attempts", + "rolled back changes", + }, + expectedFinalContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", + expectError: true, + }, + { + name: "preserve single newline when both have newlines", + description: "both original file and new content have trailing newlines, should preserve single newline", + initialContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", + secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + rollbackShouldFail: false, + expectedErrorContains: []string{ + "failed to restart kubelet after 3 attempts", + "rolled back changes", + }, + expectedFinalContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", + expectError: true, + }, + { + name: "no newline when original file has none", + description: "original file has no newline, new content has newline, should preserve new content format", + initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + rollbackShouldFail: false, + expectedErrorContains: []string{ + "failed to restart kubelet after 3 attempts", + "rolled back changes", + }, + expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + expectError: true, + }, + { + name: "no newlines preserved", + description: "neither original file nor new content have newlines, should preserve format", + initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + rollbackShouldFail: false, + expectedErrorContains: []string{ + "failed to restart kubelet after 3 attempts", + "rolled back changes", + }, + expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + expectError: true, + }, + { + name: "same content with newline - no change needed", + description: "file content is identical including newline, no restart should be attempted", + initialContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + rollbackShouldFail: false, + expectedErrorContains: []string{}, + expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + expectError: false, + }, } for _, tt := range tests { @@ -106,20 +162,6 @@ func TestCheckAndFixFile(t *testing.T) { // Create test file path testFilePath := filepath.Join(tempDir, "config.json") - // Create test secret - testSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pull-secret", - Namespace: "kube-system", - }, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: []byte(tt.secretContent), - }, - } - - // Create fake client - fakeClient := fake.NewClientBuilder().WithObjects(testSecret).Build() - // Write initial content if provided if tt.initialContent != "" { err = os.WriteFile(testFilePath, []byte(tt.initialContent), 0600) @@ -133,13 +175,10 @@ func TestCheckAndFixFile(t *testing.T) { g.Expect(string(content)).To(Equal(tt.initialContent)) } - // Create reconciler for testing - reconciler := &GlobalPullSecretReconciler{ - cachedClient: fakeClient, - uncachedClient: fakeClient, + // Create syncer for testing + syncer := &GlobalPullSecretSyncer{ kubeletConfigJsonPath: testFilePath, - globalPSSecretName: "test-pull-secret", - globalPSSecretNS: "kube-system", + log: logr.Discard(), } // Save original write function and restore it after test @@ -161,7 +200,7 @@ func TestCheckAndFixFile(t *testing.T) { } // Run checkAndFixFile - err = reconciler.checkAndFixFile(context.Background(), testSecret) + err = syncer.checkAndFixFile([]byte(tt.secretContent)) // Check error expectations if tt.expectError { @@ -183,77 +222,6 @@ func TestCheckAndFixFile(t *testing.T) { } } -func TestIsTargetSecret(t *testing.T) { - tests := []struct { - name string - obj client.Object - expected bool - }{ - { - name: "correct secret", - obj: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: defaultGlobalPSSecretName, - Namespace: defaultGlobalPullSecretNamespace, - }, - }, - expected: true, - }, - { - name: "wrong namespace", - obj: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: defaultGlobalPSSecretName, - Namespace: "wrong-namespace", - }, - }, - expected: false, - }, - { - name: "wrong name", - obj: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "wrong-name", - Namespace: defaultGlobalPullSecretNamespace, - }, - }, - expected: false, - }, - { - name: "wrong name and namespace", - obj: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "wrong-name", - Namespace: "wrong-namespace", - }, - }, - expected: false, - }, - { - name: "different resource type", - obj: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: defaultGlobalPSSecretName, - Namespace: defaultGlobalPullSecretNamespace, - }, - }, - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - o := &syncGlobalPullSecretOptions{ - globalPSSecretName: defaultGlobalPSSecretName, - } - result := o.isTargetSecret(tt.obj) - if result != tt.expected { - t.Errorf("isTargetSecret() = %v, want %v", result, tt.expected) - } - }) - } -} - func TestRestartKubelet(t *testing.T) { tests := []struct { name string @@ -359,7 +327,7 @@ func TestRestartKubelet(t *testing.T) { mock := NewMockdbusConn(ctrl) tt.setupMock(mock) - err := restartKubelet(context.Background(), mock) + err := restartKubelet(mock) if err != nil { if tt.expectedError == "" { t.Errorf("unexpected error: %v", err) @@ -372,3 +340,147 @@ func TestRestartKubelet(t *testing.T) { }) } } + +func TestValidateDockerConfigJSON(t *testing.T) { + tests := []struct { + name string + input []byte + expectError bool + description string + }{ + { + name: "valid docker config with single auth", + input: []byte(`{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`), + expectError: false, + description: "valid JSON with auths key containing single registry", + }, + { + name: "valid docker config with multiple auths", + input: []byte(`{"auths":{"registry1.com":{"auth":"dGVzdDp0ZXN0"},"registry2.com":{"auth":"YW5vdGhlcjphdXRo"}}}`), + expectError: false, + description: "valid JSON with auths key containing multiple registries", + }, + { + name: "valid docker config with empty auths", + input: []byte(`{"auths":{}}`), + expectError: false, + description: "valid JSON with empty auths object", + }, + { + name: "valid docker config with additional fields", + input: []byte(`{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}},"credsStore":"desktop","credHelpers":{"registry.com":"registry-helper"}}`), + expectError: false, + description: "valid JSON with auths key and additional docker config fields", + }, + { + name: "invalid JSON - malformed", + input: []byte(`{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}`), + expectError: true, + description: "malformed JSON missing closing brace", + }, + { + name: "invalid JSON - trailing comma", + input: []byte(`{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}},}`), + expectError: true, + description: "malformed JSON with trailing comma", + }, + { + name: "invalid JSON - unquoted key", + input: []byte(`{auths:{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`), + expectError: true, + description: "malformed JSON with unquoted key", + }, + { + name: "missing auths key", + input: []byte(`{"registries":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`), + expectError: true, + description: "valid JSON but missing required auths key", + }, + { + name: "empty input", + input: []byte(``), + expectError: true, + description: "empty byte slice should fail JSON parsing", + }, + { + name: "null input", + input: []byte(`null`), + expectError: true, + description: "null JSON value should fail validation", + }, + { + name: "string input", + input: []byte(`"some string"`), + expectError: true, + description: "string JSON value should fail validation", + }, + { + name: "array input", + input: []byte(`[{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}]`), + expectError: true, + description: "array JSON value should fail validation", + }, + { + name: "number input", + input: []byte(`123`), + expectError: true, + description: "number JSON value should fail validation", + }, + { + name: "boolean input", + input: []byte(`true`), + expectError: true, + description: "boolean JSON value should fail validation", + }, + { + name: "auths key with null value", + input: []byte(`{"auths":null}`), + expectError: false, + description: "auths key with null value should be valid (auths key exists)", + }, + { + name: "auths key with string value", + input: []byte(`{"auths":"not an object"}`), + expectError: false, + description: "auths key with non-object value should be valid (auths key exists)", + }, + { + name: "auths key with array value", + input: []byte(`{"auths":[]}`), + expectError: false, + description: "auths key with array value should be valid (auths key exists)", + }, + { + name: "whitespace only", + input: []byte(` `), + expectError: true, + description: "whitespace only input should fail JSON parsing", + }, + { + name: "empty object", + input: []byte(`{}`), + expectError: true, + description: "empty object should fail validation (missing auths key)", + }, + { + name: "nested auths key", + input: []byte(`{"config":{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}}`), + expectError: true, + description: "auths key nested inside another object should fail validation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + err := validateDockerConfigJSON(tt.input) + + if tt.expectError { + g.Expect(err).To(HaveOccurred(), "Expected error for test case: %s", tt.description) + } else { + g.Expect(err).To(BeNil(), "Expected no error for test case: %s, but got: %v", tt.description, err) + } + }) + } +} diff --git a/test/e2e/autoscaling_test.go b/test/e2e/autoscaling_test.go index 198f84d97cd7..f86451d33cb5 100644 --- a/test/e2e/autoscaling_test.go +++ b/test/e2e/autoscaling_test.go @@ -42,6 +42,11 @@ func TestAutoscaling(t *testing.T) { "custom.ignore.label": "test1", } + // Set instance type to m5.xlarge for autoscaling tests to increase node capacity + if nodepool.Spec.Platform.AWS != nil { + nodepool.Spec.Platform.AWS.InstanceType = "m5.xlarge" + } + if additionalNP == nil { additionalNP = &hyperv1.NodePool{ ObjectMeta: metav1.ObjectMeta{ @@ -59,6 +64,11 @@ func TestAutoscaling(t *testing.T) { Min: 1, Max: 3, } + + // Also set m5.xlarge for the additional NodePool + if additionalNP.Spec.Platform.AWS != nil { + additionalNP.Spec.Platform.AWS.InstanceType = "m5.xlarge" + } } } } @@ -207,8 +217,8 @@ func testAutoscalingBalancing(ctx context.Context, mgtClient crclient.Client, ho BalancingIgnoredLabels: []string{ "custom.ignore.label", }, - MaxNodesTotal: ptr.To[int32](4), - MaxFreeDifferenceRatioPercent: ptr.To[int32](50), + MaxNodesTotal: ptr.To[int32](6), + MaxFreeDifferenceRatioPercent: ptr.To[int32](70), } return mgtClient.Update(ctx, hostedCluster) }) @@ -275,8 +285,8 @@ func testAutoscalingBalancing(ctx context.Context, mgtClient crclient.Client, ho // be used, not enough to have more than 1 pod per // node. workloadMemRequest := resource.MustParse(fmt.Sprintf("%v", 0.5*float32(bytes))) - expectNodes := numNodes + 2 // default nodepool(min + 1) + additional nodepool(min + 1) - workload := newWorkLoad(expectNodes, workloadMemRequest, "", globalOpts.LatestReleaseImage) + expectNodes := int32(6) // Target 6 nodes total for balancing test + workload := newWorkLoad(6, workloadMemRequest, "", globalOpts.LatestReleaseImage) err = guestClient.Create(ctx, workload) g.Expect(err).NotTo(HaveOccurred()) t.Logf("Created workload. Node: %s, memcapacity: %s, workload memory request: %s", nodes[0].Name, memCapacity.String(), workloadMemRequest.String()) @@ -287,7 +297,7 @@ func testAutoscalingBalancing(ctx context.Context, mgtClient crclient.Client, ho _ = e2eutil.WaitForNReadyNodes(t, ctx, guestClient, expectNodes, hostedCluster.Spec.Platform.Type) t.Logf("Successfully reached %d nodes", expectNodes) // Check load balancing between nodepools - e2eutil.EventuallyObjects(t, ctx, fmt.Sprintf("both nodepools (%s and %s) to have 2 replicas each", defaultNodePool.Name, additionalNodePool.Name), func(ctx context.Context) ([]*hyperv1.NodePool, error) { + e2eutil.EventuallyObjects(t, ctx, fmt.Sprintf("both nodepools (%s and %s) to have reasonable distribution totaling %d nodes", defaultNodePool.Name, additionalNodePool.Name, expectNodes), func(ctx context.Context) ([]*hyperv1.NodePool, error) { nodePools := []*hyperv1.NodePool{defaultNodePool, additionalNodePool} for _, np := range nodePools { if err := mgtClient.Get(ctx, crclient.ObjectKeyFromObject(np), np); err != nil { @@ -299,9 +309,17 @@ func testAutoscalingBalancing(ctx context.Context, mgtClient crclient.Client, ho if len(nps) != 2 { return false, fmt.Sprintf("expected 2 nodepools, got %d", len(nps)), nil } - if nps[0].Status.Replicas != 2 || nps[1].Status.Replicas != 2 { - return false, fmt.Sprintf("nodepools replicas are %d and %d, want both 2", nps[0].Status.Replicas, nps[1].Status.Replicas), nil + // Relaxing the check to allow reasonable distribution between nodepools, it's not deterministic which nodepool will get the nodes. + // This supports 2+4, 3+3, 4+2 configurations (each nodepool must have at least 2 nodes). + // With this we make sure no nodepool has ≤1 nodes and resolve flaky tests. + totalReplicas := nps[0].Status.Replicas + nps[1].Status.Replicas + if totalReplicas != 6 { + return false, fmt.Sprintf("total replicas is %d, want 6", totalReplicas), nil + } + if nps[0].Status.Replicas <= 1 || nps[1].Status.Replicas <= 1 { + return false, fmt.Sprintf("unbalanced: nodepool has ≤1 nodes (%d, %d)", nps[0].Status.Replicas, nps[1].Status.Replicas), nil } + return true, fmt.Sprintf("nodepools balanced - %s: %d, %s: %d", nps[0].Name, nps[0].Status.Replicas, nps[1].Name, nps[1].Status.Replicas), nil }}, nil, e2eutil.WithInterval(30*time.Second), e2eutil.WithTimeout(10*time.Minute)) } diff --git a/test/e2e/create_cluster_test.go b/test/e2e/create_cluster_test.go index e7df9cecdfc4..d44465e42184 100644 --- a/test/e2e/create_cluster_test.go +++ b/test/e2e/create_cluster_test.go @@ -2047,7 +2047,6 @@ func TestCreateCluster(t *testing.T) { if globalOpts.Platform == hyperv1.AzurePlatform { e2eutil.EnsureKubeAPIServerAllowedCIDRs(t, ctx, mgtClient, guestConfig, hostedCluster) } - e2eutil.EnsureGlobalPullSecret(t, ctx, mgtClient, hostedCluster) }). Execute(&clusterOpts, globalOpts.Platform, globalOpts.ArtifactDir, "create-cluster", globalOpts.ServiceAccountSigningKey) } diff --git a/test/e2e/util/globalps.go b/test/e2e/util/globalps.go index 4aa11af7143c..5d8566b25835 100644 --- a/test/e2e/util/globalps.go +++ b/test/e2e/util/globalps.go @@ -4,19 +4,17 @@ import ( "context" "fmt" "testing" - "time" . "github.com/onsi/gomega" - "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" + hccomanifests "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" + hyperutil "github.com/openshift/hypershift/support/util" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/ptr" crclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -71,11 +69,9 @@ func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crcli }, }, Spec: corev1.PodSpec{ - ServiceAccountName: manifests.GlobalPullSecretDSName, - AutomountServiceAccountToken: ptr.To(true), - SecurityContext: &corev1.PodSecurityContext{}, - DNSPolicy: corev1.DNSDefault, - Tolerations: []corev1.Toleration{{Operator: corev1.TolerationOpExists}}, + SecurityContext: &corev1.PodSecurityContext{}, + DNSPolicy: corev1.DNSDefault, + Tolerations: []corev1.Toleration{{Operator: corev1.TolerationOpExists}}, Containers: []corev1.Container{ { Name: KubeletConfigVerifierDaemonSetName, @@ -166,8 +162,8 @@ func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crcli TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ - corev1.ResourceMemory: resource.MustParse("50Mi"), - corev1.ResourceCPU: resource.MustParse("40m"), + corev1.ResourceMemory: resource.MustParse("40Mi"), + corev1.ResourceCPU: resource.MustParse("20m"), }, }, }, @@ -205,18 +201,6 @@ func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crcli return guestClient.Create(ctx, daemonSet) } -// WaitForKubeletConfigVerifierDaemonSet waits for the DaemonSet to be ready -func WaitForKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crclient.Client) error { - return wait.PollUntilContextTimeout(ctx, 10*time.Second, 20*time.Minute, true, - func(ctx context.Context) (done bool, err error) { - ds := &appsv1.DaemonSet{} - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: KubeletConfigVerifierDaemonSetName, Namespace: KubeletConfigVerifierNamespace}, ds); err != nil { - return false, err - } - return ds.Status.NumberReady == ds.Status.DesiredNumberScheduled, nil - }) -} - // VerifyKubeletConfigWithDaemonSet implements complete verification using DaemonSet func VerifyKubeletConfigWithDaemonSet(t *testing.T, ctx context.Context, guestClient crclient.Client, dsImage string) { g := NewWithT(t) @@ -226,32 +210,20 @@ func VerifyKubeletConfigWithDaemonSet(t *testing.T, ctx context.Context, guestCl err := CreateKubeletConfigVerifierDaemonSet(ctx, guestClient, dsImage) g.Expect(err).NotTo(HaveOccurred(), "failed to create kubelet config verifier DaemonSet") - // Wait for the DaemonSet to be ready - t.Log("Waiting for DaemonSet to be ready") - err = WaitForKubeletConfigVerifierDaemonSet(ctx, guestClient) - g.Expect(err).NotTo(HaveOccurred(), "failed to wait for kubelet config verifier DaemonSet") + // Wait for all DaemonSets to be ready using our new generic function + t.Log("Waiting for OVN, GlobalPullSecret, Konnectivity and kubelet config verifier DaemonSets to be ready") + availableNodesCount, err := hyperutil.CountAvailableNodes(ctx, guestClient) + g.Expect(err).NotTo(HaveOccurred(), "failed to count available nodes") - // Verify that all DaemonSet pods are running - t.Log("Verifying all DaemonSet pods are running") - EventuallyObjects(t, ctx, "DaemonSet pods to be running", func(ctx context.Context) ([]*corev1.Pod, error) { - pods := &corev1.PodList{} - err := guestClient.List(ctx, pods, &crclient.ListOptions{ - Namespace: KubeletConfigVerifierNamespace, - LabelSelector: labels.Set(map[string]string{ - "name": KubeletConfigVerifierDaemonSetName, - }).AsSelector(), - }) - if err != nil { - return nil, err - } - var items []*corev1.Pod - for i := range pods.Items { - items = append(items, &pods.Items[i]) - } - return items, nil - }, nil, []Predicate[*corev1.Pod]{func(pod *corev1.Pod) (done bool, reasons string, err error) { - return pod.Status.Phase == corev1.PodRunning, fmt.Sprintf("Pod has phase %s", pod.Status.Phase), nil - }}, WithInterval(5*time.Second), WithTimeout(30*time.Minute)) + daemonSetsToCheck := []DaemonSetManifest{ + {GetFunc: OpenshiftOVNKubeDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.GlobalPullSecretDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.KonnectivityAgentDaemonSet, AllowPartialNodes: false}, + {GetFunc: KubeletConfigVerifierDaemonSet, AllowPartialNodes: true}, + } + + err = waitForDaemonSetsReady(t, ctx, guestClient, daemonSetsToCheck, availableNodesCount) + g.Expect(err).NotTo(HaveOccurred(), "failed to wait for DaemonSets to be ready") // Clean up the DaemonSet after verification t.Log("Cleaning up kubelet config verifier DaemonSet") @@ -273,35 +245,23 @@ func VerifyKubeletConfigWithDaemonSet(t *testing.T, ctx context.Context, guestCl g.Expect(guestClient.Delete(ctx, pullSecret)).To(Succeed()) } -// GetKubeletConfigVerifierLogs gets logs from all pods of the kubelet config verifier DaemonSet -func GetKubeletConfigVerifierLogs(ctx context.Context, guestClient crclient.Client) (map[string]string, error) { - pods := &corev1.PodList{} - err := guestClient.List(ctx, pods, &crclient.ListOptions{ - Namespace: KubeletConfigVerifierNamespace, - LabelSelector: labels.Set(map[string]string{ - "name": KubeletConfigVerifierDaemonSetName, - }).AsSelector(), - }) - if err != nil { - return nil, fmt.Errorf("failed to list verifier pods: %w", err) +// Manifests +// KubeletConfigVerifierDaemonSet returns a manifest for the kubelet config verifier DaemonSet +func KubeletConfigVerifierDaemonSet() *appsv1.DaemonSet { + return &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: KubeletConfigVerifierDaemonSetName, + Namespace: KubeletConfigVerifierNamespace, + }, } +} - logs := make(map[string]string) - for _, pod := range pods.Items { - // Get logs from the container - logs[pod.Name] = fmt.Sprintf("Pod Phase: %s\n", pod.Status.Phase) - - // Add container status information - for _, container := range pod.Status.ContainerStatuses { - logs[pod.Name] += fmt.Sprintf("Container %s: Ready=%v, RestartCount=%d\n", - container.Name, container.Ready, container.RestartCount) - - if container.State.Terminated != nil { - logs[pod.Name] += fmt.Sprintf("Exit Code: %d, Reason: %s\n", - container.State.Terminated.ExitCode, container.State.Terminated.Reason) - } - } +// OpenshiftOVNKubeDaemonSet returns a manifest for the OVN-Kubernetes DaemonSet +func OpenshiftOVNKubeDaemonSet() *appsv1.DaemonSet { + return &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ovnkube-node", + Namespace: "openshift-ovn-kubernetes", + }, } - - return logs, nil } diff --git a/test/e2e/util/hypershift_framework.go b/test/e2e/util/hypershift_framework.go index c8ce268580ac..5dc863234203 100644 --- a/test/e2e/util/hypershift_framework.go +++ b/test/e2e/util/hypershift_framework.go @@ -214,6 +214,13 @@ func (h *hypershiftTest) after(hostedCluster *hyperv1.HostedCluster, platform hy } ValidateHostedClusterConditions(t, t.Context(), h.client, hostedCluster, hasWorkerNodes, 10*time.Minute) } + + // Run EnsureGlobalPullSecret at the end to avoid interference with upgrade tests + // that may have executed earlier in the same cluster. This test modifies + // /var/lib/kubelet/config.json and can cause disk validation failures in upgrades. + t.Run("EnsureGlobalPullSecret", func(t *testing.T) { + EnsureGlobalPullSecret(t, context.Background(), h.client, hostedCluster) + }) }) } diff --git a/test/e2e/util/util.go b/test/e2e/util/util.go index ef6ccadd4643..0b080f16d41b 100644 --- a/test/e2e/util/util.go +++ b/test/e2e/util/util.go @@ -31,7 +31,7 @@ import ( "github.com/openshift/hypershift/support/conditions" suppconfig "github.com/openshift/hypershift/support/config" "github.com/openshift/hypershift/support/releaseinfo" - "github.com/openshift/hypershift/support/releaseinfo/registryclient" + "github.com/openshift/hypershift/support/util" hyperutil "github.com/openshift/hypershift/support/util" configv1 "github.com/openshift/api/config/v1" @@ -65,6 +65,7 @@ import ( "k8s.io/utils/ptr" capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" crclient "sigs.k8s.io/controller-runtime/pkg/client" "github.com/go-logr/logr" @@ -1807,219 +1808,208 @@ func EnsureGuestWebhooksValidated(t *testing.T, ctx context.Context, guestClient }) } -func EnsureGlobalPullSecret(t *testing.T, ctx context.Context, mgmtClient crclient.Client, entryHostedCluster *hyperv1.HostedCluster) error { - t.Run("EnsureGlobalPullSecret", func(t *testing.T) { - AtLeast(t, Version419) - // TODO (jparrill): Change check of release version `releaseVersion.GT(Version420)` to `releaseVersion.GE(Version420)` - // during the backport to 4.20 of this PR https://github.com/openshift/hypershift/pull/6736 - if entryHostedCluster.Spec.Platform.Type != hyperv1.AzurePlatform && entryHostedCluster.Spec.Platform.Type != hyperv1.AWSPlatform { - t.Skip("test only supported on platform ARO or AWS") - } +func EnsureGlobalPullSecret(t *testing.T, ctx context.Context, mgmtClient crclient.Client, entryHostedCluster *hyperv1.HostedCluster) { + AtLeast(t, Version419) + // TODO (jparrill): Change check of release version `releaseVersion.GT(Version420)` to `releaseVersion.GE(Version420)` + // during the backport to 4.20 of this PR https://github.com/openshift/hypershift/pull/6736 + if entryHostedCluster.Spec.Platform.Type != hyperv1.AzurePlatform && entryHostedCluster.Spec.Platform.Type != hyperv1.AWSPlatform { + t.Skip("test only supported on platform ARO or AWS") + } + + if entryHostedCluster.Spec.Platform.Type == hyperv1.AWSPlatform && releaseVersion.LE(Version420) { + t.Skip("AWS platform not supported on version 4.20 or less") + } - if entryHostedCluster.Spec.Platform.Type == hyperv1.AWSPlatform && releaseVersion.LE(Version420) { - t.Skip("AWS platform not supported on version 4.20 or less") + if strings.Contains(t.Name(), "TestAutoscaling") || strings.Contains(t.Name(), "TestAutoscalingBalancing") || strings.Contains(t.Name(), "TestNodePool") { + t.Skip("Skip GlobalPullSecret test for NodePool and Autoscaling tests to avoid issues with the daemon set") + } + + // due to this bug: https://issues.redhat.com/browse/OCPBUGS-63743 we should skip the TestCreateClusterCustomConfig + // This tests adds a custom network configuration to operatorConfiguration that causes the ovnkube-node and multus DS to crashLoop + // after the triggers the kubelet restart + if strings.Contains(t.Name(), "TestCreateClusterCustomConfig") { + t.Skip("Skip GlobalPullSecret test for TestCreateClusterCustomConfig to avoid issues with OVN") + } + + if !util.IsPublicHC(entryHostedCluster) { + t.Skip("test only supported on public clusters") + } + + var ( + dummyImageTagMultiarch = "quay.io/hypershift/sleep:multiarch" + dummyImageTag12 = "quay.io/hypershift/sleep:1.2.0" + err error + + // Additional Pull Secret + additionalPullSecretName = "additional-pull-secret" + additionalPullSecretNamespace = "kube-system" + additionalPullSecretDummyData = []byte(`{"auths": {"quay.io": {"auth": "YWRtaW46cGFzc3dvcmQ="}}}`) + additionalPullSecretReadOnlyE2EData = []byte(`{"auths": {"quay.io/hypershift": {"auth": "aHlwZXJzaGlmdCtlMmVfcmVhZG9ubHk6R1U2V0ZDTzVaVkJHVDJPREE1VVAxT0lCOVlNMFg2TlY0UkZCT1lJSjE3TDBWOFpTVlFGVE5BS0daNTNNQVAzRA=="}}}`) + oldglobalPullSecretData []byte + dsImage string + g = NewWithT(t) + ) + + guestClient := WaitForGuestClient(t, ctx, mgmtClient, entryHostedCluster) + + // Get NodePool List + npList := &hyperv1.NodePoolList{} + err = mgmtClient.List(ctx, npList, crclient.InNamespace(entryHostedCluster.Namespace)) + if err != nil { + if apierrors.IsNotFound(err) { + t.Skip("NodePool is not found, skipping EnsureGlobalPullSecret test") } + g.Expect(err).NotTo(HaveOccurred(), "failed to get NodePoolList") + } - var ( - dummyImageTagMultiarch = "quay.io/hypershift/sleep:multiarch" - dummyImageTag12 = "quay.io/hypershift/sleep:1.2.0" - err error - - // Additional Pull Secret - additionalPullSecretName = "additional-pull-secret" - additionalPullSecretNamespace = "kube-system" - pullSecretNamespace = "openshift-config" - additionalPullSecretDummyData = []byte(`{"auths": {"quay.io": {"auth": "YWRtaW46cGFzc3dvcmQ="}}}`) - additionalPullSecretReadOnlyE2EData = []byte(`{"auths": {"quay.io": {"auth": "aHlwZXJzaGlmdCtlMmVfcmVhZG9ubHk6R1U2V0ZDTzVaVkJHVDJPREE1VVAxT0lCOVlNMFg2TlY0UkZCT1lJSjE3TDBWOFpTVlFGVE5BS0daNTNNQVAzRA=="}}}`) - oldglobalPullSecretData []byte - dsImage string - g = NewWithT(t) - ) + // Get the first NodePool + np := &hyperv1.NodePool{} + err = mgmtClient.Get(ctx, client.ObjectKey{Name: npList.Items[0].Name, Namespace: npList.Items[0].Namespace}, np) + g.Expect(err).NotTo(HaveOccurred(), "failed to get NodePool") + g.Expect(np.Spec.Replicas).NotTo(BeNil(), "NodePool replicas are not set") - guestClient := WaitForGuestClient(t, ctx, mgmtClient, entryHostedCluster) + if np.Spec.Management.UpgradeType == hyperv1.UpgradeTypeInPlace { + t.Skip("InPlace upgrade type is not supported for GlobalPullSecret") + } - // Create the additional-pull-secret secret in the DataPlane using the dummy pull secret. - // The dummy pull secret is authorized to pull restricted images. - err = createAdditionalPullSecret(ctx, guestClient, additionalPullSecretDummyData, additionalPullSecretName, additionalPullSecretNamespace) - g.Expect(err).NotTo(HaveOccurred(), "failed to create additional-pull-secret secret") + // Get current available nodes count instead of using NodePool replicas + // This is because the test runs in parallel with other tests, and the actual number of nodes + // may differ from the NodePool replicas due to multi-zone configuration or other test interference + nodeCount, err := hyperutil.CountAvailableNodes(ctx, guestClient) + g.Expect(err).NotTo(HaveOccurred(), "failed to count available nodes") - // Check if HCCO generates the GlobalPullSecret secret in the kube-system namespace in the DataPlane - t.Run("Check if GlobalPullSecret secret is in the right place at Dataplane", func(t *testing.T) { - globalPullSecret := hccomanifests.GlobalPullSecret() - g.Eventually(func() error { - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { - return err - } - g.Expect(globalPullSecret.Data).NotTo(BeEmpty(), "global-pull-secret secret is empty") - g.Expect(globalPullSecret.Data[corev1.DockerConfigJsonKey]).NotTo(BeEmpty(), "global-pull-secret secret is empty") - oldglobalPullSecretData = globalPullSecret.Data[corev1.DockerConfigJsonKey] - return nil - }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is not present") - }) + t.Logf("NodePool replicas: %d, Available nodes: %d", *np.Spec.Replicas, nodeCount) - // Check if the additional RBAC is present in the DataPlane - t.Run("Check if the additional RBAC is present in the DataPlane", func(t *testing.T) { - g.Eventually(func() error { - // Check RBAC in kube-system and openshift-config namespace - role := hccomanifests.GlobalPullSecretSyncerRole(additionalPullSecretNamespace) - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: role.Name, Namespace: role.Namespace}, role); err != nil { - return err - } + // Create the additional-pull-secret secret in the DataPlane using the dummy pull secret. + // The dummy pull secret is not authorized to pull restricted images. + err = createAdditionalPullSecret(ctx, guestClient, additionalPullSecretDummyData, additionalPullSecretName, additionalPullSecretNamespace) + g.Expect(err).NotTo(HaveOccurred(), "failed to create additional-pull-secret secret") - roleBinding := hccomanifests.GlobalPullSecretSyncerRoleBinding(additionalPullSecretNamespace) - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: roleBinding.Name, Namespace: roleBinding.Namespace}, roleBinding); err != nil { - return err - } + // Check if HCCO generates the GlobalPullSecret secret in the kube-system namespace in the DataPlane + t.Run("Check if GlobalPullSecret secret is in the right place at Dataplane", func(t *testing.T) { + globalPullSecret := hccomanifests.GlobalPullSecret() + g.Eventually(func() error { + if err := guestClient.Get(ctx, client.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { + return err + } + g.Expect(globalPullSecret.Data).NotTo(BeEmpty(), "global-pull-secret secret is empty") + g.Expect(globalPullSecret.Data[corev1.DockerConfigJsonKey]).NotTo(BeEmpty(), "global-pull-secret secret is empty") + oldglobalPullSecretData = globalPullSecret.Data[corev1.DockerConfigJsonKey] + return nil + }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is not present") + }) - openshiftConfigRole := hccomanifests.GlobalPullSecretSyncerRole(pullSecretNamespace) - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: openshiftConfigRole.Name, Namespace: openshiftConfigRole.Namespace}, openshiftConfigRole); err != nil { - return err - } + // Check if the DaemonSet is present in the DataPlane + t.Run("Check if the DaemonSet is present in the DataPlane", func(t *testing.T) { + g.Eventually(func() error { + daemonSet := hccomanifests.GlobalPullSecretDaemonSet() + if err := guestClient.Get(ctx, client.ObjectKey{Name: daemonSet.Name, Namespace: daemonSet.Namespace}, daemonSet); err != nil { + return err + } + dsImage = daemonSet.Spec.Template.Spec.Containers[0].Image + return nil + }, 30*time.Second, 5*time.Second).Should(Succeed(), "DaemonSet is not present") + }) - openshiftConfigRoleBinding := hccomanifests.GlobalPullSecretSyncerRoleBinding(pullSecretNamespace) - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: openshiftConfigRoleBinding.Name, Namespace: openshiftConfigRoleBinding.Namespace}, openshiftConfigRoleBinding); err != nil { - return err - } + t.Run("Wait for critical DaemonSets to be ready - first check", func(t *testing.T) { + daemonSetsToCheck := []DaemonSetManifest{ + {GetFunc: OpenshiftOVNKubeDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.GlobalPullSecretDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.KonnectivityAgentDaemonSet, AllowPartialNodes: false}, + } - serviceAccount := hccomanifests.GlobalPullSecretSyncerServiceAccount() - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: serviceAccount.Name, Namespace: serviceAccount.Namespace}, serviceAccount); err != nil { - return err - } + err := waitForDaemonSetsReady(t, ctx, guestClient, daemonSetsToCheck, nodeCount) + g.Expect(err).NotTo(HaveOccurred(), "failed to wait for DaemonSets to be ready") + }) - return nil - }, 30*time.Second, 5*time.Second).Should(Succeed(), "RBAC is not present") - }) + // Create a pod which uses the restricted image, should fail + t.Run("Create a pod which uses the restricted image, should fail", func(t *testing.T) { + shouldFail := true + runAndCheckPod(t, ctx, guestClient, dummyImageTagMultiarch, additionalPullSecretNamespace, "global-pull-secret-fail", shouldFail) + }) - // Check if the DaemonSet is present in the DataPlane - t.Run("Check if the DaemonSet is present in the DataPlane", func(t *testing.T) { - g.Eventually(func() error { - daemonSet := hccomanifests.GlobalPullSecretDaemonSet() - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: daemonSet.Name, Namespace: daemonSet.Namespace}, daemonSet); err != nil { - return err - } - dsImage = daemonSet.Spec.Template.Spec.Containers[0].Image - return nil - }, 30*time.Second, 5*time.Second).Should(Succeed(), "DaemonSet is not present") - }) + // Modify the additional-pull-secret secret in the DataPlane + t.Run("Modify the additional-pull-secret secret in the DataPlane by adding the valid pull secret", func(t *testing.T) { + additionalPullSecret := hccomanifests.AdditionalPullSecret() + err := guestClient.Get(ctx, client.ObjectKey{Name: additionalPullSecret.Name, Namespace: additionalPullSecret.Namespace}, additionalPullSecret) + g.Expect(err).NotTo(HaveOccurred(), "failed to get additional-pull-secret secret") + additionalPullSecret.Data[corev1.DockerConfigJsonKey] = additionalPullSecretReadOnlyE2EData + err = guestClient.Update(ctx, additionalPullSecret) + g.Expect(err).NotTo(HaveOccurred(), "failed to update additional-pull-secret secret") + }) - // Check if we can pull restricted images - t.Run("Check if we can pull restricted images, should fail", func(t *testing.T) { - g.Eventually(func() error { - globalPullSecret := hccomanifests.GlobalPullSecret() - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { - return err - } - pullSecretData := globalPullSecret.Data[corev1.DockerConfigJsonKey] - _, _, _, err := registryclient.GetMetadata(ctx, dummyImageTagMultiarch, pullSecretData) - if err == nil { - return fmt.Errorf("succeeded to get metadata for restricted image, should fail") - } - return nil - }, 1*time.Minute, 5*time.Second).Should(Succeed(), "should not be able to get repo setup") - }) + // Check if GlobalPullSecret secret is updated in the DataPlane + t.Run("Check if GlobalPullSecret secret is updated in the DataPlane", func(t *testing.T) { + globalPullSecret := hccomanifests.GlobalPullSecret() + g.Eventually(func() error { + if err := guestClient.Get(ctx, client.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { + return err + } + g.Expect(globalPullSecret.Data[corev1.DockerConfigJsonKey]).NotTo(BeEmpty(), "global-pull-secret secret is empty") + if bytes.Equal(globalPullSecret.Data[corev1.DockerConfigJsonKey], oldglobalPullSecretData) { + return fmt.Errorf("global-pull-secret secret is equal to the old global-pull-secret secret, should be different") + } + return nil + }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is not updated") + }) - // Create a pod which uses the restricted image, should fail - t.Run("Create a pod which uses the restricted image, should fail", func(t *testing.T) { - shouldFail := true - runAndCheckPod(t, ctx, guestClient, dummyImageTagMultiarch, additionalPullSecretNamespace, "global-pull-secret-fail", shouldFail) - }) + t.Run("Wait for critical DaemonSets to be ready - second check", func(t *testing.T) { + daemonSetsToCheck := []DaemonSetManifest{ + {GetFunc: OpenshiftOVNKubeDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.GlobalPullSecretDaemonSet, AllowPartialNodes: false}, + {GetFunc: hccomanifests.KonnectivityAgentDaemonSet, AllowPartialNodes: false}, + } - // Modify the additional-pull-secret secret in the DataPlane - t.Run("Modify the additional-pull-secret secret in the DataPlane by adding the valid pull secret", func(t *testing.T) { - additionalPullSecret := hccomanifests.AdditionalPullSecret() - err := guestClient.Get(ctx, crclient.ObjectKey{Name: additionalPullSecret.Name, Namespace: additionalPullSecret.Namespace}, additionalPullSecret) - g.Expect(err).NotTo(HaveOccurred(), "failed to get additional-pull-secret secret") - additionalPullSecret.Data[corev1.DockerConfigJsonKey] = additionalPullSecretReadOnlyE2EData - err = guestClient.Update(ctx, additionalPullSecret) - g.Expect(err).NotTo(HaveOccurred(), "failed to update additional-pull-secret secret") - }) + err := waitForDaemonSetsReady(t, ctx, guestClient, daemonSetsToCheck, nodeCount) + g.Expect(err).NotTo(HaveOccurred(), "failed to wait for DaemonSets to be ready") + }) - // Check if GlobalPullSecret secret is updated in the DataPlane - t.Run("Check if GlobalPullSecret secret is updated in the DataPlane", func(t *testing.T) { - globalPullSecret := hccomanifests.GlobalPullSecret() - g.Eventually(func() error { - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { - return err - } - g.Expect(globalPullSecret.Data[corev1.DockerConfigJsonKey]).NotTo(BeEmpty(), "global-pull-secret secret is empty") - if bytes.Equal(globalPullSecret.Data[corev1.DockerConfigJsonKey], oldglobalPullSecretData) { - return fmt.Errorf("global-pull-secret secret is equal to the old global-pull-secret secret, should be different") - } - return nil - }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is not updated") - }) + // Check if we can run a pod with the restricted image + t.Run("Create a pod which uses the restricted image, should succeed", func(t *testing.T) { + shouldFail := false + runAndCheckPod(t, ctx, guestClient, dummyImageTag12, additionalPullSecretNamespace, "global-pull-secret-success", shouldFail) + }) - // Check if we can pull other restricted images, should succeed - t.Run("Check if we can pull other restricted images, should succeed", func(t *testing.T) { - g.Eventually(func() error { - globalPullSecret := hccomanifests.GlobalPullSecret() - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { + // Delete the additional-pull-secret secret in the DataPlane + t.Log("Deleting the additional-pull-secret secret in the DataPlane") + err = guestClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: additionalPullSecretName, Namespace: additionalPullSecretNamespace}}) + g.Expect(err).NotTo(HaveOccurred(), "failed to delete additional-pull-secret secret") + + // Check if the GlobalPullSecret secret is deleted in the DataPlane + t.Run("Check if the GlobalPullSecret secret is deleted in the DataPlane", func(t *testing.T) { + g.Eventually(func() error { + globalPullSecret := hccomanifests.GlobalPullSecret() + if err := guestClient.Get(ctx, client.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { + if !apierrors.IsNotFound(err) { return err } - pullSecretData := globalPullSecret.Data[corev1.DockerConfigJsonKey] - _, _, _, err := registryclient.GetMetadata(ctx, dummyImageTag12, pullSecretData) - if err != nil { - return fmt.Errorf("failed to get metadata for restricted image: %v", err) - } return nil - }, 1*time.Minute, 5*time.Second).Should(Succeed(), "should be able to pull other restricted images") - }) - - // Check if we can run a pod with the restricted image - t.Run("Create a pod which uses the restricted image, should succeed", func(t *testing.T) { - shouldFail := false - runAndCheckPod(t, ctx, guestClient, dummyImageTag12, additionalPullSecretNamespace, "global-pull-secret-success", shouldFail) - }) + } + return fmt.Errorf("global-pull-secret secret is still present") + }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is still present") + }) - // Delete the additional-pull-secret secret in the DataPlane - t.Log("Deleting the additional-pull-secret secret in the DataPlane") - err = guestClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: additionalPullSecretName, Namespace: additionalPullSecretNamespace}}) - g.Expect(err).NotTo(HaveOccurred(), "failed to delete additional-pull-secret secret") + // Wait for all nodes to stabilize after global-pull-secret deletion + t.Run("Wait for pull secret synchronization to stabilize across all nodes", func(t *testing.T) { + t.Log("Waiting for GlobalPullSecretDaemonSet to process the deletion and stabilize all nodes") - // Check if the GlobalPullSecret secret is deleted in the DataPlane - t.Run("Check if the GlobalPullSecret secret is deleted in the DataPlane", func(t *testing.T) { - g.Eventually(func() error { - globalPullSecret := hccomanifests.GlobalPullSecret() - if err := guestClient.Get(ctx, crclient.ObjectKey{Name: globalPullSecret.Name, Namespace: globalPullSecret.Namespace}, globalPullSecret); err != nil { - if !apierrors.IsNotFound(err) { - return err - } - return nil - } - return fmt.Errorf("global-pull-secret secret is still present") - }, 30*time.Second, 5*time.Second).Should(Succeed(), "global-pull-secret secret is still present") - }) + // Get current available nodes count instead of using DesiredNumberScheduled + // This is because the test runs in parallel with other tests, and some nodes may be unavailable, causing flakes in the CI. + availableNodesCount, err := hyperutil.CountAvailableNodes(ctx, guestClient) + g.Expect(err).NotTo(HaveOccurred(), "failed to count available nodes") - // Wait for all nodes to stabilize after global-pull-secret deletion - t.Run("Wait for pull secret synchronization to stabilize across all nodes", func(t *testing.T) { - t.Log("Waiting for GlobalPullSecretDaemonSet to process the deletion and stabilize all nodes") - - // Wait for the GlobalPullSecretDaemonSet to be ready and stable after processing the deletion - EventuallyObject(t, ctx, "GlobalPullSecretDaemonSet to be ready after global-pull-secret deletion", func(ctx context.Context) (*appsv1.DaemonSet, error) { - ds := hccomanifests.GlobalPullSecretDaemonSet() - err := guestClient.Get(ctx, crclient.ObjectKey{Name: ds.Name, Namespace: ds.Namespace}, ds) - return ds, err - }, []Predicate[*appsv1.DaemonSet]{func(ds *appsv1.DaemonSet) (done bool, reasons string, err error) { - if ds.Status.ObservedGeneration < ds.Generation { - return false, fmt.Sprintf("DaemonSet status has not observed generation %d yet (current %d)", ds.Generation, ds.Status.ObservedGeneration), nil - } - if ds.Status.UpdatedNumberScheduled != ds.Status.DesiredNumberScheduled { - return false, fmt.Sprintf("DaemonSet update in flight: %d/%d pods updated", ds.Status.UpdatedNumberScheduled, ds.Status.DesiredNumberScheduled), nil - } - if ds.Status.NumberReady != ds.Status.DesiredNumberScheduled { - return false, fmt.Sprintf("DaemonSet not ready: %d/%d pods ready", ds.Status.NumberReady, ds.Status.DesiredNumberScheduled), nil - } - return true, fmt.Sprintf("DaemonSet ready: %d/%d pods", ds.Status.NumberReady, ds.Status.DesiredNumberScheduled), nil - }}, WithTimeout(5*time.Minute), WithInterval(10*time.Second)) - }) + daemonSetsToCheck := []DaemonSetManifest{ + {GetFunc: hccomanifests.GlobalPullSecretDaemonSet, AllowPartialNodes: true}, + } - // Check if the config.json is updated in all of the nodes - t.Run("Check if the config.json is correct in all of the nodes", func(t *testing.T) { - VerifyKubeletConfigWithDaemonSet(t, ctx, guestClient, dsImage) - }) + err = waitForDaemonSetsReady(t, ctx, guestClient, daemonSetsToCheck, availableNodesCount) + g.Expect(err).NotTo(HaveOccurred(), "failed to wait for DaemonSets to be ready") }) - return nil + // Check if the config.json is updated in all of the nodes + t.Run("Check if the config.json is correct in all of the nodes", func(t *testing.T) { + VerifyKubeletConfigWithDaemonSet(t, ctx, guestClient, dsImage) + }) } func createAdditionalPullSecret(ctx context.Context, guestClient crclient.Client, pullSecretData []byte, registrySecretName, registryNamespace string) error { @@ -2041,6 +2031,77 @@ func createAdditionalPullSecret(ctx context.Context, guestClient crclient.Client return nil } +// DaemonSetManifest represents a DaemonSet to be verified +type DaemonSetManifest struct { + GetFunc func() *appsv1.DaemonSet + AllowPartialNodes bool +} + +// waitForDaemonSetsReady waits for all specified DaemonSets to be ready +func waitForDaemonSetsReady(t *testing.T, ctx context.Context, guestClient crclient.Client, daemonSets []DaemonSetManifest, nodeCount int32) error { + for _, dsManifest := range daemonSets { + daemonSetTemplate := dsManifest.GetFunc() + dsName := daemonSetTemplate.Name + allowPartialNodes := dsManifest.AllowPartialNodes + + if allowPartialNodes { + t.Logf("Waiting for %s DaemonSet to be ready with ≤%d available nodes", dsName, nodeCount) + } else { + t.Logf("Waiting for %s DaemonSet to be ready with %d nodes", dsName, nodeCount) + } + + err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 20*time.Minute, true, func(ctx context.Context) (done bool, err error) { + daemonSet := dsManifest.GetFunc() + err = guestClient.Get(ctx, crclient.ObjectKey{Name: daemonSet.Name, Namespace: daemonSet.Namespace}, daemonSet) + if err != nil { + t.Logf("Failed to get DaemonSet %s: %v", dsName, err) + return false, nil + } + + if daemonSet.Status.ObservedGeneration < daemonSet.Generation { + t.Logf("DaemonSet %s status has not observed generation %d yet (current %d)", dsName, daemonSet.Generation, daemonSet.Status.ObservedGeneration) + return false, nil + } + + actualReady := daemonSet.Status.NumberReady + + if allowPartialNodes { + // Check UpdatedNumberScheduled for partial nodes mode + if daemonSet.Status.UpdatedNumberScheduled > nodeCount { + t.Logf("DaemonSet %s update in flight: %d/%d pods updated (based on available nodes)", dsName, daemonSet.Status.UpdatedNumberScheduled, nodeCount) + return false, nil + } + + // Allow NumberReady to be <= nodeCount + if actualReady > nodeCount { + t.Logf("DaemonSet %s not ready: %d/%d pods ready (based on available nodes)", dsName, actualReady, nodeCount) + return false, nil + } + + t.Logf("DaemonSet %s ready: %d/%d pods (based on available nodes)", dsName, actualReady, nodeCount) + return true, nil + } else { + // Exact match for normal mode + if actualReady != nodeCount { + t.Logf("DaemonSet %s not ready: %d/%d pods ready", dsName, actualReady, nodeCount) + return false, nil + } + + t.Logf("DaemonSet %s ready: %d/%d pods", dsName, actualReady, nodeCount) + return true, nil + } + }) + + if err != nil { + return fmt.Errorf("failed to wait for DaemonSet %s to be ready: %w", dsName, err) + } + + t.Logf("✓ %s DaemonSet is ready", dsName) + } + + return nil +} + func EnsureKubeAPIDNSNameCustomCert(t *testing.T, ctx context.Context, mgmtClient crclient.Client, entryHostedCluster *hyperv1.HostedCluster) { t.Run("EnsureKubeAPIDNSNameCustomCert", func(t *testing.T) { AtLeast(t, Version419) @@ -3690,14 +3751,20 @@ func runAndCheckPod(t *testing.T, ctx context.Context, guestClient crclient.Clie return err } if shouldFail { - if pod.Status.ContainerStatuses != nil && pod.Status.ContainerStatuses[0].State.Waiting.Reason == "ImagePullBackOff" { - return fmt.Errorf("pod is not running") + if pod.Status.Phase == corev1.PodFailed || + (len(pod.Status.ContainerStatuses) > 0 && + ((pod.Status.ContainerStatuses[0].State.Waiting != nil && + pod.Status.ContainerStatuses[0].State.Waiting.Reason == "ImagePullBackOff") || + (pod.Status.ContainerStatuses[0].State.Terminated != nil))) { + return nil } - return nil + return fmt.Errorf("pod should fail but is not in failure state yet, current phase: %s", pod.Status.Phase) } else { + t.Logf("Pod phase: %s, shouldFail: %t", pod.Status.Phase, shouldFail) if pod.Status.Phase != corev1.PodRunning { - return fmt.Errorf("pod is running") + return fmt.Errorf("pod is not running yet, current phase: %s", pod.Status.Phase) } + t.Logf("Pod is running! Continuing...") return nil } }, 7*time.Minute, 5*time.Second).Should(Succeed(), "pod is not running")