From 8ca9c1eed32d533086a507ca672a61ecca0ceb48 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Wed, 24 May 2023 13:26:45 +0200 Subject: [PATCH 1/3] ci: configure gci linter to enforce imports ordering Signed-off-by: Philippe Scorsolini --- .golangci.yml | 15 ++++++++++----- cmd/provider/main.go | 8 ++++---- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 7a482413..91936415 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -35,10 +35,15 @@ linters-settings: # simplify code: gofmt with `-s` option, true by default simplify: true - goimports: - # put imports beginning with prefix after 3rd-party packages; - # it's a comma-separated list of prefixes - local-prefixes: github.com/crossplane-contrib/provider-kubernetes + gci: + custom-order: true + sections: + - standard + - default + - prefix(github.com/crossplane) + - prefix(github.com/crossplane-contrib/provider-kubernetes) + - blank + - dot gocyclo: # minimal code complexity to report, 30 by default (but we recommend 10-20) @@ -110,7 +115,7 @@ linters: - gocritic - interfacer - goconst - - goimports + - gci - gofmt # We enable this as well as goimports for its simplify mode. - prealloc - golint diff --git a/cmd/provider/main.go b/cmd/provider/main.go index e2595370..9a4d1d2c 100644 --- a/cmd/provider/main.go +++ b/cmd/provider/main.go @@ -21,21 +21,21 @@ import ( "path/filepath" "time" - "github.com/crossplane/crossplane-runtime/pkg/controller" - "github.com/crossplane/crossplane-runtime/pkg/feature" "go.uber.org/zap/zapcore" - "gopkg.in/alecthomas/kingpin.v2" - _ "k8s.io/client-go/plugin/pkg/client/auth" "k8s.io/client-go/tools/leaderelection/resourcelock" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "github.com/crossplane/crossplane-runtime/pkg/controller" + "github.com/crossplane/crossplane-runtime/pkg/feature" "github.com/crossplane/crossplane-runtime/pkg/logging" "github.com/crossplane/crossplane-runtime/pkg/ratelimiter" "github.com/crossplane-contrib/provider-kubernetes/apis" object "github.com/crossplane-contrib/provider-kubernetes/internal/controller" + + _ "k8s.io/client-go/plugin/pkg/client/auth" ) func main() { From c59dc2a73ad941424781cab2afa2e3633bc6ff15 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Wed, 24 May 2023 13:28:47 +0200 Subject: [PATCH 2/3] feat: add readiness policies Signed-off-by: Philippe Scorsolini --- apis/object/v1alpha1/types.go | 24 +++++++++++ apis/object/v1alpha1/zz_generated.deepcopy.go | 16 +++++++ internal/controller/object/object.go | 42 ++++++++++++++++--- .../kubernetes.crossplane.io_objects.yaml | 14 +++++++ 4 files changed, 91 insertions(+), 5 deletions(-) diff --git a/apis/object/v1alpha1/types.go b/apis/object/v1alpha1/types.go index 6b9c0078..86b4cdfa 100644 --- a/apis/object/v1alpha1/types.go +++ b/apis/object/v1alpha1/types.go @@ -123,6 +123,30 @@ type ObjectSpec struct { // +kubebuilder:default=Default ManagementPolicy `json:"managementPolicy,omitempty"` References []Reference `json:"references,omitempty"` + Readiness Readiness `json:"readiness,omitempty"` +} + +// ReadinessPolicy defines how the Object's readiness condition should be computed. +type ReadinessPolicy string + +const ( + // ReadinessPolicySuccessfulCreate means the object is marked as ready when the + // underlying external resource is successfully created. + ReadinessPolicySuccessfulCreate ReadinessPolicy = "SuccessfulCreate" + // ReadinessPolicyDeriveFromObject means the object is marked as ready if and only if the underlying + // external resource is considered ready. + ReadinessPolicyDeriveFromObject ReadinessPolicy = "DeriveFromObject" +) + +// Readiness defines how the object's readiness condition should be computed, +// if not specified it will be considered ready as soon as the underlying external +// resource is considered up-to-date. +type Readiness struct { + // Policy defines how the Object's readiness condition should be computed. + // +optional + // +kubebuilder:validation:Enum=SuccessfulCreate;DeriveFromObject + // +kubebuilder:default=SuccessfulCreate + Policy ReadinessPolicy `json:"policy,omitempty"` } // ConnectionDetail represents an entry in the connection secret for an Object diff --git a/apis/object/v1alpha1/zz_generated.deepcopy.go b/apis/object/v1alpha1/zz_generated.deepcopy.go index b36d8373..73dd255f 100644 --- a/apis/object/v1alpha1/zz_generated.deepcopy.go +++ b/apis/object/v1alpha1/zz_generated.deepcopy.go @@ -165,6 +165,7 @@ func (in *ObjectSpec) DeepCopyInto(out *ObjectSpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + out.Readiness = in.Readiness } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObjectSpec. @@ -215,6 +216,21 @@ func (in *PatchesFrom) DeepCopy() *PatchesFrom { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Readiness) DeepCopyInto(out *Readiness) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Readiness. +func (in *Readiness) DeepCopy() *Readiness { + if in == nil { + return nil + } + out := new(Readiness) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Reference) DeepCopyInto(out *Reference) { *out = *in diff --git a/internal/controller/object/object.go b/internal/controller/object/object.go index 6434d16e..8c09d356 100644 --- a/internal/controller/object/object.go +++ b/internal/controller/object/object.go @@ -233,7 +233,7 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex return managed.ExternalObservation{}, errors.Wrap(err, errGetObject) } - if err = setObserved(cr, observed); err != nil { + if err = c.setObserved(cr, observed); err != nil { return managed.ExternalObservation{}, err } @@ -270,7 +270,7 @@ func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext return managed.ExternalCreation{}, errors.Wrap(err, errCreateObject) } - return managed.ExternalCreation{}, setObserved(cr, obj) + return managed.ExternalCreation{}, c.setObserved(cr, obj) } func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.ExternalUpdate, error) { @@ -299,7 +299,7 @@ func (c *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext return managed.ExternalUpdate{}, errors.Wrap(err, errApplyObject) } - return managed.ExternalUpdate{}, setObserved(cr, obj) + return managed.ExternalUpdate{}, c.setObserved(cr, obj) } func (c *external) Delete(ctx context.Context, mg resource.Managed) error { @@ -353,11 +353,41 @@ func getLastApplied(obj *v1alpha1.Object, observed *unstructured.Unstructured) ( return last, nil } -func setObserved(obj *v1alpha1.Object, observed *unstructured.Unstructured) error { +func (c *external) setObserved(obj *v1alpha1.Object, observed *unstructured.Unstructured) error { var err error if obj.Status.AtProvider.Manifest.Raw, err = observed.MarshalJSON(); err != nil { return errors.Wrap(err, errFailedToMarshalExisting) } + + if err := c.updateConditionFromObserved(obj, observed); err != nil { + return err + } + return nil +} + +func (c *external) updateConditionFromObserved(obj *v1alpha1.Object, observed *unstructured.Unstructured) error { + switch obj.Spec.Readiness.Policy { + case v1alpha1.ReadinessPolicyDeriveFromObject: + conditioned := xpv1.ConditionedStatus{} + err := fieldpath.Pave(observed.Object).GetValueInto("status", &conditioned) + if err != nil { + c.logger.Debug("Got error while getting conditions from observed object, setting it as Unavailable", "error", err, "observed", observed) + obj.SetConditions(xpv1.Unavailable()) + return nil + } + if status := conditioned.GetCondition(xpv1.TypeReady).Status; status != v1.ConditionTrue { + c.logger.Debug("Observed object is not ready, setting it as Unavailable", "status", status, "observed", observed) + obj.SetConditions(xpv1.Unavailable()) + return nil + } + obj.SetConditions(xpv1.Available()) + case v1alpha1.ReadinessPolicySuccessfulCreate, "": + // do nothing, will be handled by c.handleLastApplied method + // "" should never happen, but just in case we will treat it as SuccessfulCreate for backward compatibility + default: + // should never happen + return errors.Errorf("unknown readiness policy %q", obj.Spec.Readiness.Policy) + } return nil } @@ -453,7 +483,9 @@ func (c *external) handleLastApplied(ctx context.Context, obj *v1alpha1.Object, if isUpToDate { c.logger.Debug("Up to date!") - obj.Status.SetConditions(xpv1.Available()) + if p := obj.Spec.Readiness.Policy; p == v1alpha1.ReadinessPolicySuccessfulCreate || p == "" { + obj.Status.SetConditions(xpv1.Available()) + } cd, err := connectionDetails(ctx, c.client, obj.Spec.ConnectionDetails) if err != nil { diff --git a/package/crds/kubernetes.crossplane.io_objects.yaml b/package/crds/kubernetes.crossplane.io_objects.yaml index 8ba87c0e..755adcf2 100644 --- a/package/crds/kubernetes.crossplane.io_objects.yaml +++ b/package/crds/kubernetes.crossplane.io_objects.yaml @@ -293,6 +293,20 @@ spec: required: - name type: object + readiness: + description: Readiness defines how the object's readiness condition + should be computed, if not specified it will be considered ready + as soon as the underlying external resource is considered up-to-date. + properties: + policy: + default: SuccessfulCreate + description: Policy defines how the Object's readiness condition + should be computed. + enum: + - SuccessfulCreate + - DeriveFromObject + type: string + type: object references: items: description: Reference refers to an Object or arbitrary Kubernetes From e30839e83ac725c7492872528940ce3c820acdae Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Wed, 24 May 2023 14:24:36 +0200 Subject: [PATCH 3/3] tests: cover a updateConditionFromObserved Signed-off-by: Philippe Scorsolini --- internal/controller/object/object_test.go | 184 ++++++++++++++++++++++ 1 file changed, 184 insertions(+) diff --git a/internal/controller/object/object_test.go b/internal/controller/object/object_test.go index a61f7b29..f474d6b1 100644 --- a/internal/controller/object/object_test.go +++ b/internal/controller/object/object_test.go @@ -1643,3 +1643,187 @@ func Test_connectionDetails(t *testing.T) { }) } } + +func Test_updateConditionFromObserved(t *testing.T) { + type args struct { + obj *v1alpha1.Object + observed *unstructured.Unstructured + } + type want struct { + err error + conditions []xpv1.Condition + } + cases := map[string]struct { + args + want + }{ + "NoopIfNoPolicyDefined": { + args: args{ + obj: &v1alpha1.Object{}, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": xpv1.ConditionedStatus{}, + }, + }, + }, + want: want{ + err: nil, + conditions: nil, + }, + }, + "NoopIfSuccessfulCreatePolicyDefined": { + args: args{ + obj: &v1alpha1.Object{ + Spec: v1alpha1.ObjectSpec{ + Readiness: v1alpha1.Readiness{ + Policy: v1alpha1.ReadinessPolicySuccessfulCreate, + }, + }, + }, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": xpv1.ConditionedStatus{}, + }, + }, + }, + want: want{ + err: nil, + conditions: nil, + }, + }, + "UnavailableIfDeriveFromObjectAndNotReady": { + args: args{ + obj: &v1alpha1.Object{ + Spec: v1alpha1.ObjectSpec{ + Readiness: v1alpha1.Readiness{ + Policy: v1alpha1.ReadinessPolicyDeriveFromObject, + }, + }, + }, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": xpv1.ConditionedStatus{ + Conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + }, + }, + want: want{ + err: nil, + conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionFalse, + Reason: xpv1.ReasonUnavailable, + }, + }, + }, + }, + "UnavailableIfDerivedFromObjectAndNoCondition": { + args: args{ + obj: &v1alpha1.Object{ + Spec: v1alpha1.ObjectSpec{ + Readiness: v1alpha1.Readiness{ + Policy: v1alpha1.ReadinessPolicyDeriveFromObject, + }, + }, + }, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": xpv1.ConditionedStatus{}, + }, + }, + }, + want: want{ + err: nil, + conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionFalse, + Reason: xpv1.ReasonUnavailable, + }, + }, + }, + }, + "AvailableIfDeriveFromObjectAndReady": { + args: args{ + obj: &v1alpha1.Object{ + Spec: v1alpha1.ObjectSpec{ + Readiness: v1alpha1.Readiness{ + Policy: v1alpha1.ReadinessPolicyDeriveFromObject, + }, + }, + }, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": xpv1.ConditionedStatus{ + Conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + }, + }, + want: want{ + err: nil, + conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionTrue, + Reason: xpv1.ReasonAvailable, + }, + }, + }, + }, + "UnavailableIfDerivedFromObjectAndCantParse": { + args: args{ + obj: &v1alpha1.Object{ + Spec: v1alpha1.ObjectSpec{ + Readiness: v1alpha1.Readiness{ + Policy: v1alpha1.ReadinessPolicyDeriveFromObject, + }, + }, + }, + observed: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": "not a conditioned status", + }, + }, + }, + want: want{ + err: nil, + conditions: []xpv1.Condition{ + { + Type: xpv1.TypeReady, + Status: corev1.ConditionFalse, + Reason: xpv1.ReasonUnavailable, + }, + }, + }, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + e := &external{ + logger: logging.NewNopLogger(), + } + gotErr := e.updateConditionFromObserved(tc.args.obj, tc.args.observed) + if diff := cmp.Diff(tc.want.err, gotErr, test.EquateErrors()); diff != "" { + t.Fatalf("updateConditionFromObserved(...): -want error, +got error: %s", diff) + } + if diff := cmp.Diff(tc.want.conditions, tc.args.obj.Status.Conditions, cmpopts.SortSlices(func(a, b xpv1.Condition) bool { + return a.Type < b.Type + }), cmpopts.IgnoreFields(xpv1.Condition{}, "LastTransitionTime")); diff != "" { + t.Errorf("updateConditionFromObserved(...): -want result, +got result: %s", diff) + } + }) + } +}