Merge pull request kubernetes-sigs#3040 from jon-fearer/fix-encryptio…

…n-config Fix EKS encryption config value comparisons
databricks · Dec 21, 2021 · 27b09a7 · 27b09a7
2 parents 7ef1746 + fcdc2ba
commit 27b09a7
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 7 deletions.
diff --git a/controlplane/eks/api/v1beta1/awsmanagedcontrolplane_webhook.go b/controlplane/eks/api/v1beta1/awsmanagedcontrolplane_webhook.go
@@ -139,7 +139,11 @@ func (r *AWSManagedControlPlane) ValidateUpdate(old runtime.Object) error {
 	}
 
 	// If encryptionConfig is already set, do not allow change in provider
-	if r.Spec.EncryptionConfig != nil && oldAWSManagedControlplane.Spec.EncryptionConfig != nil && *r.Spec.EncryptionConfig.Provider != *oldAWSManagedControlplane.Spec.EncryptionConfig.Provider {
+	if r.Spec.EncryptionConfig != nil &&
+		r.Spec.EncryptionConfig.Provider != nil &&
+		oldAWSManagedControlplane.Spec.EncryptionConfig != nil &&
+		oldAWSManagedControlplane.Spec.EncryptionConfig.Provider != nil &&
+		*r.Spec.EncryptionConfig.Provider != *oldAWSManagedControlplane.Spec.EncryptionConfig.Provider {
 		allErrs = append(allErrs,
 			field.Invalid(field.NewPath("spec", "encryptionConfig", "provider"), r.Spec.EncryptionConfig.Provider, "changing EKS encryption is not allowed after it has been enabled"),
 		)

diff --git a/pkg/cloud/services/eks/cluster.go b/pkg/cloud/services/eks/cluster.go
@@ -707,12 +707,19 @@ func compareEncryptionConfig(updatedEncryptionConfig, existingEncryptionConfig [
 		return false
 	}
 	for index, encryptionConfig := range updatedEncryptionConfig {
-		if encryptionConfig.Provider != existingEncryptionConfig[index].Provider {
+		if getKeyArn(encryptionConfig) != getKeyArn(existingEncryptionConfig[index]) {
 			return false
 		}
-		if cmp.Equals(encryptionConfig.Resources, existingEncryptionConfig[index].Resources) {
+		if !cmp.Equals(encryptionConfig.Resources, existingEncryptionConfig[index].Resources) {
 			return false
 		}
 	}
 	return true
 }
+
+func getKeyArn(encryptionConfig *eks.EncryptionConfig) string {
+	if encryptionConfig.Provider != nil {
+		return aws.StringValue(encryptionConfig.Provider.KeyArn)
+	}
+	return ""
+}
diff --git a/pkg/cloud/services/eks/cluster_test.go b/pkg/cloud/services/eks/cluster_test.go
@@ -441,12 +441,25 @@ func TestReconcileEKSEncryptionConfig(t *testing.T) {
 		expectError         bool
 	}{
 		{
-			name:                "no upgrade necessary",
+			name:                "no upgrade necessary - encryption disabled",
 			oldEncryptionConfig: &ekscontrolplanev1.EncryptionConfig{},
 			newEncryptionConfig: &ekscontrolplanev1.EncryptionConfig{},
 			expect:              func(m *mock_eksiface.MockEKSAPIMockRecorder) {},
 			expectError:         false,
 		},
+		{
+			name: "no upgrade necessary - encryption config unchanged",
+			oldEncryptionConfig: &ekscontrolplanev1.EncryptionConfig{
+				Provider:  pointer.String("provider"),
+				Resources: []*string{pointer.String("foo"), pointer.String("bar")},
+			},
+			newEncryptionConfig: &ekscontrolplanev1.EncryptionConfig{
+				Provider:  pointer.String("provider"),
+				Resources: []*string{pointer.String("foo"), pointer.String("bar")},
+			},
+			expect:      func(m *mock_eksiface.MockEKSAPIMockRecorder) {},
+			expectError: false,
+		},
 		{
 			name:                "needs upgrade",
 			oldEncryptionConfig: nil,

diff --git a/pkg/internal/cmp/slice.go b/pkg/internal/cmp/slice.go
@@ -42,7 +42,7 @@ func Equals(slice1, slice2 []*string) bool {
 
 	if len(slice1) == len(slice2) {
 		for i, v := range slice1 {
-			if pointer.StringEqual(v, slice2[i]) {
+			if !pointer.StringEqual(v, slice2[i]) {
 				return false
 			}
 		}

diff --git a/pkg/internal/cmp/slice_test.go b/pkg/internal/cmp/slice_test.go
@@ -30,9 +30,9 @@ func TestCompareSlices(t *testing.T) {
 	slice2 := []*string{pointer.String("bar"), pointer.String("foo")}
 
 	expected := Equals(slice1, slice2)
-	g.Expect(expected, true)
+	g.Expect(expected).To(BeTrue())
 
 	slice2 = append(slice2, pointer.String("test"))
 	expected = Equals(slice1, slice2)
-	g.Expect(expected, false)
+	g.Expect(expected).To(BeFalse())
 }