Put back autofix logic for ClusterRole and Role (#219)

* Put back autofix logic for ClusterRole and Role * Rename mapPairMatch to equalValueForKey
Shopify · Jul 23, 2019 · 9c1546f · 9c1546f
1 parent 650cc1b
commit 9c1546f
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 36 deletions.
diff --git a/cmd/autofix_test.go b/cmd/autofix_test.go
@@ -486,7 +486,7 @@ func TestFindItemInMapSlice(t *testing.T) {
 	assert.Equal(-1, index)
 }
 
-func TestMapPairMatch(t *testing.T) {
+func TestEqualValueForKey(t *testing.T) {
 	assert := assert.New(t)
 
 	// same string
@@ -495,17 +495,17 @@ func TestMapPairMatch(t *testing.T) {
 		{Key: "k", Value: "v"},
 		{Key: "k2", Value: "v2"},
 	}
-	assert.True(mapPairMatch("k2", m1, m2))
+	assert.True(equalValueForKey("k2", m1, m2))
 
 	// different string
 	m1 = yaml.MapSlice{{Key: "k", Value: "v"}}
 	m2 = yaml.MapSlice{{Key: "k", Value: "v2"}}
-	assert.False(mapPairMatch("k2", m1, m2))
+	assert.False(equalValueForKey("k2", m1, m2))
 
 	// string and number
 	m1 = yaml.MapSlice{{Key: "k", Value: "2"}}
 	m2 = yaml.MapSlice{{Key: "k", Value: 2}}
-	assert.False(mapPairMatch("k", m1, m2))
+	assert.False(equalValueForKey("k", m1, m2))
 }
 
 func TestSequenceItemMatch(t *testing.T) {

diff --git a/cmd/autofix_util.go b/cmd/autofix_util.go
@@ -241,8 +241,8 @@ func isCommentSlice(b []byte) bool {
 	return true
 }
 
-// mapPairMatch returns true if map1 and map2 have the same key-value pair for the given key
-func mapPairMatch(key string, map1, map2 yaml.MapSlice) bool {
+// equalValueForKey returns true if map1 and map2 have the same key-value pair for the given key
+func equalValueForKey(key string, map1, map2 yaml.MapSlice) bool {
 	if item1, index1 := findItemInMapSlice(key, map1); index1 != -1 {
 		if item2, index2 := findItemInMapSlice(key, map2); index2 != -1 {
 			return deepEqual(item1.Value, item2.Value)
@@ -465,31 +465,31 @@ func sequenceItemMatch(sequenceKey string, item1, item2 yaml.SequenceItem) bool
 	// EndpointSubset.addresses : EndpointAddress.[hostname OR ip]
 	// EndpointSubset.notReadyAddresses : EndpointAddress.[hostname OR ip]
 	case "addresses", "notReadyAddresses":
-		if mapPairMatch("hostname", map1, map2) {
+		if equalValueForKey("hostname", map1, map2) {
 			return true
 		}
-		return mapPairMatch("ip", map1, map2)
+		return equalValueForKey("ip", map1, map2)
 
 	// Container.envFrom : EnvFromSource.[configMapRef OR secretRef].name
 	case "envFrom":
 		if val1, index1 := findItemInMapSlice("configMapRef", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("configMapRef", map2); index2 != -1 {
-				return mapPairMatch("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 		}
 		if val1, index1 := findItemInMapSlice("secretRef", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("secretRef", map2); index2 != -1 {
-				return mapPairMatch("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 		}
 		return false
 
 	// NetworkPolicySpec.ingress : NetworkPolicyIngressRule.[ports OR from]
 	case "ingress":
-		if mapPairMatch("ports", map1, map2) {
+		if equalValueForKey("ports", map1, map2) {
 			return true
 		}
-		return mapPairMatch("from", map1, map2)
+		return equalValueForKey("from", map1, map2)
 
 	// ConfigMapProjection.items : KeyToPath.key
 	// ConfigMapVolumeSource.items : KeyToPath.key
@@ -499,29 +499,29 @@ func sequenceItemMatch(sequenceKey string, item1, item2 yaml.SequenceItem) bool
 	case "items":
 		// ConfigMapVolumeSource.items : KeyToPath.key
 		// SecretVolumeSource.items : KeyToPath.key
-		if mapPairMatch("key", map1, map2) {
+		if equalValueForKey("key", map1, map2) {
 			return true
 		}
 		// DownwardAPIVolumeSource.items : DownwardAPIVolumeFile.path
-		return mapPairMatch("path", map1, map2)
+		return equalValueForKey("path", map1, map2)
 
 	// NodeSelector.nodeSelectorTerms : NodeSelectorTerm.[matchExpressions OR matchFields]
 	case "nodeSelectorTerms":
 		// This is a bit of a complicated case.
 		// See https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.13/#nodeselector-v1-core
 		// For now, only match if there is an exact match for the complex value of either the "matchExpressions" or
 		// "matchFields" fields.
-		if mapPairMatch("matchExpressions", map1, map2) {
+		if equalValueForKey("matchExpressions", map1, map2) {
 			return true
 		}
-		return mapPairMatch("matchFields", map1, map2)
+		return equalValueForKey("matchFields", map1, map2)
 
 	// ObjectMeta.ownerReferences : OwnerReference.[uid OR name]
 	case "ownerReferences":
-		if mapPairMatch("uid", map1, map2) {
+		if equalValueForKey("uid", map1, map2) {
 			return true
 		}
-		return mapPairMatch("name", map1, map2)
+		return equalValueForKey("name", map1, map2)
 
 	// NodeAffinity.preferredDuringSchedulingIgnoredDuringExecution : PreferredSchedulingTerm.preference
 	// PodAffinity.preferredDuringSchedulingIgnoredDuringExecution : WeightedPodAffinityTerm.podAffinityTerm
@@ -536,87 +536,90 @@ func sequenceItemMatch(sequenceKey string, item1, item2 yaml.SequenceItem) bool
 		// The value for the "weight" field can be updated.
 
 		// NodeAffinity.preferredDuringSchedulingIgnoredDuringExecution : PreferredSchedulingTerm.preference
-		if mapPairMatch("preference", map1, map2) {
+		if equalValueForKey("preference", map1, map2) {
 			return true
 		}
 		// PodAffinity.preferredDuringSchedulingIgnoredDuringExecution : WeightedPodAffinityTerm.podAffinityTerm
 		// PodAntiAffinity.preferredDuringSchedulingIgnoredDuringExecution : WeightedPodAffinityTerm.podAffinityTerm
-		return mapPairMatch("podAffinityTerm", map1, map2)
+		return equalValueForKey("podAffinityTerm", map1, map2)
 
 	// Container.ports : ContainerPort.containerPort
 	// EndpointSubset.ports : EndpointPort.port
 	// ServiceSpec.ports : ServicePort.port
 	case "ports":
 		// Container.ports : ContainerPort.containerPort
-		if mapPairMatch("containerPort", map1, map2) {
+		if equalValueForKey("containerPort", map1, map2) {
 			return true
 		}
 		// EndpointSubset.ports : EndpointPort.port
 		// ServiceSpec.ports : ServicePort.port
-		return mapPairMatch("port", map1, map2)
+		return equalValueForKey("port", map1, map2)
 
 	// ClusterRole.rules : PolicyRule.resources
 	// IngressSpec.rules : IngressRule.host
-	// IngressSpec.rules : IngressRule.http
 	// Role.rules : PolicyRule.resources
 	case "rules":
-		if val1, index1 := findItemInMapSlice("host", map1); index1 != -1 {
-			if val2, index2 := findItemInMapSlice("host", map2); index2 != -1 {
-				return val1.Value == val2.Value
-			}
-			return false
+		// ClusterRole.rules : PolicyRule.resources
+		// Role.rules : PolicyRule.resources
+		if equalValueForKey("resources", map1, map2) {
+			return true
+		}
+
+		// IngressSpec.rules : IngressRule.host
+		if equalValueForKey("host", map1, map2) {
+			return true
 		}
 
 	// ProjectedVolumeSource.sources
 	case "sources":
 		// ProjectedVolumeSource.sources : VolumeProjection.configMap.name
 		if val1, index1 := findItemInMapSlice("configMap", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("configMap", map2); index2 != -1 {
-				return mapPairMatch("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 			return false
 		}
 		// ProjectedVolumeSource.sources : VolumeProjection.downwardAPI.items
 		if val1, index1 := findItemInMapSlice("downwardAPI", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("downwardAPI", map2); index2 != -1 {
-				return mapPairMatch("items", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("items", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 			return false
 		}
 		// ProjectedVolumeSource.sources : VolumeProjection.secret.name
 		if val1, index1 := findItemInMapSlice("secret", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("secret", map2); index2 != -1 {
-				return mapPairMatch("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 			return false
 		}
 		// ProjectedVolumeSource.sources : VolumeProjection.serviceAccountToken.name
 		if val1, index1 := findItemInMapSlice("serviceAccountToken", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("serviceAccountToken", map2); index2 != -1 {
-				return mapPairMatch("path", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("path", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 		}
 		return false
 
 	// IngressSpec.tls : IngressTLS.[secretName OR hosts]
 	case "tls":
-		if mapPairMatch("secretName", map1, map2) {
+		if equalValueForKey("secretName", map1, map2) {
 			return true
 		}
-		return mapPairMatch("hosts", map1, map2)
+		return equalValueForKey("hosts", map1, map2)
 
 	// StatefulSetSpec.volumeClaimTemplates : PersistentVolumeClaim.metadata.name
 	case "volumeClaimTemplates":
 		if val1, index1 := findItemInMapSlice("metadata", map1); index1 != -1 {
 			if val2, index2 := findItemInMapSlice("metadata", map2); index2 != -1 {
-				return mapPairMatch("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
+				return equalValueForKey("name", val1.Value.(yaml.MapSlice), val2.Value.(yaml.MapSlice))
 			}
 		}
 		return false
 	}
 
 	if idKey, ok := identifyingKey[sequenceKey]; ok {
-		return mapPairMatch(idKey, map1, map2)
+		return equalValueForKey(idKey, map1, map2)
 	}
 
 	// FSGroupStrategyOptions.ranges : IDRange

diff --git a/fixtures/autofix-all-resources-fixed_v1.yml b/fixtures/autofix-all-resources-fixed_v1.yml
@@ -296,6 +296,39 @@ spec:
   selector: null
 status: {}
 ---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  # "namespace" omitted since ClusterRoles are not namespaced
+  name: secret-reader
+rules:
+- apiGroups:
+  - ""
+  # 1
+  resources:
+  - secrets
+  # 2
+  verbs:
+  - get
+  - watch
+  - list
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  namespace: default
+  name: pod-reader
+rules:
+# "" indicates the core API group
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - get
+  - watch
+  - list
+---
 apiVersion: networking.k8s.io/v1
 kind: NetworkPolicy
 metadata:

diff --git a/fixtures/autofix-all-resources_v1.yml b/fixtures/autofix-all-resources_v1.yml
@@ -223,3 +223,23 @@ spec:
       automountServiceAccountToken: false
   selector: null
 status: {}
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  # "namespace" omitted since ClusterRoles are not namespaced
+  name: secret-reader
+rules:
+- apiGroups: [""]
+  resources: ["secrets"] # 1
+  verbs: ["get", "watch", "list"] # 2
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  namespace: default
+  name: pod-reader
+rules:
+- apiGroups: [""] # "" indicates the core API group
+  resources: ["pods"]
+  verbs: ["get", "watch", "list"]