added additional tests for new functionality

Author: valeryia-hurynovich

controllers/managedcloudprofile_controller.go

@@ -31,6 +31,12 @@ const (
 	CloudProfileAppliedConditionType string = "CloudProfileApplied"
 )
 
+// OCIFactory provides a hook for constructing OCI sources. It defaults to
+// cloudprofilesync.NewOCI but can be overridden in tests to simulate errors.
+var OCIFactory = func(params cloudprofilesync.OCIParams, insecure bool) (cloudprofilesync.Source, error) {
+	return cloudprofilesync.NewOCI(params, insecure)
+}
+
 type Reconciler struct {
 	client.Client
 }
@@ -53,6 +59,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		cloudProfile.Spec = CloudProfileSpecToGardener(&mcp.Spec.CloudProfile)
 		errs := make([]error, 0)
 		for _, updates := range mcp.Spec.MachineImageUpdates {
+			// only call updater when an explicit source is provided
+			if updates.Source.OCI == nil {
+				continue
+			}
 			errs = append(errs, r.updateMachineImages(ctx, log, updates, &cloudProfile.Spec))
 		}
 		gardenerv1beta1.SetObjectDefaults_CloudProfile(&cloudProfile)
@@ -96,31 +106,48 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		case updates.Source.OCI != nil:
 			password, err := r.getCredential(ctx, updates.Source.OCI.Password)
 			if err != nil {
+				// credential errors already patched by getCredential caller
 				return ctrl.Result{}, err
 			}
-			oci, err := cloudprofilesync.NewOCI(cloudprofilesync.OCIParams{
+			src, err := OCIFactory(cloudprofilesync.OCIParams{
 				Registry:   updates.Source.OCI.Registry,
 				Repository: updates.Source.OCI.Repository,
 				Username:   updates.Source.OCI.Username,
 				Password:   string(password),
 				Parallel:   1,
 			}, updates.Source.OCI.Insecure)
 			if err != nil {
+				// patch status before returning so the failure is visible on the MCP
+				_ = r.patchStatusAndCondition(ctx, &mcp, v1alpha1.FailedReconcileStatus, metav1.Condition{
+					Type:               CloudProfileAppliedConditionType,
+					Status:             metav1.ConditionFalse,
+					ObservedGeneration: mcp.Generation,
+					Reason:             "GarbageCollectionFailed",
+					Message:            fmt.Sprintf("failed to initialize OCI source for garbage collection: %s", err),
+				})
 				return ctrl.Result{}, fmt.Errorf("failed to initialize OCI source for garbage collection: %w", err)
 			}
-			source = oci
+			source = src
 		default:
 			continue
 		}
 
 		versions, err := source.GetVersions(ctx)
 		if err != nil {
+			_ = r.patchStatusAndCondition(ctx, &mcp, v1alpha1.FailedReconcileStatus, metav1.Condition{
+				Type:               CloudProfileAppliedConditionType,
+				Status:             metav1.ConditionFalse,
+				ObservedGeneration: mcp.Generation,
+				Reason:             "GarbageCollectionFailed",
+				Message:            fmt.Sprintf("failed to list source versions for garbage collection: %s", err),
+			})
 			return ctrl.Result{}, fmt.Errorf("failed to list source versions for garbage collection: %w", err)
 		}
 
 		referencedVersions := r.getReferencedVersions(ctx, mcp.Name, updates.ImageName)
 
 		cutoff := time.Now().Add(-updates.GarbageCollection.MaxAge.Duration)
+		versionsToDelete := make([]string, 0)
 		for _, v := range versions {
 			if v.CreatedAt.IsZero() {
 				continue
@@ -129,36 +156,55 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 				continue
 			}
 			if v.CreatedAt.Before(cutoff) {
-				if err := r.deleteVersion(ctx, mcp.Name, updates.ImageName, v.Version); err != nil {
-					if apierrors.IsInvalid(err) {
-						log.V(1).Info("garbage collection validation failed, skipping", "image", updates.ImageName, "version", v.Version)
-						continue
-					}
-					return ctrl.Result{}, fmt.Errorf("failed to delete image version: %w", err)
+				versionsToDelete = append(versionsToDelete, v.Version)
+			}
+		}
+
+		if len(versionsToDelete) > 0 {
+			if err := r.deleteVersion(ctx, mcp.Name, updates.ImageName, versionsToDelete); err != nil {
+				if apierrors.IsInvalid(err) {
+					log.V(1).Info("garbage collection validation failed, skipping", "image", updates.ImageName)
+					continue
 				}
-				log.Info("deleted image version from CloudProfile", "image", updates.ImageName, "version", v.Version)
+				// patch status before returning so caller sees the failure
+				_ = r.patchStatusAndCondition(ctx, &mcp, v1alpha1.FailedReconcileStatus, metav1.Condition{
+					Type:               CloudProfileAppliedConditionType,
+					Status:             metav1.ConditionFalse,
+					ObservedGeneration: mcp.Generation,
+					Reason:             "GarbageCollectionFailed",
+					Message:            fmt.Sprintf("failed to delete image versions: %s", err),
+				})
+				return ctrl.Result{}, fmt.Errorf("failed to delete image versions: %w", err)
+			}
+			for _, v := range versionsToDelete {
+				log.Info("deleted image version from CloudProfile", "image", updates.ImageName, "version", v)
 			}
 		}
 	}
 
 	return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil
 }
 
