Add the readiness-retries flag, allowing the user to configure how many

attempts should be made to injest a resource into OPA while blocking the
webhook.

Previously, a failure to injest an object into the OPA cache would
strike that object from the ObjectTracker, a type tasked with blocking
the webhook from serving requests until all the necessary objects had
been observed on the API server.  That setup optimizes for availability
(the webhook serving requests ASAP) over security.

By adding retry functionality, we configure gatekeeper to block the
webhook for longer, but unblock with a more complete set of constraints.

fixes: #213

Signed-off-by: juliankatz <juliankatz@google.com>

pkg/controller/constraint/constraint_controller.go

@@ -387,7 +387,7 @@ func (r *ReconcileConstraint) cacheConstraint(instance *unstructured.Unstructure
 	unstructured.RemoveNestedField(obj.Object, "status")
 	_, err := r.opa.AddConstraint(context.Background(), obj)
 	if err != nil {
-		t.CancelExpect(obj)
+		t.TryCancelExpect(obj)
 		return err
 	}
 

pkg/readiness/noop_expectations.go

@@ -18,20 +18,17 @@ package readiness
 import "k8s.io/apimachinery/pkg/runtime"
 
 // noopExpectations returns an Expectations that is always satisfied.
-type noopExpectations struct {
-}
+type noopExpectations struct{}
 
-func (n noopExpectations) Expect(o runtime.Object) {
-}
+func (n noopExpectations) Expect(o runtime.Object) {}
 
-func (n noopExpectations) CancelExpect(o runtime.Object) {
-}
+func (n noopExpectations) CancelExpect(o runtime.Object) {}
 
-func (n noopExpectations) ExpectationsDone() {
-}
+func (n noopExpectations) TryCancelExpect(o runtime.Object) {}
 
-func (n noopExpectations) Observe(o runtime.Object) {
-}
+func (n noopExpectations) ExpectationsDone() {}
+
+func (n noopExpectations) Observe(o runtime.Object) {}
 
 func (n noopExpectations) Satisfied() bool {
 	return true

pkg/readiness/object_tracker.go

@@ -16,6 +16,7 @@ limitations under the License.
 package readiness
 
 import (
+	"flag"
 	"fmt"
 	"sync"
 
@@ -29,13 +30,16 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
+var readinessRetries = flag.Int("readiness-retries", 0, "The number of resource ingestion attempts allowed before the resource is disregarded.  A value of -1 will retry indefinitely.")
+
 // Expectations tracks expectations for runtime.Objects.
 // A set of Expect() calls are made, demarcated by ExpectationsDone().
 // Expectations are satisfied by calls to Observe().
 // Once all expectations are satisfied, Satisfied() will begin returning true.
 type Expectations interface {
 	Expect(o runtime.Object)
 	CancelExpect(o runtime.Object)
+	TryCancelExpect(o runtime.Object)
 	ExpectationsDone()
 	Observe(o runtime.Object)
 	Satisfied() bool
@@ -56,15 +60,21 @@ type objectTracker struct {
 	populated     bool                      // all expectations have been provided
 	allSatisfied  bool                      // true once all expectations have been satisfied. Acts as a circuit-breaker.
 	kindsSnapshot []schema.GroupVersionKind // Snapshot of kinds before freeing memory in Satisfied.
+	newObjData    objDataFactory            // Function that creates objData types.  Allows for setting defaults.
 }
 
-func newObjTracker(gvk schema.GroupVersionKind) *objectTracker {
+func newObjTracker(gvk schema.GroupVersionKind, fn objDataFactory) *objectTracker {
+	if fn == nil {
+		fn = objDataFromFlags
+	}
+
 	return &objectTracker{
-		gvk:       gvk,
-		cancelled: make(objSet),
-		expect:    make(objSet),
-		seen:      make(objSet),
-		satisfied: make(objSet),
+		gvk:        gvk,
+		cancelled:  make(objSet),
+		expect:     make(objSet),
+		seen:       make(objSet),
+		satisfied:  make(objSet),
+		newObjData: fn,
 	}
 }
 
@@ -99,11 +109,18 @@ func (t *objectTracker) Expect(o runtime.Object) {
 	if _, ok := t.seen[k]; ok {
 		delete(t.seen, k)
 		delete(t.expect, k)
-		t.satisfied[k] = struct{}{}
+		t.satisfied[k] = *t.newObjData()
 		return
 	}
 
-	t.expect[k] = struct{}{}
+	t.expect[k] = *t.newObjData()
+}
+
+func (t *objectTracker) cancelExpectNoLock(k objKey) {
+	delete(t.expect, k)
+	delete(t.seen, k)
+	delete(t.satisfied, k)
+	t.cancelled[k] = *t.newObjData()
 }
 
 // CancelExpect cancels an expectation and marks it so it
@@ -123,10 +140,32 @@ func (t *objectTracker) CancelExpect(o runtime.Object) {
 		return
 	}
 
-	delete(t.expect, k)
-	delete(t.seen, k)
-	delete(t.satisfied, k)
-	t.cancelled[k] = struct{}{}
+	t.cancelExpectNoLock(k)
+}
+
+func (t *objectTracker) TryCancelExpect(o runtime.Object) {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+
+	// Respect circuit-breaker.
+	if t.allSatisfied {
+		return
+	}
+
+	k, err := objKeyFromObject(o)
+	if err != nil {
+		log.Error(err, "skipping")
+		return
+	}
+
+	// Check if it's time to delete an expectation or just decrement its allotted retries
+	obj := t.expect[k]
+	shouldDel := obj.decrementRetries()
+	t.expect[k] = obj // set the changed obj back to the map, as the value is not a pointer
+
+	if shouldDel {
+		t.cancelExpectNoLock(k)
+	}
 }
 
 // ExpectationsDone tells the tracker to stop accepting new expectations.
@@ -187,7 +226,7 @@ func (t *objectTracker) Observe(o runtime.Object) {
 		// Satisfy existing expectation
 		delete(t.seen, k)
 		delete(t.expect, k)
-		t.satisfied[k] = struct{}{}
+		t.satisfied[k] = *t.newObjData()
 		return
 	case !wasExpecting && t.populated:
 		// Not expecting and no longer accepting expectations.
@@ -197,7 +236,7 @@ func (t *objectTracker) Observe(o runtime.Object) {
 	}
 
 	// Track for future expectation.
-	t.seen[k] = struct{}{}
+	t.seen[k] = *t.newObjData()
 }
 
 func (t *objectTracker) Populated() bool {
@@ -249,7 +288,7 @@ func (t *objectTracker) Satisfied() bool {
 		}
 		delete(t.seen, k)
 		delete(t.expect, k)
-		t.satisfied[k] = struct{}{}
+		t.satisfied[k] = *t.newObjData()
 		resolveCount++
 	}
 	log.V(1).Info("resolved pre-observations", "gvk", t.gvk, "count", resolveCount)

pkg/readiness/object_tracker_test.go

@@ -45,22 +45,22 @@ func makeCTSlice(prefix string, count int) []runtime.Object {
 // Verify that an unpopulated tracker is unsatisfied.
 func Test_ObjectTracker_Unpopulated_Is_Unsatisfied(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "unpopulated tracker should not be satisfied")
 }
 
 // Verify that an populated tracker with no expectations is satisfied.
 func Test_ObjectTracker_No_Expectations(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 	ot.ExpectationsDone()
 	g.Expect(ot.Satisfied()).To(gomega.BeTrue(), "populated tracker with no expectations should be satisfied")
 }
 
 // Verify that that multiple expectations are tracked correctly.
 func Test_ObjectTracker_Multiple_Expectations(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 
 	const count = 10
 	ct := makeCTSlice("ct-", count)
@@ -69,6 +69,7 @@ func Test_ObjectTracker_Multiple_Expectations(t *testing.T) {
 	}
 	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before ExpectationsDone")
 	ot.ExpectationsDone()
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied after ExpectationsDone")
 
 	for i := 0; i < len(ct); i++ {
 		g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before observations are done")
@@ -80,7 +81,7 @@ func Test_ObjectTracker_Multiple_Expectations(t *testing.T) {
 // Verify that observations can precede expectations.
 func Test_ObjectTracker_Seen_Before_Expect(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 	ct := makeCT("test-ct")
 	ot.Observe(ct)
 	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "unpopulated tracker should not be satisfied")
@@ -93,7 +94,7 @@ func Test_ObjectTracker_Seen_Before_Expect(t *testing.T) {
 // Verify that terminated resources are ignored when calling Expect.
 func Test_ObjectTracker_Terminated_Expect(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 	ct := makeCT("test-ct")
 	now := metav1.Now()
 	ct.ObjectMeta.DeletionTimestamp = &now
@@ -105,7 +106,7 @@ func Test_ObjectTracker_Terminated_Expect(t *testing.T) {
 // Verify that that expectations can be cancelled.
 func Test_ObjectTracker_Cancelled_Expectations(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 
 	const count = 10
 	ct := makeCTSlice("ct-", count)
@@ -132,7 +133,7 @@ func Test_ObjectTracker_Cancelled_Expectations(t *testing.T) {
 // Verify that that duplicate expectations only need a single observation.
 func Test_ObjectTracker_Duplicate_Expectations(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 
 	const count = 10
 	ct := makeCTSlice("ct-", count)
@@ -153,7 +154,7 @@ func Test_ObjectTracker_Duplicate_Expectations(t *testing.T) {
 // Verify that an expectation can be canceled before it's first expected.
 func Test_ObjectTracker_CancelBeforeExpect(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 	ct := makeCT("test-ct")
 	ot.CancelExpect(ct)
 	ot.Expect(ct)
@@ -166,7 +167,7 @@ func Test_ObjectTracker_CancelBeforeExpect(t *testing.T) {
 // no other operations have any impact.
 func Test_ObjectTracker_CircuitBreaker(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 
 	const count = 10
 	ct := makeCTSlice("ct-", count)
@@ -221,7 +222,7 @@ func Test_ObjectTracker_CircuitBreaker(t *testing.T) {
 // the circuit-breaker trips (which releases memory it depends on).
 func Test_ObjectTracker_kinds(t *testing.T) {
 	g := gomega.NewWithT(t)
-	ot := newObjTracker(schema.GroupVersionKind{})
+	ot := newObjTracker(schema.GroupVersionKind{}, nil)
 
 	const count = 10
 	ct := makeCTSlice("ct-", count)
@@ -243,3 +244,93 @@ func Test_ObjectTracker_kinds(t *testing.T) {
 	g.Expect(kindsBefore).ShouldNot(gomega.BeEmpty(), "expected non-empty kinds")
 	g.Expect(kindsAfter).Should(gomega.Equal(kindsBefore), "expected kinds to match")
 }
+
+// Verify that TryCancelExpect functions the same as regular CancelExpect if readinessRetries is set to 0
+func Test_ObjectTracker_TryCancelExpect_Default(t *testing.T) {
+	g := gomega.NewWithT(t)
+	ot := newObjTracker(schema.GroupVersionKind{}, func() *objData {
+		return &objData{retries: 0}
+	})
+
+	const count = 10
+	ct := makeCTSlice("ct-", count)
+	for i := 0; i < len(ct); i++ {
+		ot.Expect(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before ExpectationsDone")
+	ot.ExpectationsDone()
+
+	// Skip the first two
+	for i := 2; i < len(ct); i++ {
+		g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before observations are done")
+		ot.Observe(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "two expectation remain")
+
+	ot.TryCancelExpect(ct[0])
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "one expectation remains")
+
+	ot.TryCancelExpect(ct[1])
+	g.Expect(ot.Satisfied()).To(gomega.BeTrue(), "should be satisfied")
+}
+
+// Verify that TryCancelExpect must be called multiple times before an expectation is cancelled
+func Test_ObjectTracker_TryCancelExpect_WithRetries(t *testing.T) {
+	g := gomega.NewWithT(t)
+	ot := newObjTracker(schema.GroupVersionKind{}, func() *objData {
+		return &objData{retries: 2}
+	})
+
+	const count = 10
+	ct := makeCTSlice("ct-", count)
+	for i := 0; i < len(ct); i++ {
+		ot.Expect(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before ExpectationsDone")
+	ot.ExpectationsDone()
+
+	// Skip the first one
+	for i := 1; i < len(ct); i++ {
+		g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before observations are done")
+		ot.Observe(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "one expectation remains with two retries")
+
+	ot.TryCancelExpect(ct[0])
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "one expectation remains with one retries")
+
+	ot.TryCancelExpect(ct[0])
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "one expectation remains with zero retries")
+
+	ot.TryCancelExpect(ct[0])
+	g.Expect(ot.Satisfied()).To(gomega.BeTrue(), "should be satisfied")
+}
+
+// Verify that TryCancelExpect can be called many times without the tracker ever being satisfied,
+// due to the infinite retries setting.
+func Test_ObjectTracker_TryCancelExpect_InfiniteRetries(t *testing.T) {
+	g := gomega.NewWithT(t)
+	ot := newObjTracker(schema.GroupVersionKind{}, func() *objData {
+		return &objData{retries: -1}
+	})
+
+	const count = 10
+	ct := makeCTSlice("ct-", count)
+	for i := 0; i < len(ct); i++ {
+		ot.Expect(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before ExpectationsDone")
+	ot.ExpectationsDone()
+
+	// Skip the first one
+	for i := 1; i < len(ct); i++ {
+		g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "should not be satisfied before observations are done")
+		ot.Observe(ct[i])
+	}
+	g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "one expectation should remain after two retries")
+
+	for i := 0; i < 20; i++ {
+		ot.TryCancelExpect(ct[0])
+		g.Expect(ot.Satisfied()).NotTo(gomega.BeTrue(), "expectation should remain due to infinite retries")
+	}
+}