deploy: refactor whitelisting with custom kustomize RemoverPlugin (op…

…endatahub-io#1123)

* deploy: pass resource.Resource to manageResource

Do not convert Resource to Unstructured early but just before final
changes in Unstructured before operations on the cluster.

This will allow to run kustomize filters after getResource() call,
meaning after analysing resource state in the cluster.

This is supposed to be used to filter out whitelisted fields from
the manifests if the resource already exist and leave them as
default values if the resource is being created by the operator.

Unfortunately, it makes a bit of code duplication since the Resource
to Unstructured conversion have to be done for both create and patch
branches.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* plugins: add RemoverPlugin, field remover

Make it the same way is other builtin plugins are done implementing
an RNode filter.

Since it is supposed to be called actually to a single resource, not
the whole ResourceMap, implement extra method TransformResource.

Use Pipe*() instead of direct filter calling even that it calls only
one filter.

- first time to call the methods of the inhereted RNode object of
the Resource;
- second (PipeE) to automatically skip the result

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* cluster/gvk: add Deployment GroupVersionKind

Will be used to filter Deployments for whitelisted fields.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* plugins: remover: add tests

Test if the plugin able to remove replicas and resources from
Deployment object.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* deploy: skipUpdateOnWhitelistedFields: convert to RemoverPlugin

Use more universal way to filter whitelisted fields.

Make it possible to configure list of the fields as an array of
configured RemoverPlugins and define components to remove in a map
"Component Name" -> "pointer to the list of the fields". It makes it
possible to define them separately and reuse the list of the
fields (array of RemoverPlugins) for different components.

skipUpdateOnWhitelistedFields now takes Resource, so make the
conversion to Unstructured after it.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* update:

- update in testcase
- rename function and variables
- move var declaration into removeplugin

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: old testcase should not get replicas reconciled back

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* chore: comments

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: remove WhitelistedComponent

- we make all components resource and replicas configable

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add check on annoation for whitelistedfield

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Co-authored-by: Yauheni Kaliuta <ykaliuta@redhat.com>

pkg/cluster/gvk/gvk.go

@@ -15,6 +15,12 @@ var (
 		Kind:    "DataScienceCluster",
 	}
 
+	Deployment = schema.GroupVersionKind{
+		Group:   "apps",
+		Version: "v1",
+		Kind:    "Deployment",
+	}
+
 	KnativeServing = schema.GroupVersionKind{
 		Group:   "operator.knative.dev",
 		Version: "v1beta1",
@@ -38,10 +44,12 @@ var (
 		Version: "v1",
 		Kind:    "OdhApplication",
 	}
+
 	OdhDocument = schema.GroupVersionKind{
 		Group:   "dashboard.opendatahub.io",
 		Version: "v1", Kind: "OdhDocument",
 	}
+
 	OdhQuickStart = schema.GroupVersionKind{
 		Group:   "console.openshift.io",
 		Version: "v1", Kind: "OdhQuickStart",

pkg/conversion/conversion.go

@@ -5,7 +5,7 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/yaml"
 )
 
@@ -33,20 +33,13 @@ func StrToUnstructured(resources string) ([]*unstructured.Unstructured, error) {
 	return objs, nil
 }
 
-// ResMapToUnstructured converts a ResMap to a slice of Unstructured objects.
-func ResMapToUnstructured(resMap resmap.ResMap) ([]*unstructured.Unstructured, error) {
-	resources := make([]*unstructured.Unstructured, 0, resMap.Size())
-	for _, res := range resMap.Resources() {
-		u := &unstructured.Unstructured{}
-		asYAML, errToYAML := res.AsYAML()
-		if errToYAML != nil {
-			return nil, errToYAML
-		}
-		if errUnmarshal := yaml.Unmarshal(asYAML, u); errUnmarshal != nil {
-			return nil, errUnmarshal
-		}
-		resources = append(resources, u)
+// ResourceToUnstructured converts a resource.Resource to an Unstructured object.
+func ResourceToUnstructured(res *resource.Resource) (*unstructured.Unstructured, error) {
+	u := &unstructured.Unstructured{}
+
+	if err := yaml.Unmarshal([]byte(res.MustYaml()), u); err != nil {
+		return nil, err
 	}
 
-	return resources, nil
+	return u, nil
 }

pkg/deploy/deploy.go

@@ -41,6 +41,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/kustomize/api/krusty"
 	"sigs.k8s.io/kustomize/api/resmap"
+	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/kyaml/filesys"
 
 	"github.com/opendatahub-io/opendatahub-operator/v2/components"
@@ -183,13 +184,9 @@ func DeployManifestsFromPath(
 		return fmt.Errorf("failed applying labels plugin when preparing Kustomize resources. %w", err)
 	}
 
-	objs, err := conversion.ResMapToUnstructured(resMap)
-	if err != nil {
-		return err
-	}
 	// Create / apply / delete resources in the cluster
-	for _, obj := range objs {
-		err = manageResource(ctx, cli, obj, owner, namespace, componentName, componentEnabled)
+	for _, res := range resMap.Resources() {
+		err = manageResource(ctx, cli, res, owner, namespace, componentName, componentEnabled)
 		if err != nil {
 			return err
 		}
@@ -198,21 +195,21 @@ func DeployManifestsFromPath(
 	return nil
 }
 
-func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object, applicationNamespace, componentName string, enabled bool) error {
-	// Skip if resource is of Kind: Namespace and Name: applicationNamespace
-	if obj.GetKind() == "Namespace" && obj.GetName() == applicationNamespace {
+func manageResource(ctx context.Context, cli client.Client, res *resource.Resource, owner metav1.Object, applicationNamespace, componentName string, enabled bool) error {
+	// Return if resource is of Kind: Namespace and Name: applicationsNamespace
+	if res.GetKind() == "Namespace" && res.GetName() == applicationNamespace {
 		return nil
 	}
 
-	found, err := getResource(ctx, cli, obj)
+	found, err := getResource(ctx, cli, res)
 
 	if err != nil {
 		if !k8serr.IsNotFound(err) {
 			return err
 		}
 		// Create resource if it doesn't exist and component enabled
 		if enabled {
-			return createResource(ctx, cli, obj, owner)
+			return createResource(ctx, cli, res, owner)
 		}
 		// Skip if resource doesn't exist and component is disabled
 		return nil
@@ -226,59 +223,27 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un
 		if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
 			return nil
 		}
-		return updateResource(ctx, cli, obj, found, owner, componentName)
+		return updateResource(ctx, cli, res, found, owner)
 	}
 	// Delete resource if it exists and component is disabled
 	return handleDisabledComponent(ctx, cli, found, componentName)
 }
 
-// removeResourcesFromDeployment checks if the provided resource is a Deployment,
-// and if so, removes the resources field from each container in the Deployment. This ensures we do not overwrite the
-// resources field when Patch is applied with the returned unstructured resource.
-// TODO: remove this function once RHOAIENG-7929 is finished.
-func removeResourcesFromDeployment(u *unstructured.Unstructured) error {
-	// Check if the resource is a Deployment. This can be expanded to other resources as well.
-	if u.GetKind() != "Deployment" {
-		return nil
-	}
-	// Navigate to the containers array in the Deployment spec
-	containers, exists, err := unstructured.NestedSlice(u.Object, "spec", "template", "spec", "containers")
-	if err != nil {
-		return fmt.Errorf("error when trying to retrieve containers from Deployment: %w", err)
-	}
-	// Return if no containers exist
-	if !exists {
-		return nil
-	}
-
-	// Iterate over the containers to remove the resources field
-	for i := range containers {
-		container, ok := containers[i].(map[string]interface{})
-		// If containers field is not in expected type, return.
-		if !ok {
-			return nil
-		}
-		// Check and delete the resources field. This can be expanded to any whitelisted field.
-		delete(container, "resources")
-		containers[i] = container
-	}
-
-	// Update the containers in the original unstructured object
-	if err := unstructured.SetNestedSlice(u.Object, containers, "spec", "template", "spec", "containers"); err != nil {
-		return fmt.Errorf("failed to update containers in Deployment: %w", err)
-	}
-
-	return nil
-}
-
-func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+func getResource(ctx context.Context, cli client.Client, obj *resource.Resource) (*unstructured.Unstructured, error) {
 	found := &unstructured.Unstructured{}
+	residGvk := obj.GetGvk()
+	gvk := schema.GroupVersionKind{
+		Group:   residGvk.Group,
+		Version: residGvk.Version,
+		Kind:    residGvk.Kind,
+	}
 	// Setting gvk is required to do Get request
-	found.SetGroupVersionKind(obj.GroupVersionKind())
+	found.SetGroupVersionKind(gvk)
+
 	err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found)
 	if errors.Is(err, &meta.NoKindMatchError{}) {
 		// convert the error to NotFound to handle both the same way in the caller
-		return nil, k8serr.NewNotFound(schema.GroupResource{Group: obj.GroupVersionKind().Group}, obj.GetName())
+		return nil, k8serr.NewNotFound(schema.GroupResource{Group: gvk.Group}, obj.GetName())
 	}
 	if err != nil {
 		return nil, err
@@ -308,7 +273,11 @@ func deleteResource(ctx context.Context, cli client.Client, found *unstructured.
 	return nil
 }
 
-func createResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object) error {
+func createResource(ctx context.Context, cli client.Client, res *resource.Resource, owner metav1.Object) error {
+	obj, err := conversion.ResourceToUnstructured(res)
+	if err != nil {
+		return err
+	}
 	if obj.GetKind() != "CustomResourceDefinition" && obj.GetKind() != "OdhDashboardConfig" {
 		if err := ctrl.SetControllerReference(owner, metav1.Object(obj), cli.Scheme()); err != nil {
 			return err
@@ -317,13 +286,40 @@ func createResource(ctx context.Context, cli client.Client, obj *unstructured.Un
 	return cli.Create(ctx, obj)
 }
 
-// TODO: cleanup some logic once RHOAIENG-7929 is finished.
-func skipUpdateOnWhitelistedFields(obj *unstructured.Unstructured, componentName string) error {
-	if componentName == "kserve" || componentName == "model-mesh" {
-		if err := removeResourcesFromDeployment(obj); err != nil {
+func updateResource(ctx context.Context, cli client.Client, res *resource.Resource, found *unstructured.Unstructured, owner metav1.Object) error {
+	// Skip ODHDashboardConfig Update
+	if found.GetKind() == "OdhDashboardConfig" {
+		return nil
+	}
+
+	// only reconcile whiltelistedFields if the existing resource has annoation set to "true"
+	// all other cases, whiltelistedfields will be skipped by ODH operator
+	if managed, exists := found.GetAnnotations()[annotations.ManagedByODHOperator]; !exists || managed != "true" {
+		if err := skipUpdateOnWhitelistedFields(res); err != nil {
 			return err
 		}
 	}
+
+	obj, err := conversion.ResourceToUnstructured(res)
+	if err != nil {
+		return err
+	}
+
+	// Retain existing labels on update
+	updateLabels(found, obj)
+
+	return performPatch(ctx, cli, obj, found, owner)
+}
+
+// skipUpdateOnWhitelistedFields applies RemoverPlugin to the component's resources
+// This ensures that we do not overwrite the fields when Patch is applied later to the resource.
+func skipUpdateOnWhitelistedFields(res *resource.Resource) error {
+	for _, rmPlugin := range plugins.WhitelistedFields {
+		if err := rmPlugin.TransformResource(res); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -347,20 +343,4 @@ func performPatch(ctx context.Context, cli client.Client, obj, found *unstructur
 	return cli.Patch(ctx, found, client.RawPatch(types.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner(owner.GetName()))
 }
 
-func updateResource(ctx context.Context, cli client.Client, obj, found *unstructured.Unstructured, owner metav1.Object, componentName string) error {
-	// Skip ODHDashboardConfig Update
-	if found.GetKind() == "OdhDashboardConfig" {
-		return nil
-	}
-	// skip updating whitelisted fields from component
-	if err := skipUpdateOnWhitelistedFields(obj, componentName); err != nil {
-		return err
-	}
-
-	// Retain existing labels on update
-	updateLabels(found, obj)
-
-	return performPatch(ctx, cli, obj, found, owner)
-}
-
 // TODO : Add function to cleanup code created as part of pre install and post install task of a component