Merge pull request #119 from phisco/readiness_fromobject

feat: deriving readiness from object

Author: turkenh

.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

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

apis/object/v1alpha1/zz_generated.deepcopy.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() {

cmd/provider/main.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 {

internal/controller/object/object.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)
+			}
+		})
+	}
+}