Add migration support to the SSA client (#480)

Author: metlos

pkg/client/ssa_client.go

@@ -4,9 +4,14 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/util/csaupgrade"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -24,21 +29,59 @@ type SSAApplyClient struct {
 
 	// The field owner to use for SSA-applied objects.
 	FieldOwner string
+
+	// MigrateSSAByDefault specifies the default SSA migration behavior.
+	//
+	// When checking for the migration, there is an additional GET of the resource, followed by optional
+	// UPDATE (if the migration is needed) before the actual changes to the objects are applied.
+	//
+	// This field specifies the default behavior that can be overridden by supplying an explicit MigrateSSA() option
+	// to ApplyObject or Apply methods.
+	//
+	// The main advantage of using the SSA in our code is that ability of SSA to handle automatic deletion of fields
+	// that we no longer set in our templates. But this only works when the fields are owned by managers and applied
+	// using "Apply" operation. As long as there is an "Update" entry with given field (even if the owner is the same)
+	// the field WILL NOT be automatically deleted by Kubernetes.
+	//
+	// Therefore, we need to make sure that our manager uses ONLY the Apply operations. This maximizes the chance
+	// that the object will look the way we need.
+	MigrateSSAByDefault bool
+
+	// NonSSAFieldOwner should be set to the same value as the user agent used by the provided Kubernetes client
+	// or to the value of the explicit field owner that the calling code used to use with the normal CRUD operations
+	// (highly unlikely and not the case in our codebase).
+	//
+	// The user agent can be obtained from the REST config from which the client is constructed.
+	//
+	// The user agent in the REST config is usually empty, so there's no need to set it here either in that case.
+	NonSSAFieldOwner string
 }
 
 // NewSSAApplyClient creates a new SSAApplyClient from the provided parameters that will use the provided field owner
 // for the patches.
+//
+// The returned client checks for the SSA migration by default.
 func NewSSAApplyClient(cl client.Client, fieldOwner string) *SSAApplyClient {
 	return &SSAApplyClient{
-		Client:     cl,
-		FieldOwner: fieldOwner,
+		Client:              cl,
+		FieldOwner:          fieldOwner,
+		MigrateSSAByDefault: true,
 	}
 }
 
+type migrateSSA int
+
+const (
+	migrateSSANotSpecified migrateSSA = iota
+	migrateSSAYes
+	migrateSSANo
+)
+
 type ssaApplyObjectConfiguration struct {
-	owner     metav1.Object
-	newLabels map[string]string
-	skipIf    func(client.Object) bool
+	owner      metav1.Object
+	newLabels  map[string]string
+	skipIf     func(client.Object) bool
+	migrateSSA migrateSSA
 }
 
 func newSSAApplyObjectConfiguration(options ...SSAApplyObjectOption) ssaApplyObjectConfiguration {
@@ -76,6 +119,19 @@ func EnsureLabels(labels map[string]string) SSAApplyObjectOption {
 	}
 }
 
+// MigrateSSA instructs the apply to do the SSA managed fields migration or not.
+// If not used at all, the MigrateSSAByDefault field of the SSA client determines
+// whether the fields will be migrated or not.
+func MigrateSSA(value bool) SSAApplyObjectOption {
+	return func(config *ssaApplyObjectConfiguration) {
+		if value {
+			config.migrateSSA = migrateSSAYes
+		} else {
+			config.migrateSSA = migrateSSANo
+		}
+	}
+}
+
 // Configure sets the owner reference and merges the labels. Other options modify the logic
 // of apply function and therefore need to be checked manually.
 func (c *ssaApplyObjectConfiguration) Configure(obj client.Object, s *runtime.Scheme) error {
@@ -100,6 +156,12 @@ func (c *SSAApplyClient) ApplyObject(ctx context.Context, obj client.Object, opt
 		return composeError(obj, fmt.Errorf("failed to prepare the object for SSA: %w", err))
 	}
 
+	if config.migrateSSA == migrateSSAYes || (config.migrateSSA == migrateSSANotSpecified && c.MigrateSSAByDefault) {
+		if err := c.migrateSSA(ctx, obj); err != nil {
+			return composeError(obj, err)
+		}
+	}
+
 	if config.skipIf != nil && config.skipIf(obj) {
 		return nil
 	}
@@ -111,6 +173,32 @@ func (c *SSAApplyClient) ApplyObject(ctx context.Context, obj client.Object, opt
 	return nil
 }
 
+func (c *SSAApplyClient) migrateSSA(ctx context.Context, obj client.Object) error {
+	orig := obj.DeepCopyObject().(client.Object)
+	if err := c.Client.Get(ctx, client.ObjectKeyFromObject(obj), orig); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to get the object from the cluster while migrating managed fields: %w", err)
+		}
+		orig = nil
+	}
+
+	if orig != nil {
+		oldFieldOwner := c.NonSSAFieldOwner
+		if len(oldFieldOwner) == 0 {
+			// this is how the kubernetes api server determines the default owner from the user agent
+			// The default user agent has the form of "name-of-binary/version information etc.".
+			// The owner is the first part of the UA unless explicitly specified in the request URI.
+			oldFieldOwner = strings.Split(rest.DefaultKubernetesUserAgent(), "/")[0]
+		}
+		if isSsaMigrationNeeded(orig, oldFieldOwner) {
+			if err := migrateToSSA(ctx, c.Client, orig, oldFieldOwner, c.FieldOwner); err != nil {
+				return fmt.Errorf("failed to migrate the managed fields: %w", err)
+			}
+		}
+	}
+	return nil
+}
+
 func composeError(obj client.Object, err error) error {
 	message := "unable to patch '%s' called '%s' in namespace '%s': %w"
 	if !obj.GetObjectKind().GroupVersionKind().Empty() {
@@ -157,3 +245,19 @@ func (c *SSAApplyClient) Apply(ctx context.Context, toolchainObjects []client.Ob
 	}
 	return nil
 }
+
+func isSsaMigrationNeeded(obj client.Object, expectedOwner string) bool {
+	for _, mf := range obj.GetManagedFields() {
+		if mf.Manager == expectedOwner && mf.Operation != metav1.ManagedFieldsOperationApply {
+			return true
+		}
+	}
+	return false
+}
+
+func migrateToSSA(ctx context.Context, cl client.Client, obj client.Object, oldFieldOwner, newFieldOwner string) error {
+	if err := csaupgrade.UpgradeManagedFields(obj, sets.New(oldFieldOwner), newFieldOwner); err != nil {
+		return err
+	}
+	return cl.Update(ctx, obj)
+}

pkg/client/ssa_client_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	stderrors "errors"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/codeready-toolchain/toolchain-common/pkg/client"
@@ -16,6 +17,8 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/rest"
+	"k8s.io/utils/ptr"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
@@ -154,6 +157,91 @@ func TestSsaClient(t *testing.T) {
 			inCluster := &corev1.ConfigMap{}
 			require.True(t, errors.IsNotFound(cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(obj), inCluster)))
 		})
+		t.Run("MigrateSSA", func(t *testing.T) {
+			for _, setup := range []struct {
+				defaultMigrate    bool
+				explicitMigrate   *bool
+				migrationExpected bool
+			}{
+				{
+					defaultMigrate:    false,
+					explicitMigrate:   ptr.To(true),
+					migrationExpected: true,
+				},
+				{
+					defaultMigrate:    false,
+					explicitMigrate:   ptr.To(false),
+					migrationExpected: false,
+				},
+				{
+					defaultMigrate:    false,
+					explicitMigrate:   nil,
+					migrationExpected: false,
+				},
+				{
+					defaultMigrate:    true,
+					explicitMigrate:   ptr.To(true),
+					migrationExpected: true,
+				},
+				{
+					defaultMigrate:    true,
+					explicitMigrate:   ptr.To(false),
+					migrationExpected: false,
+				},
+				{
+					defaultMigrate:    true,
+					explicitMigrate:   nil,
+					migrationExpected: true,
+				},
+			} {
+				testName := fmt.Sprintf("default: %v, explicit: %v", setup.defaultMigrate, setup.explicitMigrate)
+				t.Run(testName, func(t *testing.T) {
+					// given
+					obj := &corev1.Service{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "obj",
+							Namespace: "default",
+							ManagedFields: []metav1.ManagedFieldsEntry{
+								{
+									FieldsType: "FieldsV1",
+									FieldsV1:   &metav1.FieldsV1{Raw: []byte(`{"f:spec": {"f:selector": {}}}`)},
+									Manager:    strings.Split(rest.DefaultKubernetesUserAgent(), "/")[0],
+									Operation:  metav1.ManagedFieldsOperationUpdate,
+								},
+							},
+						},
+						Spec: corev1.ServiceSpec{},
+					}
+					toApply := obj.DeepCopy()
+					toApply.SetManagedFields(nil)
+
+					cl, acl := NewTestSsaApplyClient(t, obj)
+					acl.MigrateSSAByDefault = setup.defaultMigrate
+
+					// when
+					var opts []client.SSAApplyObjectOption
+					if setup.explicitMigrate != nil {
+						opts = append(opts, client.MigrateSSA(*setup.explicitMigrate))
+					}
+					inCluster := &corev1.Service{}
+					require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(obj), inCluster))
+					require.NoError(t, acl.ApplyObject(context.TODO(), toApply, opts...))
+
+					// then
+					inCluster = &corev1.Service{}
+					require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(obj), inCluster))
+					if setup.migrationExpected {
+						assert.Len(t, inCluster.ManagedFields, 1)
+						assert.Equal(t, "test-field-owner", inCluster.ManagedFields[0].Manager)
+						assert.Equal(t, metav1.ManagedFieldsOperationApply, inCluster.ManagedFields[0].Operation)
+					} else {
+						assert.Len(t, inCluster.ManagedFields, 1)
+						assert.NotEqual(t, "test-field-owner", inCluster.ManagedFields[0].Manager)
+						assert.Equal(t, metav1.ManagedFieldsOperationUpdate, inCluster.ManagedFields[0].Operation)
+					}
+				})
+			}
+		})
 		t.Run("propagates k8s errors", func(t *testing.T) {
 			// given
 			cl, acl := NewTestSsaApplyClient(t)