Allow denying special case of empty node selector on namespace

Author: bastjan

config.go

@@ -26,6 +26,10 @@ type Config struct {
 	// AllowedNodeSelectors is a map of allowed node selectors.
 	// Both the key and the value are anchored regexes.
 	AllowedNodeSelectors map[string]string
+
+	// NamespaceDenyEmptyNodeSelector is a flag to enable or disable the rejection of empty node selectors on namespaces.
+	// If true this will reject a { "openshift.io/node-selector": "" } annotation.
+	NamespaceDenyEmptyNodeSelector bool
 }
 
 func ConfigFromFile(path string) (Config, error) {

config.yaml

@@ -16,3 +16,6 @@ PrivilegedClusterRoles:
 # Key and values are anchored regexes
 AllowedNodeSelectors:
   appuio.io/node-class: flex|plus
+# NamespaceDenyEmptyNodeSelector is a flag to enable or disable the rejection of empty node selectors on namespaces.
+# If true this will reject a { "openshift.io/node-selector": "" } annotation.
+NamespaceDenyEmptyNodeSelector: false

main.go

@@ -89,14 +89,7 @@ func main() {
 		PrivilegedGroups:       conf.PrivilegedGroups,
 		PrivilegedClusterRoles: conf.PrivilegedClusterRoles,
 	}
-	ans := &validate.AllowedLabels{}
-	for k, v := range conf.AllowedNodeSelectors {
-		if err := ans.Add(k, v); err != nil {
-			setupLog.Error(err, "unable to add allowed node selector")
-			os.Exit(1)
-		}
-	}
-	registerNodeSelectorValidationWebhooks(mgr, psk, ans)
+	registerNodeSelectorValidationWebhooks(mgr, psk, conf)
 
 	if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {
 		setupLog.Error(err, "unable to setup health endpoint")
@@ -114,17 +107,26 @@ func main() {
 	}
 }
 
-func registerNodeSelectorValidationWebhooks(mgr ctrl.Manager, skipper skipper.Skipper, allowedNodeSelectors *validate.AllowedLabels) {
+func registerNodeSelectorValidationWebhooks(mgr ctrl.Manager, skipper skipper.Skipper, conf Config) {
+	ans := &validate.AllowedLabels{}
+	for k, v := range conf.AllowedNodeSelectors {
+		if err := ans.Add(k, v); err != nil {
+			setupLog.Error(err, "unable to add allowed node selector")
+			os.Exit(1)
+		}
+	}
+
 	mgr.GetWebhookServer().Register("/validate-namespace-node-selector", &webhook.Admission{
 		Handler: &webhooks.NamespaceNodeSelectorValidator{
-			Skipper:              skipper,
-			AllowedNodeSelectors: allowedNodeSelectors,
+			Skipper:               skipper,
+			AllowedNodeSelectors:  ans,
+			DenyEmptyNodeSelector: conf.NamespaceDenyEmptyNodeSelector,
 		},
 	})
 	mgr.GetWebhookServer().Register("/validate-workload-node-selector", &webhook.Admission{
 		Handler: &webhooks.WorkloadNodeSelectorValidator{
 			Skipper:              skipper,
-			AllowedNodeSelectors: allowedNodeSelectors,
+			AllowedNodeSelectors: ans,
 		},
 	})
 }

webhooks/namespace_node_selector_validator.go

@@ -22,6 +22,9 @@ type NamespaceNodeSelectorValidator struct {
 
 	Skipper              skipper.Skipper
 	AllowedNodeSelectors *validate.AllowedLabels
+
+	// DenyEmptyNodeSelector denies namespaces with an existing but empty node selector.
+	DenyEmptyNodeSelector bool
 }
 
 // Handle handles the admission requests
@@ -53,11 +56,15 @@ func (v *NamespaceNodeSelectorValidator) Handle(ctx context.Context, req admissi
 		return admission.Errored(400, err)
 	}
 
-	rawSel := ns.Annotations[OpenshiftNodeSelectorAnnotation]
-	if rawSel == "" {
+	rawSel, exists := ns.Annotations[OpenshiftNodeSelectorAnnotation]
+	if !exists {
 		l.V(1).Info("allowed: no node selector")
 		return admission.Allowed("no node selector")
 	}
+	if rawSel == "" && v.DenyEmptyNodeSelector {
+		l.V(1).Info("denied: empty node selector")
+		return admission.Denied("empty `\"\"` node selector")
+	}
 
 	sel, err := labels.ConvertSelectorToLabelsMap(rawSel)
 	if err != nil {

webhooks/namespace_node_selector_validator_test.go

@@ -46,6 +46,33 @@ func Test_NamespaceNodeSelectorValidator_Handle(t *testing.T) {
 	}
 }
 
+func Test_NamespaceNodeSelectorValidator_DenyEmpty(t *testing.T) {
+	allowed := &validate.AllowedLabels{}
+	require.NoError(t, allowed.Add("appuio.io/node-class", "flex|plus"))
+
+	subject := NamespaceNodeSelectorValidator{
+		AllowedNodeSelectors: allowed,
+		Skipper:              skipper.StaticSkipper{},
+	}
+	require.NoError(t, subject.InjectDecoder(decoder(t)))
+
+	empty := map[string]string{OpenshiftNodeSelectorAnnotation: ""}
+
+	t.Run("Allow", func(t *testing.T) {
+		subject.DenyEmptyNodeSelector = false
+		resp := subject.Handle(context.Background(), admissionRequestForObject(t, newNamespace("test", nil, empty)))
+		t.Log("Response:", resp.Result.Reason, resp.Result.Message)
+		require.True(t, resp.Allowed)
+	})
+
+	t.Run("Deny", func(t *testing.T) {
+		subject.DenyEmptyNodeSelector = true
+		resp := subject.Handle(context.Background(), admissionRequestForObject(t, newNamespace("test", nil, empty)))
+		t.Log("Response:", resp.Result.Reason, resp.Result.Message)
+		require.False(t, resp.Allowed)
+	})
+}
+
 func newNamespace(name string, labels, annotations map[string]string) *corev1.Namespace {
 	return &corev1.Namespace{
 		TypeMeta: metav1.TypeMeta{