From 5fb7618d5b69ef48b932fcaa50adc66433db64fd Mon Sep 17 00:00:00 2001
From: Peter Hunt <pehunt@redhat.com>
Date: Tue, 16 Nov 2021 11:39:18 -0500
Subject: [PATCH] config: merge runtime and workload allowed annotations

to allow for a more smooth transition between workloads and runtime class level allowed annotations

Signed-off-by: Peter Hunt <pehunt@redhat.com>
---
 pkg/config/config.go | 33 ---------------------------------
 server/utils.go      |  8 +++-----
 test/workloads.bats  | 17 +++++++++++++----
 3 files changed, 16 insertions(+), 42 deletions(-)

diff --git a/pkg/config/config.go b/pkg/config/config.go
index ebd80261920..8907196fb05 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -961,10 +961,6 @@ func (c *RuntimeConfig) Validate(systemContext *types.SystemContext, onExecution
 			return errors.Wrap(err, "runtime validation")
 		}
 
-		if err := c.ValidateAllowedAnnotations(); err != nil {
-			return errors.Wrap(err, "allowed annotations validation")
-		}
-
 		// Validate the system registries configuration
 		if _, err := sysregistriesv2.GetRegistries(systemContext); err != nil {
 			return errors.Wrap(err, "invalid registries")
@@ -1305,35 +1301,6 @@ func validateAllowedAndGenerateDisallowedAnnotations(allowed []string) (disallow
 	return disallowed, nil
 }
 
-// In the interim between adding workload level allowed annotations
-// and disabling runtime level allowed annotations, we need to do a separate
-// validation step to ensure neither list are stepping on the other's toes.
-// Instead of complicated logic, declare workload level allowed annotations to
-// always overwrite runtime level ones.
-func (c *RuntimeConfig) ValidateAllowedAnnotations() error {
-	var workloadHasAnnotation bool
-	for _, wl := range c.Workloads {
-		if len(wl.AllowedAnnotations) != 0 {
-			workloadHasAnnotation = true
-		}
-	}
-	if !workloadHasAnnotation {
-		for _, wl := range c.Workloads {
-			wl.AllowedAnnotations = []string{}
-			wl.DisallowedAnnotations = []string{}
-		}
-		logrus.Infof("Workload does not have an allowed annotation configured. Clearing allowed annotations from runtimes")
-		return nil
-	}
-	logrus.Infof("Workload has an allowed annotation configured. Clearing allowed annotations from runtimes")
-	for name, rh := range c.Runtimes {
-		logrus.Infof("Clearing allowed annotations from %s", name)
-		rh.AllowedAnnotations = []string{}
-		rh.DisallowedAnnotations = []string{}
-	}
-	return nil
-}
-
 // CNIPlugin returns the network configuration CNI plugin
 func (c *NetworkConfig) CNIPlugin() ocicni.CNIPlugin {
 	return c.cniManager.Plugin()
diff --git a/server/utils.go b/server/utils.go
index d5cc40170c5..90187148f14 100644
--- a/server/utils.go
+++ b/server/utils.go
@@ -221,14 +221,12 @@ func (s *Server) FilterDisallowedAnnotations(toFind, toFilter map[string]string,
 	// When runtime level allowed annotations are deprecated, this will be dropped.
 	// TODO: eventually, this should be in the container package, but it's going through a lot of churn
 	// and SpecAddAnnotations is already passed too many arguments
-	rtAllowed, err := s.Runtime().AllowedAnnotations(runtimeHandler)
+	allowed, err := s.Runtime().AllowedAnnotations(runtimeHandler)
 	if err != nil {
 		return err
 	}
-	allowed := s.config.Workloads.AllowedAnnotations(toFind)
-	if len(allowed) == 0 {
-		allowed = rtAllowed
-	}
+	allowed = append(allowed, s.config.Workloads.AllowedAnnotations(toFind)...)
+	logrus.Warnf("Allowed annotations are specified for workload %v %s", allowed, runtimeHandler)
 
 	return s.config.Workloads.FilterDisallowedAnnotations(allowed, toFilter)
 }
diff --git a/test/workloads.bats b/test/workloads.bats
index 8ed489fc5c8..d116b43a126 100644
--- a/test/workloads.bats
+++ b/test/workloads.bats
@@ -293,20 +293,29 @@ function check_conmon_fields() {
 	[[ "$df" != *'16384'* ]]
 }
 
-@test "test workload allowed annotation overrides runtime" {
-	create_workload_with_allowed_annotation "io.kubernetes.cri-o.userns-mode"
+@test "test workload allowed annotation appended with runtime" {
+	if test -n "$CONTAINER_UID_MAPPINGS"; then
+		skip "userNS enabled"
+	fi
+	create_workload_with_allowed_annotation "io.kubernetes.cri-o.Devices"
 	create_runtime_with_allowed_annotation "shmsize" "io.kubernetes.cri-o.ShmSize"
 
 	start_crio
 
-	jq '.annotations."io.kubernetes.cri-o.ShmSize" = "16Mi"' \
+	jq --arg act "$activation" \
+		'   .annotations[$act] = "true"
+	    |   .annotations."io.kubernetes.cri-o.ShmSize" = "16Mi"
+	    |   .annotations."io.kubernetes.cri-o.Devices" = "/dev/null:/dev/peterfoo:rwm"' \
 		"$TESTDATA"/sandbox_config.json > "$sboxconfig"
 
 	ctrconfig="$TESTDATA"/container_sleep.json
 	ctr_id=$(crictl run "$ctrconfig" "$sboxconfig")
 
 	df=$(crictl exec --sync "$ctr_id" df | grep /dev/shm)
-	[[ "$df" != *'16384'* ]]
+	[[ "$df" == *'16384'* ]]
+
+	crictl exec --sync "$ctr_id" sh -c "head -n1 /dev/peterfoo"
+
 }
 
 @test "test workload allowed annotation works for pod" {