Bug 1986408: NE-310 HSTS Route Admission Plugin

[candita]

Administrators who are tasked with route management and/or regulatory compliance face a number of issues with regard to enforcing HSTS. For efficiency and protection against configuration errors, they have requested to automatically and globally enable HSTS on the basis of the cluster Ingress domains. However, because enabling HSTS automatically and globally can cause disruption of service on a wide scale, they should also be able to audit and change HSTS configuration without further outages. Finally, they should be able to consistently apply the same HSTS configuration and predict the outcome on any cluster.

• Register a new plugin route.openshift.io/RequiredRouteAnnotations
• Add a new admissions package called requiredrouteannotations, with methods to validate route annotation configuration
• Add unit tests

Enhancement proposal: openshift/enhancements#749

[candita]

/test verify

[candita]

/test verify

[sttts]

this races

[sttts]

needs locking or an atomic datatype

[candita]

Discussed with @sanchezl - removing the boolean, and now checking every time.

[frobware]

We could do the following:

diff --git a/pkg/route/apiserver/admission/requiredrouteannotations/admission.go b/pkg/route/apiserver/admission/requiredrouteannotations/admission.go
index f6b2f9279..268ab3fb0 100644
--- a/pkg/route/apiserver/admission/requiredrouteannotations/admission.go
+++ b/pkg/route/apiserver/admission/requiredrouteannotations/admission.go
@@ -9,6 +9,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -50,7 +51,7 @@ type requiredRouteAnnotations struct {
 	routeLister   routev1listers.RouteLister
 	nsLister      corev1listers.NamespaceLister
 	ingressLister configv1listers.IngressLister
-	cachesSynced  bool
+	cachesSynced  cacheSync
 	cachesToSync  []cache.InformerSynced
 }
 
@@ -62,6 +63,24 @@ var _ = openshiftapiserveradmission.WantsOpenShiftRouteInformers(&requiredRouteA
 
 var maxAgeRegExp = regexp.MustCompile(`max-age=(\d+)`)
 
+type cacheSync struct {
+	readMu    sync.RWMutex
+	writeMu   sync.RWMutex
+	hasSynced bool
+}
+
+func (cs *cacheSync) HasSycned() bool {
+	cs.readMu.Lock()
+	defer cs.readMu.Unlock()
+	return cs.hasSynced
+}
+
+func (cs *cacheSync) SetSynced() {
+	cs.writeMu.Lock()
+	cs.hasSynced = true
+	cs.writeMu.Unlock()
+}
+
 // Validate ensures that routes specify required annotations, and returns nil if valid.
 // The admission handler ensures this is only called for Create/Update operations.
 func (o *requiredRouteAnnotations) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) (err error) {
@@ -90,11 +109,11 @@ func (o *requiredRouteAnnotations) Validate(ctx context.Context, a admission.Att
 	}
 
 	// Wait up to 30 seconds for all caches to sync.  This is needed only once.
-	if !o.cachesSynced {
-		if synced := o.waitForSyncedStore(ctx); !synced {
+	if !o.cachesSynced.HasSycned() {
+		if !o.waitForSyncedStore(ctx) {
 			return admission.NewForbidden(a, errors.New(pluginName+": caches not synchronized"))
 		}
-		o.cachesSynced = true
+		o.cachesSynced.SetSynced()
 	}
 
 	ingress, err := o.ingressLister.Get("cluster")

or more discretely with locking on the read and write access. I tried to encapsulate here to avoid any mistakes.

[frobware]

fixing my typo;

func (cs *cacheSync) HasSynced() bool {

[candita]

I made the funcs private and changed a few other minor things.

[candita]

/test e2e-aws-upgrade

[sanchezl]

/lgtm
/retest

[frobware]

@candita, @sttts mentioned to me that the we only have 30s for admission calls to complete so let's go for 20s on the sync. Sorry for the churn.

[frobware]

/lgtm

[sttts]

Suggested change

	cacheSyncLock cacheSync
	<empty line>
	isSyncedLock sync.RWMutex
	isSynced bool

[sttts]

And delete the data type above

[candita]

per our chat, this is a moot point

[sttts]

Suggested change

	if !o.cacheSyncLock.hasSynced() {
	if !o.waitForSyncedStore(ctx) {
	return admission.NewForbidden(a, errors.New(pluginName+": caches not synchronized"))
	}
	o.cacheSyncLock.setSynced()
	}
	if !o.waitForSyncedStore(ctx) {
	return admission.NewForbidden(a, errors.New(pluginName+": caches not synchronized"))
	}

[sttts]

Suggested change

	func (o *requiredRouteAnnotations) waitForSyncedStore(ctx context.Context) bool {
	syncCtx, cancelFn := context.WithTimeout(ctx, timeToWaitForCacheSync)
	defer cancelFn()
	return cache.WaitForCacheSync(syncCtx.Done(), o.cachesToSync...)
	}
	func (o *requiredRouteAnnotations) waitForSyncedStore(ctx context.Context) bool {
	isSyncedLock.RLock()
	defer isSyncedLock.RLock()
	if o.isSynced {
	return nil
	}
	syncCtx, cancelFn := context.WithTimeout(ctx, timeToWaitForCacheSync)
	defer cancelFn()
	err := cache.WaitForCacheSync(syncCtx.Done(), o.cachesToSync...)
	if err != nil {
	return err
	}
	o.isSyncedLock.Lock()
	defer o.isSyncedLock.Unlock()
	o.isSynced = true
	return err
	}

[candita]

I made a similar change, ptal and let me know if you approve by approving. :)

[frobware]

Don't know whether we're just endlessly bike shedding now, particularly if this is temporary and we're gravitating to a more general solution, but creating the context should move insider the if below. Is there any value in creating this each and every call?

[candita]

I will take this as a followup, per our discussion.

[frobware]

/lgtm

[sttts]

/retest

[sttts]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, frobware, sanchezl, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@candita: All pull requests linked via external trackers have merged:

• openshift/openshift-apiserver#224

Bugzilla bug 1986408 has been moved to the MODIFIED state.

In response to this:

Bug 1986408: NE-310 HSTS Route Admission Plugin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.