diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index 9c8dd8ca68e..2cd93d5cecf 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -17,8 +17,6 @@ package v1alpha1 import ( - "encoding/json" - "github.com/knative/pkg/apis" "github.com/knative/pkg/apis/duck" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" @@ -55,6 +53,10 @@ var _ duckv1alpha1.ConditionsAccessor = (*SubscriptionStatus)(nil) // Check that Subscription implements the Conditions duck type. var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Conditions{}) +// Check that the Subscription implements the Generation duck type. +var emptyGen duckv1alpha1.Generation +var _ = duck.VerifyType(&Subscription{}, &emptyGen) + // And it's Subscribable var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Subscribable{}) @@ -157,6 +159,8 @@ type Callable struct { TargetURI *string `json:"targetURI,omitempty"` } +// ResultStrategy specifies the handling of the Callable's returned result. If +// no Callable is specified, the identity function is assumed. type ResultStrategy struct { // This object must fulfill the Sinkable contract. // @@ -170,7 +174,9 @@ type ResultStrategy struct { Target *corev1.ObjectReference `json:"target,omitempty"` } -var subCondSet = duckv1alpha1.NewLivingConditionSet() +// subCondSet is a condition set with Ready as the happy condition and +// ReferencesResolved and FromReady as the dependent conditions. +var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionFromReady) // SubscriptionStatus (computed) for a subscription type SubscriptionStatus struct { @@ -188,7 +194,7 @@ const ( // SubscriptionConditionReady has status True when all subconditions below have been set to True. SubscriptionConditionReady = duckv1alpha1.ConditionReady - // SubscriptionReferencesResolved has status True when all the specified references have been successfully + // SubscriptionConditionReferencesResolved has status True when all the specified references have been successfully // resolved. SubscriptionConditionReferencesResolved duckv1alpha1.ConditionType = "Resolved" @@ -197,11 +203,6 @@ const ( SubscriptionConditionFromReady duckv1alpha1.ConditionType = "FromReady" ) -// GetSpecJSON returns spec as json -func (s *Subscription) GetSpecJSON() ([]byte, error) { - return json.Marshal(s.Spec) -} - // GetCondition returns the condition currently associated with the given type, or nil. func (ss *SubscriptionStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alpha1.Condition { return subCondSet.Manage(ss).GetCondition(t) @@ -213,6 +214,26 @@ func (ss *SubscriptionStatus) GetConditions() duckv1alpha1.Conditions { return ss.Conditions } +// IsReady returns true if the resource is ready overall. +func (ss *SubscriptionStatus) IsReady() bool { + return subCondSet.Manage(ss).IsHappy() +} + +// InitializeConditions sets relevant unset conditions to Unknown state. +func (ss *SubscriptionStatus) InitializeConditions() { + subCondSet.Manage(ss).InitializeConditions() +} + +// MarkReferencesResolved sets the ReferencesResolved condition to True state. +func (ss *SubscriptionStatus) MarkReferencesResolved() { + subCondSet.Manage(ss).MarkTrue(SubscriptionConditionReferencesResolved) +} + +// MarkFromReady sets the FromReady condition to True state. +func (ss *SubscriptionStatus) MarkFromReady() { + subCondSet.Manage(ss).MarkTrue(SubscriptionConditionFromReady) +} + // SetConditions sets the Conditions array. This enables generic handling of // conditions by implementing the duckv1alpha1.Conditions interface. func (ss *SubscriptionStatus) SetConditions(conditions duckv1alpha1.Conditions) { diff --git a/pkg/apis/eventing/v1alpha1/subscription_types_test.go b/pkg/apis/eventing/v1alpha1/subscription_types_test.go index f41df7c91f0..e9c552a452d 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types_test.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" corev1 "k8s.io/api/core/v1" ) @@ -42,12 +43,12 @@ var subscriptionConditionFromReady = duckv1alpha1.Condition{ func TestSubscriptionGetCondition(t *testing.T) { tests := []struct { name string - cs *SubscriptionStatus + ss *SubscriptionStatus condQuery duckv1alpha1.ConditionType want *duckv1alpha1.Condition }{{ name: "single condition", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, }, @@ -56,7 +57,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionReady, }, { name: "multiple conditions", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionReferencesResolved, @@ -66,7 +67,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionReferencesResolved, }, { name: "multiple conditions, condition true", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionFromReady, @@ -76,7 +77,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionFromReady, }, { name: "unknown condition", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionReferencesResolved, @@ -88,7 +89,7 @@ func TestSubscriptionGetCondition(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := test.cs.GetCondition(test.condQuery) + got := test.ss.GetCondition(test.condQuery) if diff := cmp.Diff(test.want, got); diff != "" { t.Errorf("unexpected condition (-want, +got) = %v", diff) } @@ -96,6 +97,123 @@ func TestSubscriptionGetCondition(t *testing.T) { } } +func TestSubscriptionInitializeConditions(t *testing.T) { + tests := []struct { + name string + ss *SubscriptionStatus + want *SubscriptionStatus + }{{ + name: "empty", + ss: &SubscriptionStatus{}, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionUnknown, + }}, + }, + }, { + name: "one false", + ss: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionFalse, + }}, + }, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionFalse, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionUnknown, + }}, + }, + }, { + name: "one true", + ss: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionTrue, + }}, + }, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionTrue, + }}, + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + test.ss.InitializeConditions() + ignore := cmpopts.IgnoreFields(duckv1alpha1.Condition{}, "LastTransitionTime") + if diff := cmp.Diff(test.want, test.ss, ignore); diff != "" { + t.Errorf("unexpected conditions (-want, +got) = %v", diff) + } + }) + } +} + +func TestSubscriptionIsReady(t *testing.T) { + tests := []struct { + name string + markResolved bool + markFromReady bool + wantReady bool + }{{ + name: "all happy", + markResolved: true, + markFromReady: true, + wantReady: true, + }, { + name: "one sad", + markResolved: false, + markFromReady: true, + wantReady: false, + }, { + name: "other sad", + markResolved: true, + markFromReady: false, + wantReady: false, + }, { + name: "both sad", + markResolved: false, + markFromReady: false, + wantReady: false, + }} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ss := &SubscriptionStatus{} + if test.markResolved { + ss.MarkReferencesResolved() + } + if test.markFromReady { + ss.MarkFromReady() + } + got := ss.IsReady() + if test.wantReady != got { + t.Errorf("unexpected readiness: want %v, got %v", test.wantReady, got) + } + }) + } +} + func TestSubscriptionSetConditions(t *testing.T) { c := &Subscription{ Status: SubscriptionStatus{}, @@ -107,32 +225,3 @@ func TestSubscriptionSetConditions(t *testing.T) { t.Errorf("unexpected conditions (-want, +got) = %v", diff) } } - -func TestSubscriptionGetSpecJSON(t *testing.T) { - targetURI := "http://example.com" - c := &Subscription{ - Spec: SubscriptionSpec{ - From: corev1.ObjectReference{ - Name: "foo", - }, - Call: &Callable{ - TargetURI: &targetURI, - }, - Result: &ResultStrategy{ - Target: &corev1.ObjectReference{ - Name: "result", - }, - }, - }, - } - - want := `{"from":{"name":"foo"},"call":{"targetURI":"http://example.com"},"result":{"target":{"name":"result"}}}` - got, err := c.GetSpecJSON() - if err != nil { - t.Fatalf("unexpected spec JSON error: %v", err) - } - - if diff := cmp.Diff(want, string(got)); diff != "" { - t.Errorf("unexpected spec JSON (-want, +got) = %v", diff) - } -} diff --git a/pkg/controller/eventing/subscription/reconcile.go b/pkg/controller/eventing/subscription/reconcile.go index 93f61bed3c2..c1479fa92d0 100644 --- a/pkg/controller/eventing/subscription/reconcile.go +++ b/pkg/controller/eventing/subscription/reconcile.go @@ -78,9 +78,7 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err } func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { - // TODO: Should this just also set up a defer call for subscription.SetConditions. - // No time right now as I'm turning into a pumpking but seems reasonable. - conditions := []duckv1alpha1.Condition{} + subscription.Status.InitializeConditions() // See if the subscription has been deleted accessor, err := meta.Accessor(subscription) @@ -130,10 +128,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { } // Everything that was supposed to be resolved was, so flip the status bit on that. - conditions = append(conditions, duckv1alpha1.Condition{ - Type: v1alpha1.SubscriptionConditionReferencesResolved, - Status: corev1.ConditionTrue, - }) + subscription.Status.MarkReferencesResolved() // Ok, now that we have the From and at least one of the Call/Result, let's reconcile // the From with this information. @@ -143,11 +138,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { return err } // Everything went well, set the fact that subscriptions have been modified - conditions = append(conditions, duckv1alpha1.Condition{ - Type: duckv1alpha1.ConditionReady, - Status: corev1.ConditionTrue, - }) - subscription.Status.SetConditions(conditions) + subscription.Status.MarkFromReady() return nil } diff --git a/pkg/controller/eventing/subscription/reconcile_test.go b/pkg/controller/eventing/subscription/reconcile_test.go index b883bf03d52..eb748e4f382 100644 --- a/pkg/controller/eventing/subscription/reconcile_test.go +++ b/pkg/controller/eventing/subscription/reconcile_test.go @@ -234,9 +234,10 @@ var testCases = []controllertesting.TestCase{ // TODO: Again this works on gke cluster, but I need to set // something else up here. later... // getNewSubscriptionWithReferencesResolvedStatus(), - getNewSubscription(), + getNewSubscriptionWithUnknownConditions(), }, - Scheme: scheme.Scheme, + IgnoreTimes: true, + Scheme: scheme.Scheme, Objects: []runtime.Object{ // Source channel &unstructured.Unstructured{ @@ -412,15 +413,6 @@ func getNewChannel(name string) *eventingv1alpha1.Channel { return channel } -func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription { - s := getNewSubscription() - s.Status.SetConditions([]duckv1alpha1.Condition{{ - Type: eventingv1alpha1.SubscriptionConditionReferencesResolved, - Status: corev1.ConditionTrue, - }}) - return s -} - func getNewSubscription() *eventingv1alpha1.Subscription { subscription := &eventingv1alpha1.Subscription{ TypeMeta: subscriptionType(), @@ -454,6 +446,18 @@ func getNewSubscription() *eventingv1alpha1.Subscription { return subscription } +func getNewSubscriptionWithUnknownConditions() *eventingv1alpha1.Subscription { + s := getNewSubscription() + s.Status.InitializeConditions() + return s +} + +func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription { + s := getNewSubscriptionWithUnknownConditions() + s.Status.MarkReferencesResolved() + return s +} + func channelType() metav1.TypeMeta { return metav1.TypeMeta{ APIVersion: eventingv1alpha1.SchemeGroupVersion.String(), diff --git a/pkg/controller/testing/table.go b/pkg/controller/testing/table.go index 5a85c1a7e39..af7195752d5 100644 --- a/pkg/controller/testing/table.go +++ b/pkg/controller/testing/table.go @@ -24,6 +24,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/apis" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -77,6 +78,9 @@ type TestCase struct { // Fake dynamic objects Objects []runtime.Object + + // IgnoreTimes causes comparisons to ignore fields of type apis.VolatileTime. + IgnoreTimes bool } // Runner returns a testing func that can be passed to t.Run. @@ -109,9 +113,8 @@ func (tc *TestCase) Runner(t *testing.T, r reconcile.Reconciler, c *MockClient) func (tc *TestCase) GetDynamicClient() dynamic.Interface { if tc.Scheme == nil { return dynamicfake.NewSimpleDynamicClient(runtime.NewScheme(), tc.Objects...) - } else { - return dynamicfake.NewSimpleDynamicClient(tc.Scheme, tc.Objects...) } + return dynamicfake.NewSimpleDynamicClient(tc.Scheme, tc.Objects...) } // GetClient returns the mockClient to use for this test case. @@ -203,9 +206,18 @@ func (tc *TestCase) VerifyWantPresent(c client.Client) error { } } - // Ignore TypeMeta, since the objects created by the controller won't have - // it - if diff := cmp.Diff(wp, o, cmpopts.IgnoreTypes(metav1.TypeMeta{})); diff != "" { + diffOpts := cmp.Options{ + // Ignore TypeMeta, since the objects created by the controller won't have + // it + cmpopts.IgnoreTypes(metav1.TypeMeta{}), + } + + if tc.IgnoreTimes { + // Ignore VolatileTime fields, since they rarely compare correctly. + diffOpts = append(diffOpts, cmpopts.IgnoreTypes(apis.VolatileTime{})) + } + + if diff := cmp.Diff(wp, o, diffOpts...); diff != "" { errs.errors = append(errs.errors, fmt.Errorf("Unexpected present %T %s/%s (-want +got):\n%v", wp, acc.GetNamespace(), acc.GetName(), diff)) } }