Faster approvals for the auto-approve controller (#329)

johnbelamaric · web-flow · commit feb6c2f91cc3 · 2023-07-24T19:59:03.000Z
There is now no longer a default delay; it only delays if you
specifically give a delay annotation.

The minimum delay is now 0.

Re-ordered the checks so that the delay will not come into play unless
all other checks are already met.

Added some tests.
diff --git a/controllers/pkg/reconcilers/approval/reconciler.go b/controllers/pkg/reconcilers/approval/reconciler.go
@@ -46,7 +46,6 @@ import (
 
 const (
 	DelayAnnotationName          = "approval.nephio.org/delay"
-	DelayConditionType           = "approval.nephio.org.DelayExpired"
 	PolicyAnnotationName         = "approval.nephio.org/policy"
 	InitialPolicyAnnotationValue = "initial"
 )
@@ -104,44 +103,16 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
-	// If it is published, ignore it
-	if porchv1alpha1.LifecycleIsPublished(pr.Spec.Lifecycle) {
-		return ctrl.Result{}, nil
-	}
-
-	// Delay if needed
-	// This is a workaround for some "settling" that seems to be needed
-	// in Porch and/or PackageVariant. We should be able to remove it if
-	// we can fix that.
-	requeue, err := r.manageDelay(ctx, pr)
-	if err != nil {
-		r.recorder.Eventf(pr, corev1.EventTypeWarning,
-			"Error", "error processing %q: %s", DelayAnnotationName, err.Error())
-
-		return ctrl.Result{}, err
-	}
-
-	// if requeue is > 0, then we should do nothing more with this PackageRevision
-	// for at least that long
-	if requeue > 0 {
-		r.recorder.Event(pr, corev1.EventTypeNormal,
-			"NotApproved", "delay time not met")
-		return ctrl.Result{RequeueAfter: requeue}, nil
-	}
-
-	// Check for the approval policy annotation
-	policy, ok := pr.GetAnnotations()[PolicyAnnotationName]
+	// If we shouldn't process this at all, just return
+	policy, ok := shouldProcess(pr)
 	if !ok {
-		// no policy set, so just return, we are done
 		return ctrl.Result{}, nil
 	}
 
 	// If the package revision is owned by a PackageVariant, check the Ready condition
 	// of the package variant. If it is not Ready, then we should not approve yet. The
 	// lack of readiness could indicate an error which even impacts whether or not the
 	// readiness gates have been properly set.
-	//
-	//
 	pvReady, err := porchutil.PackageVariantReady(ctx, pr, r.porchClient)
 	if err != nil {
 		r.recorder.Event(pr, corev1.EventTypeWarning,
@@ -192,7 +163,32 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
-	// policy met
+	// Delay if needed, and let the user know via an event
+	// We should be able to get rid of this if we add a policy to check
+	// the specializer condition. We need to check the *specific* condition,
+	// because if the condition has not been added to the readiness gates yet,
+	// we could pass all the gates even though that specific condition is missing.
+	// That check shouldn't be needed if the initial clone creates the readiness gate
+	// entry though (with the function pipeline run).
+	requeue, err := manageDelay(pr)
+	if err != nil {
+		r.recorder.Eventf(pr, corev1.EventTypeWarning,
+			"Error", "error processing %q: %s", DelayAnnotationName, err.Error())
+
+		// Do not propagate the error; we do not want it to force an immediate requeue
+		// If we could not parse the annotation, it is a user error
+		return ctrl.Result{}, nil
+	}
+
+	// if requeue is > 0, then we should do nothing more with this PackageRevision
+	// for at least that long
+	if requeue > 0 {
+		r.recorder.Event(pr, corev1.EventTypeNormal,
+			"NotApproved", "delay time not met")
+		return ctrl.Result{RequeueAfter: requeue}, nil
+	}
+
+	// All policies met
 	if pr.Spec.Lifecycle == porchv1alpha1.PackageRevisionLifecycleDraft {
 		pr.Spec.Lifecycle = porchv1alpha1.PackageRevisionLifecycleProposed
 		err = r.Update(ctx, pr)
@@ -211,19 +207,33 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	return ctrl.Result{}, err
 }
 
-func (r *reconciler) manageDelay(ctx context.Context, pr *porchv1alpha1.PackageRevision) (time.Duration, error) {
+func shouldProcess(pr *porchv1alpha1.PackageRevision) (string, bool) {
+	result := true
+
+	// If it is published, ignore it
+	result = result && !porchv1alpha1.LifecycleIsPublished(pr.Spec.Lifecycle)
+
+	// Check for the approval policy annotation
+	policy, ok := pr.GetAnnotations()[PolicyAnnotationName]
+	result = result && ok
+
+	return policy, result
+}
+
+func manageDelay(pr *porchv1alpha1.PackageRevision) (time.Duration, error) {
 	delay, ok := pr.GetAnnotations()[DelayAnnotationName]
 	if !ok {
-		delay = "2m"
+		// only delay if there is a delay annotation
+		return 0, nil
 	}
+
 	d, err := time.ParseDuration(delay)
 	if err != nil {
-		return 0, fmt.Errorf("error parsing delay duration: %w", err)
+		return 0, err
 	}
 
-	// force at least a 30 second delay
-	if d < 30*time.Second {
-		d = 30 * time.Second
+	if d < 0 {
+		return 0, fmt.Errorf("invalid delay %q; delay must be 0 or more", delay)
 	}
 
 	if time.Since(pr.CreationTimestamp.Time) > d {
diff --git a/controllers/pkg/reconcilers/approval/reconciler_test.go b/controllers/pkg/reconcilers/approval/reconciler_test.go
@@ -0,0 +1,149 @@
+// Copyright 2023 The Nephio Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package approval
+
+import (
+	"testing"
+	"time"
+
+	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestShouldProcess(t *testing.T) {
+	testCases := map[string]struct {
+		pr             porchapi.PackageRevision
+		expectedPolicy string
+		expectedShould bool
+	}{
+		"draft with no annotation": {
+			pr:             porchapi.PackageRevision{},
+			expectedPolicy: "",
+			expectedShould: false,
+		},
+		"draft with policy annotation": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"approval.nephio.org/policy": "initial",
+					},
+				},
+			},
+			expectedPolicy: "initial",
+			expectedShould: true,
+		},
+		"draft with no policy annotation, but delay annotation": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"approval.nephio.org/delay": "20s",
+					},
+				},
+			},
+			expectedPolicy: "",
+			expectedShould: false,
+		},
+		"published with policy annotation": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"approval.nephio.org/policy": "initial",
+					},
+				},
+				Spec: porchapi.PackageRevisionSpec{
+					Lifecycle: "Published",
+				},
+			},
+			expectedPolicy: "initial",
+			expectedShould: false,
+		},
+	}
+	for tn, tc := range testCases {
+		t.Run(tn, func(t *testing.T) {
+			actualPolicy, actualShould := shouldProcess(&tc.pr)
+			require.Equal(t, tc.expectedPolicy, actualPolicy)
+			require.Equal(t, tc.expectedShould, actualShould)
+		})
+	}
+}
+
+func TestManageDelay(t *testing.T) {
+	now := time.Now()
+	testCases := map[string]struct {
+		pr             porchapi.PackageRevision
+		expectedRequeue bool
+		expectedError bool
+	}{
+		"no annotation": {
+			pr:             porchapi.PackageRevision{},
+			expectedRequeue: false,
+			expectedError: false,
+		},
+		"unparseable annotation": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"approval.nephio.org/delay": "foo",
+					},
+				},
+			},
+			expectedRequeue: false,
+			expectedError: true,
+		},
+		"negative annotation": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"approval.nephio.org/delay": "-5s",
+					},
+				},
+			},
+			expectedRequeue: false,
+			expectedError: true,
+		},
+		"not old enough": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					CreationTimestamp: metav1.Time{now},
+					Annotations: map[string]string{
+						"approval.nephio.org/delay": "1h",
+					},
+				},
+			},
+			expectedRequeue: true,
+			expectedError: false,
+		},
+		"old enough": {
+			pr: porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					CreationTimestamp: metav1.Time{now.AddDate(-1,0,0)},
+					Annotations: map[string]string{
+						"approval.nephio.org/delay": "1h",
+					},
+				},
+			},
+			expectedRequeue: false,
+			expectedError: false,
+		},
+	}
+	for tn, tc := range testCases {
+		t.Run(tn, func(t *testing.T) {
+			actualRequeue, actualError := manageDelay(&tc.pr)
+			require.Equal(t, tc.expectedRequeue, actualRequeue > 0)
+			require.Equal(t, tc.expectedError, actualError != nil)
+		})
+	}
+}
