(bugfix): make k8s 1.25 validation logic check api group before issui…

…ng a warning (openshift#274) * fix a bug in k8s 1.25 validation logic to now check the apiGroup/resource to determine if an api is deprecated. Signed-off-by: Bryce Palmer <bpalmer@redhat.com> * update warning and error checks to use a map Signed-off-by: Bryce Palmer <bpalmer@redhat.com> Signed-off-by: Bryce Palmer <bpalmer@redhat.com> Upstream-repository: api Upstream-commit: f1b729684854a053f229464eb327527222188fd1
tmshort · Apr 6, 2023 · b7ec23d · b7ec23d
1 parent a7c1900
commit b7ec23d
Show file tree

Hide file tree

Showing 8 changed files with 464 additions and 140 deletions.
diff --git a/staging/api/pkg/validation/internal/removed_apis.go b/staging/api/pkg/validation/internal/removed_apis.go
@@ -12,6 +12,7 @@ import (
 	interfaces "github.com/operator-framework/api/pkg/validation/interfaces"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // k8sVersionKey defines the key which can be used by its consumers
@@ -306,51 +307,35 @@ func getRemovedAPIsOn1_25From(bundle *manifests.Bundle) (map[string][]string, ma
 	deprecatedAPIs := make(map[string][]string)
 	warnDeprecatedAPIs := make(map[string][]string)
 
+	deprecatedGvk := map[schema.GroupVersionKind]struct{}{
+		{Group: "batch", Version: "v1beta1", Kind: "CronJob"}:                       {},
+		{Group: "discovery.k8s.io", Version: "v1beta1", Kind: "EndpointSlice"}:      {},
+		{Group: "events.k8s.io", Version: "v1beta1", Kind: "Event"}:                 {},
+		{Group: "autoscaling", Version: "v2beta1", Kind: "HorizontalPodAutoscaler"}: {},
+		{Group: "policy", Version: "v1beta1", Kind: "PodDisruptionBudget"}:          {},
+		{Group: "policy", Version: "v1beta1", Kind: "PodSecurityPolicy"}:            {},
+		{Group: "node.k8s.io", Version: "v1beta1", Kind: "RuntimeClass"}:            {},
+	}
+
 	addIfDeprecated := func(u *unstructured.Unstructured) {
-		switch u.GetAPIVersion() {
-		case "batch/v1beta1":
-			if u.GetKind() == "CronJob" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
-		case "discovery.k8s.io/v1beta1":
-			if u.GetKind() == "EndpointSlice" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
-		case "events.k8s.io/v1beta1":
-			if u.GetKind() == "Event" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
-		case "autoscaling/v2beta1":
-			if u.GetKind() == "HorizontalPodAutoscaler" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
-		case "policy/v1beta1":
-			if u.GetKind() == "PodDisruptionBudget" || u.GetKind() == "PodSecurityPolicy" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
-		case "node.k8s.io/v1beta1":
-			if u.GetKind() == "RuntimeClass" {
-				deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
-			}
+		if _, ok := deprecatedGvk[u.GetObjectKind().GroupVersionKind()]; ok {
+			deprecatedAPIs[u.GetKind()] = append(deprecatedAPIs[u.GetKind()], u.GetName())
 		}
 	}
 
-	warnIfDeprecated := func(res string, msg string) {
-		switch res {
-		case "cronjobs":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "endpointslices":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "events":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "horizontalpodautoscalers":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "poddisruptionbudgets":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "podsecuritypolicies":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
-		case "runtimeclasses":
-			warnDeprecatedAPIs[res] = append(warnDeprecatedAPIs[res], msg)
+	deprecatedGroupResource := map[schema.GroupResource]struct{}{
+		{Group: "batch", Resource: "cronjobs"}:                       {},
+		{Group: "discovery.k8s.io", Resource: "endpointslices"}:      {},
+		{Group: "events.k8s.io", Resource: "events"}:                 {},
+		{Group: "autoscaling", Resource: "horizontalpodautoscalers"}: {},
+		{Group: "policy", Resource: "poddisruptionbudgets"}:          {},
+		{Group: "policy", Resource: "podsecuritypolicies"}:           {},
+		{Group: "node.k8s.io", Resource: "runtimeclasses"}:           {},
+	}
+
+	warnIfDeprecated := func(gr schema.GroupResource, msg string) {
+		if _, ok := deprecatedGroupResource[gr]; ok {
+			warnDeprecatedAPIs[gr.Resource] = append(warnDeprecatedAPIs[gr.Resource], msg)
 		}
 	}
 
@@ -403,11 +388,13 @@ func getRemovedAPIsOn1_25From(bundle *manifests.Bundle) (map[string][]string, ma
 					permCheck := func(permField string, perms []v1alpha1.StrategyDeploymentPermissions) {
 						for i, perm := range perms {
 							for j, rule := range perm.Rules {
-								for _, res := range rule.Resources {
-									if _, ok := resInCsvCrds[res]; ok {
-										continue
+								for _, apiGroup := range rule.APIGroups {
+									for _, res := range rule.Resources {
+										if _, ok := resInCsvCrds[res]; ok {
+											continue
+										}
+										warnIfDeprecated(schema.GroupResource{Group: apiGroup, Resource: res}, fmt.Sprintf("ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.%s[%d].Rules[%d]", permField, i, j))
 									}
-									warnIfDeprecated(res, fmt.Sprintf("ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.%s[%d].Rules[%d]", permField, i, j))
 								}
 							}
 						}

diff --git a/staging/api/pkg/validation/internal/removed_apis_test.go b/staging/api/pkg/validation/internal/removed_apis_test.go
@@ -74,7 +74,12 @@ func Test_GetRemovedAPIsOn1_25From(t *testing.T) {
 
 	warnMock := make(map[string][]string)
 	warnMock["cronjobs"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.ClusterPermissions[0].Rules[7]"}
+	warnMock["endpointslices"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[3]"}
 	warnMock["events"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[2]"}
+	warnMock["horizontalpodautoscalers"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[4]"}
+	warnMock["poddisruptionbudgets"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[5]"}
+	warnMock["podsecuritypolicies"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[5]"}
+	warnMock["runtimeclasses"] = []string{"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[6]"}
 
 	type args struct {
 		bundleDir string
@@ -99,6 +104,14 @@ func Test_GetRemovedAPIsOn1_25From(t *testing.T) {
 				bundleDir: "./testdata/removed_api_1_25",
 			},
 			errWant:  mock,
+			warnWant: map[string][]string{},
+		},
+		{
+			name: "should return warnings with all deprecated APIs in 1.25",
+			args: args{
+				bundleDir: "./testdata/deprecated_api_1_25",
+			},
+			errWant:  mock,
 			warnWant: warnMock,
 		},
 	}
@@ -188,11 +201,7 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.22. " +
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-22. " +
 				"Migrate the API(s) for CRD: ([\"etcdbackups.etcd.database.coreos.com\" " +
-				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])",
-				"this bundle is using APIs which were deprecated and removed in v1.25. " +
-					"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-					"Migrate the API(s) for events: " +
-					"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[1]\"])"},
+				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])"},
 		},
 		{
 			name: "should return an error when the k8sVersion is >= 1.22 and has the deprecated API",
@@ -206,11 +215,6 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-22. " +
 				"Migrate the API(s) for CRD: ([\"etcdbackups.etcd.database.coreos.com\"" +
 				" \"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])"},
-			wantWarning: true,
-			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.25. " +
-				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-				"Migrate the API(s) for events: " +
-				"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[1]\"])"},
 		},
 		{
 			name: "should return an error when the k8sVersion is >= 1.25 and found removed APIs on 1.25",
@@ -224,12 +228,6 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
 				"Migrate the API(s) for HorizontalPodAutoscaler: ([\"memcached-operator-hpa\"])," +
 				"PodDisruptionBudget: ([\"memcached-operator-policy-manager\"]),"},
-			wantWarning: true,
-			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.25. " +
-				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-				"Migrate the API(s) for cronjobs: " +
-				"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.ClusterPermissions[0].Rules[7]\"])" +
-				",events: ([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[2]\"]),"},
 		},
 		{
 			name: "should return a warning if the k8sVersion is empty and found removed APIs on 1.25",
@@ -242,12 +240,7 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.25. " +
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
 				"Migrate the API(s) for HorizontalPodAutoscaler: ([\"memcached-operator-hpa\"])," +
-				"PodDisruptionBudget: ([\"memcached-operator-policy-manager\"]),",
-				"this bundle is using APIs which were deprecated and removed in v1.25. " +
-					"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-					"Migrate the API(s) for cronjobs: " +
-					"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.ClusterPermissions[0].Rules[7]\"])" +
-					",events: ([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[2]\"]),"},
+				"PodDisruptionBudget: ([\"memcached-operator-policy-manager\"]),"},
 		},
 		{
 			name: "should return an error when the k8sVersion is >= 1.26 and found removed APIs on 1.26",
@@ -260,11 +253,6 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 			errStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.26. " +
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26. " +
 				"Migrate the API(s) for HorizontalPodAutoscaler: ([\"memcached-operator-hpa\"])"},
-			wantWarning: true,
-			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.25. " +
-				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-				"Migrate the API(s) for events: " +
-				"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[2]\"])"},
 		},
 		{
 			name: "should return a warning when the k8sVersion is empty and found removed APIs on 1.26",
@@ -274,13 +262,9 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 				directory:      "./testdata/removed_api_1_26",
 			},
 			wantWarning: true,
-			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.25. " +
-				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-				"Migrate the API(s) for events: " +
-				"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[2]\"])",
-				"this bundle is using APIs which were deprecated and removed in v1.26. " +
-					"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26. " +
-					"Migrate the API(s) for HorizontalPodAutoscaler: ([\"memcached-operator-hpa\"])"},
+			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.26. " +
+				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26. " +
+				"Migrate the API(s) for HorizontalPodAutoscaler: ([\"memcached-operator-hpa\"])"},
 		},
 		{
 			name: "should return an error when the k8sVersion informed is invalid",
@@ -295,11 +279,7 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.22. " +
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-22. " +
 				"Migrate the API(s) for CRD: ([\"etcdbackups.etcd.database.coreos.com\" " +
-				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])",
-				"this bundle is using APIs which were deprecated and removed in v1.25. " +
-					"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-					"Migrate the API(s) for events: " +
-					"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[1]\"])"},
+				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])"},
 		},
 		{
 			name: "should return an error when the csv.spec.minKubeVersion informed is invalid",
@@ -314,11 +294,7 @@ func TestValidateDeprecatedAPIS(t *testing.T) {
 			warnStrings: []string{"this bundle is using APIs which were deprecated and removed in v1.22. " +
 				"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-22. " +
 				"Migrate the API(s) for CRD: ([\"etcdbackups.etcd.database.coreos.com\" " +
-				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])",
-				"this bundle is using APIs which were deprecated and removed in v1.25. " +
-					"More info: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-25. " +
-					"Migrate the API(s) for events: " +
-					"([\"ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[1]\"])"},
+				"\"etcdclusters.etcd.database.coreos.com\" \"etcdrestores.etcd.database.coreos.com\"])"},
 		},
 	}
 	for _, tt := range tests {

diff --git a/...pi/pkg/validation/internal/testdata/deprecated_api_1_25/cache.example.com_memcacheds.yaml b/...pi/pkg/validation/internal/testdata/deprecated_api_1_25/cache.example.com_memcacheds.yaml
@@ -0,0 +1,66 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    controller-gen.kubebuilder.io/version: v0.4.1
+  creationTimestamp: null
+  name: memcacheds.cache.example.com
+spec:
+  group: cache.example.com
+  names:
+    kind: Memcached
+    listKind: MemcachedList
+    plural: memcacheds
+    singular: memcached
+  scope: Namespaced
+  versions:
+  - name: v1alpha1
+    schema:
+      openAPIV3Schema:
+        description: Memcached is the Schema for the memcacheds API
+        properties:
+          apiVersion:
+            description: 'APIVersion defines the versioned schema of this representation
+              of an object. Servers should convert recognized schemas to the latest
+              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+            type: string
+          kind:
+            description: 'Kind is a string value representing the REST resource this
+              object represents. Servers may infer this from the endpoint the client
+              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+            type: string
+          metadata:
+            type: object
+          spec:
+            description: MemcachedSpec defines the desired state of Memcached
+            properties:
+              foo:
+                description: Foo is an example field of Memcached. Edit memcached_types.go
+                  to remove/update
+                type: string
+              size:
+                description: Size defines the number of Memcached instances
+                format: int32
+                type: integer
+            type: object
+          status:
+            description: MemcachedStatus defines the observed state of Memcached
+            properties:
+              nodes:
+                description: Nodes store the name of the pods which are running Memcached
+                  instances
+                items:
+                  type: string
+                type: array
+            type: object
+        type: object
+    served: true
+    storage: true
+    subresources:
+      status: {}
+status:
+  acceptedNames:
+    kind: ""
+    plural: ""
+  conditions: []
+  storedVersions: []
diff --git a/staging/api/pkg/validation/internal/testdata/deprecated_api_1_25/horizontal-pod.yaml b/staging/api/pkg/validation/internal/testdata/deprecated_api_1_25/horizontal-pod.yaml
@@ -0,0 +1,18 @@
+apiVersion: autoscaling/v2beta1
+kind: HorizontalPodAutoscaler
+metadata:
+  name: memcached-operator-hpa
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: memcached-operator-controller-manager
+  minReplicas: 1
+  maxReplicas: 10
+  metrics:
+    - type: Resource
+      resource:
+        name: cpu
+        target:
+          type: Utilization
+          averageUtilization: 50