-func (r *Reconciler) deleteVersion(ctx context.Context, cloudProfileName, imageName, version string) error {
+func (r *Reconciler) deleteVersion(ctx context.Context, cloudProfileName, imageName string, versions []string) error {
 	var cp gardenerv1beta1.CloudProfile
 	if err := r.Get(ctx, types.NamespacedName{Name: cloudProfileName}, &cp); err != nil {
 		return err
 	}
+
+	versionsSet := make(map[string]bool)
+	for _, v := range versions {
+		versionsSet[v] = true
+	}
+
 	for i := range cp.Spec.MachineImages {
 		if cp.Spec.MachineImages[i].Name != imageName {
 			continue
 		}
 		newVersions := make([]gardenerv1beta1.MachineImageVersion, 0, len(cp.Spec.MachineImages[i].Versions))
 		for _, mv := range cp.Spec.MachineImages[i].Versions {
-			if mv.Version == version {
-				continue
+			if !versionsSet[mv.Version] {
+				newVersions = append(newVersions, mv)
 			}
-			newVersions = append(newVersions, mv)
 		}
 		cp.Spec.MachineImages[i].Versions = newVersions
 	}
@@ -171,10 +217,16 @@ func (r *Reconciler) deleteVersion(ctx context.Context, cloudProfileName, imageN
 				}
 				filtered := make([]providercfg.MachineImageVersion, 0, len(cfg.MachineImages[i].Versions))
 				for _, mv := range cfg.MachineImages[i].Versions {
-					if strings.HasSuffix(mv.Image, ":"+version) {
-						continue
+					found := false
+					for _, version := range versions {
+						if strings.HasSuffix(mv.Image, ":"+version) {
+							found = true
+							break
+						}
+					}
+					if !found {
+						filtered = append(filtered, mv)
 					}
-					filtered = append(filtered, mv)
 				}
 				cfg.MachineImages[i].Versions = filtered
 			}
@@ -191,22 +243,40 @@ func (r *Reconciler) deleteVersion(ctx context.Context, cloudProfileName, imageN
 func (r *Reconciler) getReferencedVersions(ctx context.Context, cloudProfileName, imageName string) map[string]bool {
 	referenced := make(map[string]bool)
 
-	shootList := &gardenerv1beta1.ShootList{}
-	if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err != nil {
-		return referenced
-	}
-
-	for _, shoot := range shootList.Items {
-		if shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName {
-			continue
+	var cp gardenerv1beta1.CloudProfile
+	if err := r.Get(ctx, types.NamespacedName{Name: cloudProfileName}, &cp); err == nil {
+		if cp.Spec.ProviderConfig != nil {
+			var cfg providercfg.CloudProfileConfig
+			if err := json.Unmarshal(cp.Spec.ProviderConfig.Raw, &cfg); err == nil {
+				for _, img := range cfg.MachineImages {
+					if img.Name != imageName {
+						continue
+					}
+					for _, v := range img.Versions {
+						if idx := strings.LastIndex(v.Image, ":"); idx != -1 {
+							version := v.Image[idx+1:]
+							referenced[version] = true
+						}
+					}
+				}
+			}
 		}
+	}
 
-		for _, worker := range shoot.Spec.Provider.Workers {
-			if worker.Machine.Image == nil || worker.Machine.Image.Name != imageName {
+	shootList := &gardenerv1beta1.ShootList{}
+	if err := r.List(ctx, shootList, client.InNamespace(metav1.NamespaceAll)); err == nil {
+		for _, shoot := range shootList.Items {
+			if shoot.Spec.CloudProfile == nil || shoot.Spec.CloudProfile.Name != cloudProfileName {
 				continue
 			}
-			if worker.Machine.Image.Version != nil {
-				referenced[*worker.Machine.Image.Version] = true
+
+			for _, worker := range shoot.Spec.Provider.Workers {
+				if worker.Machine.Image == nil || worker.Machine.Image.Name != imageName {
+					continue
+				}
+				if worker.Machine.Image.Version != nil {
+					referenced[*worker.Machine.Image.Version] = true
+				}
 			}
 		}
 	}
@@ -222,7 +292,7 @@ func (r *Reconciler) updateMachineImages(ctx context.Context, log logr.Logger, u
 		if err != nil {
 			return err
 		}
-		oci, err := cloudprofilesync.NewOCI(cloudprofilesync.OCIParams{
+		src, err := OCIFactory(cloudprofilesync.OCIParams{
 			Registry:   update.Source.OCI.Registry,
 			Repository: update.Source.OCI.Repository,
 			Username:   update.Source.OCI.Username,
@@ -232,7 +302,7 @@ func (r *Reconciler) updateMachineImages(ctx context.Context, log logr.Logger, u
 		if err != nil {
 			return fmt.Errorf("failed to initialize oci source: %w", err)
 		}
-		source = oci
+		source = src
 	default:
 		return errors.New("no machine images source configured")
 	}