From dd36393c266f452fc9e5525902bed20e3675fea0 Mon Sep 17 00:00:00 2001 From: Lovro Sviben <46844730+lsviben@users.noreply.github.com> Date: Mon, 22 Jan 2024 10:34:40 +0200 Subject: [PATCH] Merge pull request #181 from lsviben/conversion-bug Handle empty converts Signed-off-by: lsviben --- apis/object/v1alpha1/conversion.go | 18 +++++-- apis/object/v1alpha1/conversion_test.go | 67 +++++++++++++++++++++--- examples/in-composition/composition.yaml | 2 +- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/apis/object/v1alpha1/conversion.go b/apis/object/v1alpha1/conversion.go index 3c374ff1..0e423fb1 100644 --- 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 index bc68c3a3..41dca560 100644 --- 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,7 +453,43 @@ 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{ @@ -461,13 +497,32 @@ func TestConvertFrom(t *testing.T) { }, 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 index 68cde505..4c3faffe 100644 --- 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: