Merge pull request #182 from lsviben/bck-conversion-bug

[Backport release-0.11] Handle empty converts
crossplane-contrib · Jan 22, 2024 · b87ee5b · b87ee5b
2 parents cc10a8c + dd36393
commit b87ee5b
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 12 deletions.
diff --git a/apis/object/v1alpha1/conversion.go b/apis/object/v1alpha1/conversion.go
@@ -17,12 +17,11 @@ limitations under the License.
 package v1alpha1
 
 import (
-	"errors"
-
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+	"github.com/crossplane/crossplane-runtime/pkg/errors"
 
 	"github.com/crossplane-contrib/provider-kubernetes/apis/object/v1alpha2"
 )
@@ -101,7 +100,7 @@ func (src *Object) ConvertTo(dstRaw conversion.Hub) error { // nolint:golint //
 	case Observe:
 		dst.Spec.ManagementPolicies = xpv1.ManagementPolicies{xpv1.ManagementActionObserve}
 	default:
-		return errors.New("unknown management policy")
+		return errors.Errorf("unknown management policy: %v", src.Spec.ManagementPolicy)
 	}
 
 	return nil
@@ -169,6 +168,14 @@ func (dst *Object) ConvertFrom(srcRaw conversion.Hub) error { // nolint:golint,
 		},
 	}
 
+	// Policies are unset and the object is not yet created.
+	// As the managementPolicies would default to ["*"], we can set
+	// the management policy to Default.
+	if src.GetManagementPolicies() == nil && src.CreationTimestamp.IsZero() {
+		dst.Spec.ManagementPolicy = Default
+		return nil
+	}
+
 	// handle management policies migration
 	policySet := sets.New[xpv1.ManagementAction](src.GetManagementPolicies()...)
 
@@ -187,8 +194,9 @@ func (dst *Object) ConvertFrom(srcRaw conversion.Hub) error { // nolint:golint,
 		!policySet.HasAny(xpv1.ManagementActionCreate, xpv1.ManagementActionUpdate, xpv1.ManagementActionDelete):
 		dst.Spec.ManagementPolicy = Observe
 	default:
-		// TODO(turkenh): Should we default to something here instead of erroring out?
-		return errors.New("unsupported management policy")
+		// NOTE(lsviben): Other combinations of v1alpha2 management policies
+		// were not supported in v1alpha1. Leaving it empty to avoid
+		// errors during conversion instead of failing.
 	}
 
 	return nil

diff --git a/apis/object/v1alpha1/conversion_test.go b/apis/object/v1alpha1/conversion_test.go
@@ -257,7 +257,7 @@ func TestConvertTo(t *testing.T) {
 				},
 			},
 			want: want{
-				err: errors.New("unknown management policy"),
+				err: errors.New("unknown management policy: unknown"),
 			},
 		},
 	}
@@ -293,7 +293,7 @@ func TestConvertFrom(t *testing.T) {
 		want want
 	}{
 		{
-			name: "converts to v1alpha2",
+			name: "converts to v1alpha1",
 			args: args{
 				src: &v1alpha2.Object{
 					ObjectMeta: metav1.ObjectMeta{
@@ -386,7 +386,7 @@ func TestConvertFrom(t *testing.T) {
 			},
 		},
 		{
-			name: "converts to v1alpha2 - nil checks",
+			name: "converts to v1alpha1 - nil checks",
 			args: args{
 				src: &v1alpha2.Object{
 					ObjectMeta: metav1.ObjectMeta{
@@ -453,21 +453,76 @@ func TestConvertFrom(t *testing.T) {
 			},
 		},
 		{
-			name: "errors if management policy is unknown",
+			name: "converts to v1alpha1 - empty policy",
+			args: args{
+				src: &v1alpha2.Object{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "coolobject",
+					},
+					Spec: v1alpha2.ObjectSpec{
+						ResourceSpec: v1.ResourceSpec{
+							DeletionPolicy: v1.DeletionDelete,
+						},
+						ForProvider: v1alpha2.ObjectParameters{
+							Manifest: runtime.RawExtension{Raw: []byte("apiVersion: v1\nkind: Secret\nmetadata:\n  name: topsecret\n")},
+						},
+					},
+				},
+			},
+			want: want{
+				dst: &v1alpha1.Object{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "coolobject",
+					},
+					Spec: v1alpha1.ObjectSpec{
+						ResourceSpec: v1alpha1.ResourceSpec{
+							DeletionPolicy: v1.DeletionDelete,
+						},
+						ForProvider: v1alpha1.ObjectParameters{
+							Manifest: runtime.RawExtension{Raw: []byte("apiVersion: v1\nkind: Secret\nmetadata:\n  name: topsecret\n")},
+						},
+						ManagementPolicy:  v1alpha1.Default,
+						ConnectionDetails: []v1alpha1.ConnectionDetail{},
+						References:        []v1alpha1.Reference{},
+					},
+				},
+			},
+		},
+		{
+			name: "converts to v1alpha1 - unsupported policy",
 			args: args{
 				src: &v1alpha2.Object{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "coolobject",
 					},
 					Spec: v1alpha2.ObjectSpec{
 						ResourceSpec: v1.ResourceSpec{
-							ManagementPolicies: []v1.ManagementAction{},
+							DeletionPolicy:     v1.DeletionDelete,
+							ManagementPolicies: []v1.ManagementAction{v1.ManagementActionDelete},
+						},
+						ForProvider: v1alpha2.ObjectParameters{
+							Manifest: runtime.RawExtension{Raw: []byte("apiVersion: v1\nkind: Secret\nmetadata:\n  name: topsecret\n")},
 						},
 					},
 				},
 			},
 			want: want{
-				err: errors.New("unsupported management policy"),
+				dst: &v1alpha1.Object{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "coolobject",
+					},
+					Spec: v1alpha1.ObjectSpec{
+						ResourceSpec: v1alpha1.ResourceSpec{
+							DeletionPolicy: v1.DeletionDelete,
+						},
+						ForProvider: v1alpha1.ObjectParameters{
+							Manifest: runtime.RawExtension{Raw: []byte("apiVersion: v1\nkind: Secret\nmetadata:\n  name: topsecret\n")},
+						},
+						ManagementPolicy:  "",
+						ConnectionDetails: []v1alpha1.ConnectionDetail{},
+						References:        []v1alpha1.Reference{},
+					},
+				},
 			},
 		},
 	}

diff --git a/examples/in-composition/composition.yaml b/examples/in-composition/composition.yaml
@@ -206,7 +206,7 @@ spec:
       readinessChecks:
         - type: None
     - base:
-        apiVersion: kubernetes.crossplane.io/v1beta1
+        apiVersion: kubernetes.crossplane.io/v1alpha2
         kind: Object
         spec:
           forProvider: