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.  This 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.

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

@@ -27,6 +27,9 @@ func (n noopExpectations) Expect(o runtime.Object) {
 func (n noopExpectations) CancelExpect(o runtime.Object) {
 }
 
+func (n noopExpectations) TryCancelExpect(o runtime.Object) {
+}
+
 func (n noopExpectations) ExpectationsDone() {
 }
 

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.
+	mutator       objDataMutator            // functions that mutate objData types during their creation.  Allows for defaults.
 }
 
-func newObjTracker(gvk schema.GroupVersionKind) *objectTracker {
+func newObjTracker(gvk schema.GroupVersionKind, mutator objDataMutator) *objectTracker {
+	if mutator == nil {
+		mutator = setRetriesFromFlag
+	}
+
 	return &objectTracker{
 		gvk:       gvk,
 		cancelled: make(objSet),
 		expect:    make(objSet),
 		seen:      make(objSet),
 		satisfied: make(objSet),
+		mutator:   mutator,
 	}
 }
 
@@ -99,11 +109,11 @@ 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] = mutatedObjData(t.mutator)
 		return
 	}
 
-	t.expect[k] = struct{}{}
+	t.expect[k] = mutatedObjData(t.mutator)
 }
 
 // CancelExpect cancels an expectation and marks it so it
@@ -126,7 +136,33 @@ func (t *objectTracker) CancelExpect(o runtime.Object) {
 	delete(t.expect, k)
 	delete(t.seen, k)
 	delete(t.satisfied, k)
-	t.cancelled[k] = struct{}{}
+	t.cancelled[k] = mutatedObjData(t.mutator)
+}
+
+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
+	}
+
+	obj := t.expect[k]
+	shouldDelete := obj.decrementRetries()
+	t.expect[k] = obj
+	if shouldDelete {
+		delete(t.expect, k)
+		delete(t.seen, k)
+		delete(t.satisfied, k)
+		t.cancelled[k] = mutatedObjData(t.mutator)
+	}
 }
 
 // ExpectationsDone tells the tracker to stop accepting new expectations.
@@ -187,7 +223,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] = mutatedObjData(t.mutator)
 		return
 	case !wasExpecting && t.populated:
 		// Not expecting and no longer accepting expectations.
@@ -197,7 +233,7 @@ func (t *objectTracker) Observe(o runtime.Object) {
 	}
 
 	// Track for future expectation.
-	t.seen[k] = struct{}{}
+	t.seen[k] = mutatedObjData(t.mutator)
 }
 
 func (t *objectTracker) Populated() bool {
@@ -249,7 +285,7 @@ func (t *objectTracker) Satisfied() bool {
 		}
 		delete(t.seen, k)
 		delete(t.expect, k)
-		t.satisfied[k] = struct{}{}
+		t.satisfied[k] = mutatedObjData(t.mutator)
 		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,66 @@ 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(o *objData) *objData {
+		o.retries = 0
+		return o
+	})
+
+	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(o *objData) *objData {
+		o.retries = 2
+		return o
+	})
+
+	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")
+}

pkg/readiness/objset.go

@@ -27,9 +27,42 @@ type objKey struct {
 	namespacedName types.NamespacedName
 }
 
-type objSet map[objKey]struct{}
-
 func (k objKey) String() string {
 	return fmt.Sprintf("%s [%s]", k.namespacedName.String(), k.gvk.String())
+}
+
+type objData struct {
+	retries int
+}
+
+// decrementRetries handles objData retries, and returns whether it's time to delete the objData.
+func (o *objData) decrementRetries() bool {
+	// if retries is less than 0, allowed retries are infinite
+	if o.retries < 0 {
+		return false
+	}
+
+	// If we have retries left, use one
+	if o.retries > 0 {
+		o.retries--
+		return false
+	}
 
+	// if we have zero retries, we can delete
+	return true
 }
+
+type objDataMutator func(*objData) *objData
+
+func setRetriesFromFlag(o *objData) *objData {
+	o.retries = *readinessRetries
+	return o
+}
+
+func mutatedObjData(mut objDataMutator) objData {
+	out := objData{}
+	mut(&out)
+	return out
+}
+
+type objSet map[objKey]objData

pkg/readiness/ready_tracker.go

@@ -70,8 +70,8 @@ type Tracker struct {
 func NewTracker(lister Lister) *Tracker {
 	return &Tracker{
 		lister:             lister,
-		templates:          newObjTracker(v1beta1.SchemeGroupVersion.WithKind("ConstraintTemplate")),
-		config:             newObjTracker(configv1alpha1.GroupVersion.WithKind("Config")),
+		templates:          newObjTracker(v1beta1.SchemeGroupVersion.WithKind("ConstraintTemplate"), nil),
+		config:             newObjTracker(configv1alpha1.GroupVersion.WithKind("Config"), nil),
 		constraints:        newTrackerMap(),
 		data:               newTrackerMap(),
 		ready:              make(chan struct{}),

pkg/readiness/tracker_map.go

@@ -64,7 +64,7 @@ func (t *trackerMap) Get(gvk schema.GroupVersionKind) Expectations {
 
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	entry := newObjTracker(gvk)
+	entry := newObjTracker(gvk, nil)
 	t.m[gvk] = entry
 	return entry
 }