CNF-13703: Immutable fields check for ClusterInstance (#207)

* Immutable fields check for ClusterInstance

* Move allowedFields var to constant and allow updating on scaling

* Use IP addresses reserved for documentation in unittests

config/samples/v1alpha1_clusterrequest.yaml

@@ -34,9 +34,8 @@ spec:
         AgentClusterInstall:
           extra-label-key: extra-label-value
         ManagedCluster:
-          common: "true"
-          group-du-sno: test
-          sites: site-sno-du-1
+          cluster-version: "v4.16"
+          clustertemplate-a-policy: "v1"
       ingressVIPs:
         - 192.0.2.3
       machineNetwork:

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/openshift/assisted-service/api v0.0.0-20240405132132-484ec5c683c6
 	github.com/peterhellberg/link v1.2.0
 	github.com/prometheus/client_golang v1.18.0
+	github.com/r3labs/diff/v3 v3.0.1
 	github.com/spf13/cobra v1.7.0
 	github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace
 	github.com/stolostron/siteconfig v0.0.0-20240911204707-0bba68d7dcc0
@@ -94,6 +95,8 @@ require (
 	github.com/prometheus/client_model v0.5.0 // indirect
 	github.com/prometheus/common v0.45.0 // indirect
 	github.com/prometheus/procfs v0.12.0 // indirect
+	github.com/vmihailenco/msgpack/v5 v5.3.5 // indirect
+	github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect
 	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
 	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
 	go.mongodb.org/mongo-driver v1.10.0 // indirect

go.sum

@@ -319,6 +319,8 @@ github.com/prometheus/common v0.45.0 h1:2BGz0eBc2hdMDLnO/8n0jeB3oPrt2D08CekT0lne
 github.com/prometheus/common v0.45.0/go.mod h1:YJmSTw9BoKxJplESWWxlbyttQR4uaEcGyv9MZjVOJsY=
 github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
 github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
+github.com/r3labs/diff/v3 v3.0.1 h1:CBKqf3XmNRHXKmdU7mZP1w7TV0pDyVCis1AUHtA4Xtg=
+github.com/r3labs/diff/v3 v3.0.1/go.mod h1:f1S9bourRbiM66NskseyUdo0fTmEE0qKrikYJX63dgo=
 github.com/rogpeppe/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
 github.com/rogpeppe/go-internal v1.2.2/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
 github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
@@ -360,6 +362,10 @@ github.com/thoas/go-funk v0.9.3 h1:7+nAEx3kn5ZJcnDm2Bh23N2yOtweO14bi//dvRtgLpw=
 github.com/thoas/go-funk v0.9.3/go.mod h1:+IWnUfUmFO1+WVYQWQtIJHeRRdaIyyYglZN7xzUPe4Q=
 github.com/tidwall/pretty v1.0.0 h1:HsD+QiTn7sK6flMKIvNmpqz1qrpP3Ps6jOKIKMooyg4=
 github.com/tidwall/pretty v1.0.0/go.mod h1:XNkn88O1ChpSDQmQeStsy+sBenx6DDtFZJxhVysOjyk=
+github.com/vmihailenco/msgpack/v5 v5.3.5 h1:5gO0H1iULLWGhs2H5tbAHIZTV8/cYafcFOr9znI5mJU=
+github.com/vmihailenco/msgpack/v5 v5.3.5/go.mod h1:7xyJ9e+0+9SaZT0Wt1RGleJXzli6Q/V5KbhBonMG9jc=
+github.com/vmihailenco/tagparser/v2 v2.0.0 h1:y09buUbR+b5aycVFQs/g70pqKVZNBmxwAhO7/IwNM9g=
+github.com/vmihailenco/tagparser/v2 v2.0.0/go.mod h1:Wri+At7QHww0WTrCBeu4J6bNtoV6mEfg5OIWRZA9qds=
 github.com/xdg-go/pbkdf2 v1.0.0/go.mod h1:jrpuAogTd400dnrH08LKmI/xc1MbPOebTwRqcT5RDeI=
 github.com/xdg-go/scram v1.0.2/go.mod h1:1WAq6h33pAW+iRreB34OORO2Nf7qel3VV3fjBj+hCSs=
 github.com/xdg-go/scram v1.1.1/go.mod h1:RaEWvsqvNKKvBPvcKeFjrG2cJqOkHTiyTpzz23ni57g=

internal/controllers/clusterrequest_controller.go

@@ -46,6 +46,7 @@ import (
 	oranv1alpha1 "github.com/openshift-kni/oran-o2ims/api/v1alpha1"
 	"github.com/openshift-kni/oran-o2ims/internal/controllers/utils"
 	siteconfig "github.com/stolostron/siteconfig/api/v1alpha1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
 	policiesv1 "open-cluster-management.io/governance-policy-propagator/api/v1"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -524,10 +525,53 @@ func (t *clusterRequestReconcilerTask) renderClusterInstanceTemplate(
 		labels[clusterRequestNamespaceLabel] = t.object.Namespace
 		renderedClusterInstanceUnstructure.SetLabels(labels)
 
-		// Create the ClusterInstance namespace.
-		err = t.createClusterInstanceNamespace(ctx, renderedClusterInstanceUnstructure.GetName())
+		// Create the ClusterInstance namespace if not exist.
+		ciName := renderedClusterInstanceUnstructure.GetName()
+		err = t.createClusterInstanceNamespace(ctx, ciName)
 		if err != nil {
-			return nil, fmt.Errorf("failed to create cluster namespace %s: %w", renderedClusterInstanceUnstructure.GetName(), err)
+			return nil, fmt.Errorf("failed to create cluster namespace %s: %w", ciName, err)
+		}
+
+		// Check for updates to immutable fields in the ClusterInstance, if it exists.
+		// Once provisioning has started or reached a final state (Completed or Failed),
+		// updates to immutable fields in the ClusterInstance spec are disallowed,
+		// with the exception of scaling up/down when Cluster provisioning is completed.
+		crProvisionedCond := meta.FindStatusCondition(t.object.Status.Conditions,
+			string(utils.CRconditionTypes.ClusterProvisioned))
+		if crProvisionedCond != nil && crProvisionedCond.Reason != string(utils.CRconditionReasons.Unknown) {
+			existingClusterInstance := &unstructured.Unstructured{}
+			existingClusterInstance.SetGroupVersionKind(
+				renderedClusterInstanceUnstructure.GroupVersionKind())
+			ciExists, err := utils.DoesK8SResourceExist(
+				ctx, t.client, ciName, ciName, existingClusterInstance,
+			)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get ClusterInstance (%s): %w",
+					ciName, err)
+			}
+			if ciExists {
+				updatedFields, scalingNodes, err := utils.FindClusterInstanceImmutableFieldUpdates(
+					existingClusterInstance, renderedClusterInstanceUnstructure)
+				if err != nil {
+					return nil, fmt.Errorf(
+						"failed to find immutable field updates for ClusterInstance (%s): %w", ciName, err)
+				}
+
+				var disallowedChanges []string
+				if len(updatedFields) != 0 {
+					disallowedChanges = append(disallowedChanges, updatedFields...)
+				}
+				if len(scalingNodes) != 0 &&
+					crProvisionedCond.Reason != string(utils.CRconditionReasons.Completed) {
+					// In-progress || Failed
+					disallowedChanges = append(disallowedChanges, scalingNodes...)
+				}
+
+				if len(disallowedChanges) != 0 {
+					return nil, utils.NewInputError(fmt.Sprintf(
+						"detected changes in immutable fields: %s", strings.Join(disallowedChanges, ", ")))
+				}
+			}
 		}
 
 		// Validate the rendered ClusterInstance with dry-run
@@ -750,8 +794,13 @@ func (t *clusterRequestReconcilerTask) applyClusterInstance(ctx context.Context,
 			return utils.NewInputError(err.Error())
 		}
 	} else {
-		// TODO: only update the existing clusterInstance when a list of allowed fields are changed
-		// TODO: What about if the ClusterInstance is not generated by a ClusterRequest?
+		if _, ok := clusterInstance.(*siteconfig.ClusterInstance); ok {
+			// No update needed, return
+			if equality.Semantic.DeepEqual(existingClusterInstance.Spec,
+				clusterInstance.(*siteconfig.ClusterInstance).Spec) {
+				return nil
+			}
+		}
 
 		// Make sure these fields from existing object are copied
 		clusterInstance.SetResourceVersion(existingClusterInstance.GetResourceVersion())
@@ -1277,7 +1326,7 @@ func (t *clusterRequestReconcilerTask) createClusterInstanceNamespace(
 	labels[clusterRequestNamespaceLabel] = t.object.Namespace
 	namespace.SetLabels(labels)
 
-	err := utils.CreateK8sCR(ctx, t.client, namespace, nil, utils.UPDATE)
+	err := utils.CreateK8sCR(ctx, t.client, namespace, nil, "")
 	if err != nil {
 		return fmt.Errorf("failed to create or update namespace %s: %w", clusterName, err)
 	}
@@ -1643,7 +1692,7 @@ func (r *ClusterRequestReconciler) handleFinalizer(
 			return ctrl.Result{}, true, err
 		}
 
-		// Remove clusterInstanceFinalizer. Once all finalizers have been
+		// Remove clusterRequestFinalizer. Once all finalizers have been
 		// removed, the object will be deleted.
 		r.Logger.Info("Removing ClusterRequest finalizer", "name", clusterRequest.Name)
 		patch := client.MergeFrom(clusterRequest.DeepCopy())