Fix issues with drift detection for nil values & handling of kubernet…

…es control fields (status, generation etc)
gavinbunney · Oct 10, 2021 · 4c0b8e6 · 4c0b8e6
1 parent c79527a
commit 4c0b8e6
Show file tree

Hide file tree

Showing 4 changed files with 385 additions and 45 deletions.
diff --git a/flatten/flatten.go b/flatten/flatten.go
@@ -5,11 +5,11 @@ import (
 	"reflect"
 )
 
+// Flatten takes a structure and turns into a flat map[string]string.
+//
 // Based on the Terraform implementation at https://github.com/hashicorp/terraform/blob/master/flatmap/flatten.go
 // Added more generic handling for different types
 //
-// Flatten takes a structure and turns into a flat map[string]string.
-//
 // Within the "thing" parameter, only primitive values are allowed. Structs are
 // not supported. Therefore, it can only be slices, maps, primitives, and
 // any combination of those together.
@@ -19,6 +19,16 @@ func Flatten(thing map[string]interface{}) map[string]string {
 	result := make(map[string]string)
 
 	for k, raw := range thing {
+		// when the raw value is nil, skip it to treat it like an empty map/string, i.e. it's not kept in the result map
+		if raw == nil {
+			continue
+		}
+
+		// ignore any keys which are empty strings, as there isn't anything reference it in the resultant map
+		if k == "" {
+			continue
+		}
+
 		flatten(result, k, reflect.ValueOf(raw))
 	}
 

diff --git a/flatten/flatten_test.go b/flatten/flatten_test.go
@@ -0,0 +1,122 @@
+package flatten
+
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func TestFlattenMap(t *testing.T) {
+	testCases := []struct {
+		description string
+		test        map[string]interface{}
+		expected    map[string]string
+	}{
+		{
+			description: "Simple map with string value",
+			test: map[string]interface{}{
+				"test1": "test2",
+			},
+			expected: map[string]string{
+				"test1": "test2",
+			},
+		},
+		{
+			description: "All primitive types",
+			test: map[string]interface{}{
+				"a_string":  "test2",
+				"a_boolean": true,
+				"a_number":  123,
+				"a_float":   123.456,
+			},
+			expected: map[string]string{
+				"a_boolean": "true",
+				"a_float":   "123.456",
+				"a_number":  "123",
+				"a_string":  "test2",
+			},
+		},
+		{
+			description: "Map with empty keys",
+			test: map[string]interface{}{
+				"": "",
+			},
+			expected: map[string]string{},
+		},
+		{
+			description: "Empty map",
+			test:        map[string]interface{}{},
+			expected:    map[string]string{},
+		},
+		{
+			description: "Nil map",
+			test:        nil,
+			expected:    map[string]string{},
+		},
+		{
+			description: "One level map",
+			test: map[string]interface{}{
+				"atest": "test",
+				"meta": map[string]interface{}{
+					"annotations": map[string]string{
+						"helm.sh/hook": "crd-install",
+					},
+				},
+			},
+			expected: map[string]string{
+				"atest":                         "test",
+				"meta.annotations.helm.sh/hook": "crd-install",
+			},
+		},
+		{
+			description: "One level map empty value",
+			test: map[string]interface{}{
+				"atest": "test",
+				"meta":  map[string]interface{}{},
+			},
+			expected: map[string]string{
+				"atest": "test",
+			},
+		},
+		{
+			description: "One level map, nil value",
+			test: map[string]interface{}{
+				"atest": "test",
+				"meta":  nil,
+			},
+			expected: map[string]string{
+				"atest": "test",
+			},
+		},
+		{
+			description: "One level slice",
+			test: map[string]interface{}{
+				"my-slice": []string{"first", "second"},
+			},
+			expected: map[string]string{
+				"my-slice.#": "2",
+				"my-slice.0": "first",
+				"my-slice.1": "second",
+			},
+		},
+		{
+			description: "Map with slice elements",
+			test: map[string]interface{}{
+				"meta": map[string]interface{}{
+					"my-slice": []string{"first", "second"},
+				},
+			},
+			expected: map[string]string{
+				"meta.my-slice.#": "2",
+				"meta.my-slice.0": "first",
+				"meta.my-slice.1": "second",
+			},
+		},
+	}
+
+	for _, tcase := range testCases {
+		t.Run(tcase.description, func(t *testing.T) {
+			result := Flatten(tcase.test)
+			assert.Equal(t, tcase.expected, result)
+		})
+	}
+}
diff --git a/kubernetes/resource_kubectl_manifest.go b/kubernetes/resource_kubectl_manifest.go
@@ -179,15 +179,7 @@ metadata:
 				_ = d.Set("yaml_incluster", liveManifestFingerprint)
 				_ = d.Set("live_manifest_incluster", liveManifestFingerprint)
 
-				// get selfLink or generate (for Kubernetes 1.20+)
-				selfLink := metaObjLive.GetSelfLink()
-				if len(selfLink) == 0 {
-					selfLink = generateSelfLink(
-						metaObjLive.GetAPIVersion(),
-						metaObjLive.GetNamespace(),
-						metaObjLive.GetKind(),
-						metaObjLive.GetName())
-				}
+				selfLink := getSelfLink(metaObjLive)
 
 				// set fields captured normally during creation/updates
 				d.SetId(selfLink)
@@ -537,15 +529,7 @@ func resourceKubectlManifestApply(ctx context.Context, d *schema.ResourceData, m
 		return fmt.Errorf("%v failed to fetch resource from kubernetes: %+v", manifest, err)
 	}
 
-	// get selfLink or generate (for Kubernetes 1.20+)
-	selfLink := response.GetSelfLink()
-	if len(selfLink) == 0 {
-		selfLink = generateSelfLink(
-			response.GetAPIVersion(),
-			response.GetNamespace(),
-			response.GetKind(),
-			response.GetName())
-	}
+	selfLink := getSelfLink(response)
 
 	d.SetId(selfLink)
 	log.Printf("[DEBUG] %v fetched successfully, set id to: %v", manifest, d.Id())
@@ -936,11 +920,17 @@ func getFingerprint(s string) string {
 
 func getLiveManifestFields_WithIgnoredFields(ignoredFields []string, userProvided *meta_v1_unstruct.Unstructured, liveManifest *meta_v1_unstruct.Unstructured) string {
 
+	selfLink := getSelfLink(userProvided)
 	flattenedUser := flatten.Flatten(userProvided.Object)
 	flattenedLive := flatten.Flatten(liveManifest.Object)
 
-	// remove any fields from the user provided set that we want to ignore
-	for _, field := range ignoredFields {
+	// remove any fields from the user provided set or control fields that we want to ignore
+	fieldsToTrim := append([]string(nil), kubernetesControlFields...)
+	if len(ignoredFields) > 0 {
+		fieldsToTrim = append(fieldsToTrim, ignoredFields...)
+	}
+
+	for _, field := range fieldsToTrim {
 		delete(flattenedUser, field)
 
 		// check for any nested fields to ignore
@@ -955,23 +945,17 @@ func getLiveManifestFields_WithIgnoredFields(ignoredFields []string, userProvide
 	// this implicitly excludes anything that the user didn't provide as it was added by kubernetes runtime (annotations/mutations etc)
 	userKeys := []string{}
 	for userKey, userValue := range flattenedUser {
-
-		// ignore any skipping fields that don't make sense for users to update
-		if _, exists := kubernetesControlFields[userKey]; exists {
-			continue
-		}
-
 		// only include the value if it exists in the live version
 		// that is, don't add to the userKeys array unless the key still exists in the live manifest
 		if _, exists := flattenedLive[userKey]; exists {
 			userKeys = append(userKeys, userKey)
 			flattenedUser[userKey] = strings.TrimSpace(flattenedLive[userKey])
 			if strings.TrimSpace(userValue) != flattenedUser[userKey] {
-				log.Printf("[TRACE] yaml drift detected for %s, was:\n%s now:\n%s", userKey, userValue, flattenedLive[userKey])
+				log.Printf("[TRACE] yaml drift detected in %s for %s, was:\n%s now:\n%s", selfLink, userKey, userValue, flattenedLive[userKey])
 			}
 		} else {
 			if strings.TrimSpace(userValue) != "" {
-				log.Printf("[TRACE] yaml drift detected for %s, was %s now blank", userKey, userValue)
+				log.Printf("[TRACE] yaml drift detected in %s for %s, was %s now blank", selfLink, userKey, userValue)
 			}
 		}
 	}
@@ -985,15 +969,25 @@ func getLiveManifestFields_WithIgnoredFields(ignoredFields []string, userProvide
 	return strings.Join(returnedValues, ",")
 }
 
-var kubernetesControlFields = map[string]bool{
-	"status":            true,
-	"finalizers":        true,
-	"initializers":      true,
-	"ownerReferences":   true,
-	"creationTimestamp": true,
-	"generation":        true,
-	"resourceVersion":   true,
-	"uid":               true,
+var kubernetesControlFields = []string{
+	"status",
+	"metadata.finalizers",
+	"metadata.initializers",
+	"metadata.ownerReferences",
+	"metadata.creationTimestamp",
+	"metadata.generation",
+	"metadata.resourceVersion",
+	"metadata.uid",
+	"metadata.annotations.kubectl.kubernetes.io/last-applied-configuration",
+}
+
+func getSelfLink(manifest *meta_v1_unstruct.Unstructured) string {
+	selfLink := manifest.GetSelfLink()
+	if len(selfLink) > 0 {
+		return selfLink
+	}
+
+	return generateSelfLink(manifest.GetAPIVersion(), manifest.GetNamespace(), manifest.GetKind(), manifest.GetName())
 }
 
 // generateSelfLink creates a selfLink of the form: