address fixmes and todos; add godocs

Author: stlaz

pkg/psalabelsyncer/podsecurity_label_sync_controller.go

@@ -33,6 +33,10 @@ const (
 	labelSyncControlLabel = "security.openshift.io/scc.podSecurityLabelSync"
 )
 
+// PodSecurityAdmissionLabelSynchronizationController watches over namespaces labelled with
+// "security.openshift.io/scc.podSecurityLabelSync: true" and configures the PodSecurity
+// admission namespace label to match the user account privileges in terms of being able
+// to use SCCs
 type PodSecurityAdmissionLabelSynchronizationController struct {
 	namespaceClient corev1client.NamespaceInterface
 
@@ -109,7 +113,7 @@ func NewPodSecurityAdmissionLabelSynchronizationController(
 			rbacInformers.ClusterRoles().Informer(),
 		).
 		WithFilteredEventsInformersQueueKeysFunc(
-			nsToQueueKey,
+			nameToKey,
 			func(obj interface{}) bool {
 				ns, ok := obj.(*corev1.Namespace)
 				if !ok {
@@ -118,7 +122,6 @@ func NewPodSecurityAdmissionLabelSynchronizationController(
 					return false
 				}
 				// the SCC mapping requires the annotation
-				// FIXME: make the mapping not panic but error out instead
 				if ns.Annotations == nil || len(ns.Annotations[securityv1.UIDRangeAnnotation]) == 0 {
 					return false
 				}
@@ -158,14 +161,14 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) sync(ctx context.Co
 		return fmt.Errorf(errFmt, qKey, err)
 	}
 
-	if err := c.syncNamespace(ctx, ns); err != nil {
+	if err := c.syncNamespace(ctx, controllerContext, ns); err != nil {
 		return fmt.Errorf(errFmt, qKey, err)
 	}
 
 	return nil
 }
 
-func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx context.Context, ns *corev1.Namespace) error {
+func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx context.Context, controllerContext factory.SyncContext, ns *corev1.Namespace) error {
 	// We cannot safely determine the SCC level for an NS until it gets the UID annotation.
 	// No need to care about re-queueing the key, we should get the NS once it is updated
 	// with the annotation.
@@ -194,7 +197,10 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx c
 			// TODO: the SCC was removed in the meantime and synced in the cache?
 			return err
 		}
-		sccPSaLevel := convertSCCToPSALevel(ns, scc)
+		sccPSaLevel, err := convertSCCToPSALevel(ns, scc)
+		if err != nil {
+			return err
+		}
 
 		if sccPSaLevel > currentNSLevel {
 			currentNSLevel = sccPSaLevel
@@ -226,14 +232,18 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx c
 	return nil
 }
 
-func nsToQueueKey(obj runtime.Object) []string {
+// nameToKey turns a meta object into a key by using the object's name.
+func nameToKey(obj runtime.Object) []string {
 	metaObj, ok := obj.(metav1.ObjectMetaAccessor)
 	if !ok {
 		return factory.DefaultQueueKeysFunc(obj)
 	}
 	return []string{metaObj.GetObjectMeta().GetName()}
 }
 
+// queueKeysForObj returns slice with:
+// - a namespace name for a namespaced object
+// - all cluster namespaces names for cluster-wide objects
 func (c *PodSecurityAdmissionLabelSynchronizationController) queueKeysForObj(obj runtime.Object) []string {
 	metaObj, ok := obj.(metav1.ObjectMetaAccessor)
 	if !ok {
@@ -248,6 +258,7 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) queueKeysForObj(obj
 	return c.allWatchedNamespacesAsQueueKeys(obj)
 }
 
+// allWatchedNamespacesAsQueueKeys returns all namespace names slice irregardles of the retrieved object.
 func (c *PodSecurityAdmissionLabelSynchronizationController) allWatchedNamespacesAsQueueKeys(_ runtime.Object) []string {
 	namespaces, err := c.namespaceLister.List(c.nsLabelSelector)
 	if err != nil && !apierrors.IsNotFound(err) {
@@ -262,6 +273,8 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) allWatchedNamespace
 	return qKeys
 }
 
+// controlledNamespacesLabelSelector returns label selector to be used with the
+// PodSecurityAdmissionLabelSynchronizationController.
 func controlledNamespacesLabelSelector() (labels.Selector, error) {
 	labelRequirement, err := labels.NewRequirement(labelSyncControlLabel, selection.NotEquals, []string{"false"})
 	if err != nil {

pkg/psalabelsyncer/sccrolecache.go

@@ -7,6 +7,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/serviceaccount"
@@ -15,6 +16,7 @@ import (
 	rbacv1informers "k8s.io/client-go/informers/rbac/v1"
 	rbacv1listers "k8s.io/client-go/listers/rbac/v1"
 	"k8s.io/client-go/tools/cache"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac"
 
@@ -23,8 +25,12 @@ import (
 	securityv1listers "github.com/openshift/client-go/security/listers/security/v1"
 )
 
+// The index name to be used along with the BySAIndexKeys indexing function
 const BySAIndexName = "ByServiceAccount"
 
+// SAToSCCCache is a construct that caches roles and rolebindings
+// (and their cluster variants) and based on that and on SCCs present in the cluster
+// it allows retrieving a set of SCCs for a given ServiceAccount
 type SAToSCCCache struct {
 	roleLister                rbacv1listers.RoleLister
 	clusterRoleLister         rbacv1listers.ClusterRoleLister
@@ -75,18 +81,21 @@ func (r *roleBindingObj) Subjects() []rbacv1.Subject {
 	return r.roleBinding.Subjects
 }
 
-func (r *roleBindingObj) AppliesToNS(ns string) bool {
+func (r *roleBindingObj) Namespace() string {
 	if r.clusterRoleBinding != nil {
-		return true
+		return ""
 	}
-	return ns == r.roleBinding.Namespace
+	return r.roleBinding.Namespace
 }
 
-func (r *roleBindingObj) Namespace() string {
+// AppliesToNS returns true if:
+// - the wrapped object is a cluster role binding
+// - the namespace of a wrapped rolebinding matches the namespace supplied in `ns`
+func (r *roleBindingObj) AppliesToNS(ns string) bool {
 	if r.clusterRoleBinding != nil {
-		return ""
+		return true
 	}
-	return r.roleBinding.Namespace
+	return ns == r.roleBinding.Namespace
 }
 
 // roleObj helps to handle roles and clusterroles in a generic manner
@@ -133,6 +142,11 @@ func (r *roleObj) Namespace() string {
 	return r.role.Namespace
 }
 
+// BySAIndexKeys is a cache.IndexFunc indexing function that shall be used on
+// rolebinding and clusterrolebinding informer caches.
+// It retrieves the subjects of the incoming object and if there are SA, SA groups
+// or the system:authenticated group subjects, these will all be returned as a slice
+// of strings to create an index for the SA or SA group.
 func BySAIndexKeys(obj interface{}) ([]string, error) {
 	roleBinding, err := newRoleBindingObj(obj)
 	if err != nil {
@@ -163,17 +177,13 @@ func NewSAToSCCCache(rbacInformers rbacv1informers.Interface, sccInfomer securit
 
 		sccLister: sccInfomer.Lister(),
 
-		// TODO: do I need these?
-		rolesSynced:        rbacInformers.Roles().Informer().HasSynced,
-		roleBindingsSynced: rbacInformers.RoleBindings().Informer().HasSynced,
-
 		usefulRoles: make(map[string][]string),
 	}
 }
 
 // SCCsFor returns a slice of all the SCCs that a given service account can use
-// to run pods in its namespace
-// It expects the serviceAccount name in the system:serviceaccount:<ns>:<name> form
+// to run pods in its namespace.
+// It expects the serviceAccount name in the system:serviceaccount:<ns>:<name> form.
 func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.String, error) {
 	saUserInfo := serviceaccount.UserInfo(
 		serviceAccount.Namespace,
@@ -212,7 +222,6 @@ func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.Stri
 		}
 	}
 
-	// TODO: (idea): determine, ahead of time, which SCCs are allowed for all authenticated or for all SAs?
 	for _, o := range objs {
 		rb, err := newRoleBindingObj(o)
 		if err != nil {
@@ -242,44 +251,63 @@ func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.Stri
 	return allowedSCCs, nil
 }
 
-func (c *SAToSCCCache) GetRoleFromRoleRef(ns string, roleRef rbacv1.RoleRef) (*roleObj, error) {
+// getRoleFromRoleRef tries to retrieve the role or clusterrole from roleRef.
+func (c *SAToSCCCache) getRoleFromRoleRef(ns string, roleRef rbacv1.RoleRef) (*roleObj, error) {
 	var role interface{}
-	var err error
+	var retryErr error
 	switch kind := roleRef.Kind; kind {
-	case "Role": // FIXME: add retries (retry.WithBackoff) on NotFound
-		role, err = c.roleLister.Roles(ns).Get(roleRef.Name)
+	case "Role":
+		retryErr = retry.OnError(retry.DefaultBackoff, apierrors.IsNotFound, func() error {
+			var err error
+			role, err = c.roleLister.Roles(ns).Get(roleRef.Name)
+			return err
+		})
+
 	case "ClusterRole":
-		role, err = c.clusterRoleLister.Get(roleRef.Name)
+		retryErr = retry.OnError(retry.DefaultBackoff, apierrors.IsNotFound, func() error {
+			var err error
+			role, err = c.clusterRoleLister.Get(roleRef.Name)
+			return err
+		})
+
 	default:
 		return nil, fmt.Errorf("unknown kind in roleRef: %s", kind)
 	}
-	if err != nil {
-		return nil, err
+
+	if retryErr != nil {
+		return nil, retryErr
 	}
 
 	return newRoleObj(role)
 }
 
+// IsRoleBindingRelevant returns true if the cluster/rolebinding supplied binds
+// to a Role that provides access to at least one of the SCCs available in the
+// cluster.
 func (c *SAToSCCCache) IsRoleBindingRelevant(obj interface{}) bool {
 	rb, err := newRoleBindingObj(obj)
 	if err != nil {
 		klog.Warningf("unexpected error, this may be a bug: %v", err)
 		return false
 	}
 
-	role, err := c.GetRoleFromRoleRef(rb.Namespace(), rb.RoleRef())
+	role, err := c.getRoleFromRoleRef(rb.Namespace(), rb.RoleRef())
 	if err != nil {
 		klog.Infof("failed to retrieve a role for a rolebinding ref: %v", err)
 		return false
 	}
 
-	// TODO: actually cache the relevant rolebindings and relevant roles
-	// or maybe only the roles and update cached roles on a role update?
 	return c.IsRoleInvolvesSCCs(role, false)
 }
 
+// IsRoleInvolvesSCCs returns true if the role supplied in obj provides access
+// to at least one of the SCCs present in the cluster.
+// Set isRoleUpdate to true if instead of using the cached role the supplied object
+// should be used to update the role in the cache as well.
 func (c *SAToSCCCache) IsRoleInvolvesSCCs(obj interface{}, isRoleUpdate bool) bool {
-	c.usefulRolesLock.Lock() // TODO: comment this stuff
+	// lock the roles cache to make sure the state of the role that's later retrieved here
+	// is written properly
+	c.usefulRolesLock.Lock()
 	defer c.usefulRolesLock.Unlock()
 
 	role, err := newRoleObj(obj)
@@ -288,23 +316,31 @@ func (c *SAToSCCCache) IsRoleInvolvesSCCs(obj interface{}, isRoleUpdate bool) bo
 		return false
 	}
 
-	sccs, err := c.sccLister.List(labels.Everything()) // TODO: this should probably requeue, right?
-	if err != nil {
-		klog.Warning("failed to list SCCs: %v", err)
-		return false
-	}
-
 	if isRoleUpdate {
+		sccs, err := c.sccLister.List(labels.Everything())
+		if err != nil {
+			klog.Warning("failed to list SCCs: %v", err)
+			return false
+		}
+
 		c.syncRoleCache(role.Namespace(), role.Name(), role.Rules(), sccs)
 	}
 
 	return len(c.usefulRoles[fmt.Sprintf("%s/%s", role.Namespace(), role.Name())]) != 0
 }
 
+// ReinitializeRoleCache clears the current cache of roles that provide access
+// to SCCs and reinitializes it by pulling all cluster-/roles and SCCs and
+// reevaluating them.
+// This should be used rather rarely as there are many computations involved
+// when assessing all the present cluster-/role rules.
 func (c *SAToSCCCache) ReinitializeRoleCache() error {
-	c.usefulRolesLock.Lock() // TODO: comment this stuff
+	// rewriting the whole cache, lock is needed
+	c.usefulRolesLock.Lock()
 	defer c.usefulRolesLock.Unlock()
 
+	c.usefulRoles = map[string][]string{}
+
 	roles, err := c.roleLister.List(labels.Everything())
 	if err != nil {
 		return fmt.Errorf("failed to initialize role cache: %w", err)
@@ -331,6 +367,9 @@ func (c *SAToSCCCache) ReinitializeRoleCache() error {
 	return nil
 }
 
+// syncRoleCache will write the current mapping of "role->SCCs it allows" to the cache.
+// It expects the c.usefulRolesLock to be already locked as even the wrapping context
+// handling roles and SCCs likely requires synchronization.
 func (c *SAToSCCCache) syncRoleCache(roleNS, roleName string, rules []rbacv1.PolicyRule, sccs []*securityv1.SecurityContextConstraints) {
 	if c.usefulRolesLock.TryLock() {
 		c.usefulRolesLock.Unlock()
@@ -340,12 +379,12 @@ func (c *SAToSCCCache) syncRoleCache(roleNS, roleName string, rules []rbacv1.Pol
 	dummyUserInfo := &user.DefaultInfo{
 		Name: "dummyUser",
 	}
-	if allowedSCCs := SCCsAllowedByPolicyRules("", dummyUserInfo, sccs, rules); len(allowedSCCs) > 0 {
+	if allowedSCCs := sccsAllowedByPolicyRules("", dummyUserInfo, sccs, rules); len(allowedSCCs) > 0 {
 		c.usefulRoles[fmt.Sprintf("%s/%s", roleNS, roleName)] = allowedSCCs
 	}
 }
 
-func SCCsAllowedByPolicyRules(nsName string, saUserInfo user.Info, sccs []*securityv1.SecurityContextConstraints, rules []rbacv1.PolicyRule) []string {
+func sccsAllowedByPolicyRules(nsName string, saUserInfo user.Info, sccs []*securityv1.SecurityContextConstraints, rules []rbacv1.PolicyRule) []string {
 	ar := authorizer.AttributesRecord{
 		User:            saUserInfo,
 		APIGroup:        securityv1.GroupName,