Merge pull request cri-o#5465 from haircommander/workload-and-runtime

config: merge runtime and workload allowed annotations

pkg/config/config.go

@@ -972,10 +972,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")
@@ -1316,35 +1312,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()

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)
 }

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" {