Fix normalizing fields with empty objects/slices (#2576)

### Proposed changes This PR further improves the resource normalization logic. Prior to this merge, field values that consists of an empty `map[string]interface{}` or empty slice would be discarded. This means that normalizing `{parentField: {childField: {} }}` would result in the childField being unset as the empty object (`{}`) value for `childField` is discarded. In Kubernetes resources, especially when defining NetworkPolicies and ingress/egress rules, `{}` != unset field. A new test case is added to validate that normalizing NetworkPolicy resources does not result in unwanted behaviour. To do this, the GKE cluster we spin up for testing also enables the Calico network enforcement. ### Related issues (optional) Fixes: #2538
pulumi · Sep 28, 2023 · 8a67a13 · 8a67a13
1 parent b13fe02
commit 8a67a13
Show file tree

Hide file tree

Showing 13 changed files with 462 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Unreleased
 
+- Fix normalizing fields with empty objects/slices (https://github.com/pulumi/pulumi-kubernetes/pull/2576)
+
 ## 4.3.0 (September 25, 2023)
 
 - helm.v3.Release: Detect changes to local charts (https://github.com/pulumi/pulumi-kubernetes/pull/2568)

diff --git a/provider/pkg/clients/unstructured.go b/provider/pkg/clients/unstructured.go
@@ -68,11 +68,12 @@ func ToUnstructured(object metav1.Object) (*unstructured.Unstructured, error) {
 func Normalize(uns *unstructured.Unstructured) (*unstructured.Unstructured, error) {
 	var result *unstructured.Unstructured
 
-	if IsCRD(uns) {
+	switch {
+	case IsCRD(uns):
 		result = normalizeCRD(uns)
-	} else if IsSecret(uns) {
+	case IsSecret(uns):
 		result = normalizeSecret(uns)
-	} else {
+	default:
 		obj, err := FromUnstructured(uns)
 		// Return the input resource rather than an error if this operation fails.
 		if err != nil {

diff --git a/provider/pkg/provider/util.go b/provider/pkg/provider/util.go
@@ -139,6 +139,11 @@ func getActiveClusterFromConfig(config *clientapi.Config, overrides resource.Pro
 // pruneMap builds a pruned map by recursively copying elements from the source map that have a matching key in the
 // target map. This is useful as a preprocessing step for live resource state before comparing it to program inputs.
 func pruneMap(source, target map[string]any) map[string]any {
+	// If either map is nil, return nil.
+	if target == nil || source == nil {
+		return nil
+	}
+
 	result := make(map[string]any)
 
 	for key, value := range source {
@@ -171,6 +176,11 @@ func pruneMap(source, target map[string]any) map[string]any {
 // pruneSlice builds a pruned slice by copying elements from the source slice that have a matching element in the
 // target slice.
 func pruneSlice(source, target []any) []any {
+	// If either slice is nil, return nil.
+	if target == nil || source == nil {
+		return nil
+	}
+
 	result := make([]any, 0, len(target))
 
 	// If either slice is empty, return an empty slice.
@@ -200,12 +210,12 @@ func pruneSlice(source, target []any) []any {
 		switch valueT.Kind() {
 		case reflect.Map:
 			nestedResult := pruneMap(value.(map[string]any), targetValue.(map[string]any))
-			if len(nestedResult) > 0 {
+			if nestedResult != nil {
 				result = append(result, nestedResult)
 			}
 		case reflect.Slice:
 			nestedResult := pruneSlice(value.([]any), targetValue.([]any))
-			if len(nestedResult) > 0 {
+			if nestedResult != nil {
 				result = append(result, nestedResult)
 			}
 		default:

diff --git a/provider/pkg/provider/util_test.go b/provider/pkg/provider/util_test.go
@@ -1035,6 +1035,39 @@ func TestPruneMap(t *testing.T) {
 			},
 			want: target,
 		},
+		{
+			name:        "empty nil map",
+			description: "nil map should result in nil result",
+			args: args{
+				source: nil,
+				target: nil,
+			},
+			want: nil,
+		},
+		{
+			name:        "empty nil source map",
+			description: "nil source map should result in nil result",
+			args: args{
+				source: nil,
+				target: map[string]any{
+					"a": "a",
+					"b": "b",
+				},
+			},
+			want: nil,
+		},
+		{
+			name:        "empty nil target map",
+			description: "nil target map should result in nil result",
+			args: args{
+				source: map[string]any{
+					"a": "a",
+					"b": "b",
+				},
+				target: nil,
+			},
+			want: nil,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1074,6 +1107,24 @@ func TestPruneSlice(t *testing.T) {
 			},
 			want: []any{},
 		},
+		{
+			name:        "nil target",
+			description: "nil target slice should result in nil result",
+			args: args{
+				source: []any{"a", "b"},
+				target: nil,
+			},
+			want: nil,
+		},
+		{
+			name:        "nil source",
+			description: "nil source slice should result in nil result",
+			args: args{
+				source: nil,
+				target: []any{"a", "b"},
+			},
+			want: nil,
+		},
 		{
 			name:        "matching number of elements with different values",
 			description: "a slice where target has matching number of elements with different values",
@@ -1224,6 +1275,111 @@ func TestPruneSlice(t *testing.T) {
 				nil,
 			},
 		},
+		{
+			name:        "map slice with empty non-nil value",
+			description: "a slice of map values that include an empty non-nil value",
+			args: args{
+				source: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{},
+				},
+				target: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{},
+				},
+			},
+			want: []any{
+				map[string]any{
+					"a": "a",
+					"b": "b",
+				},
+				map[string]any{},
+			},
+		},
+		{
+			name:        "map slice with empty non-nil value in target",
+			description: "a slice of map values that include an empty non-nil value only in target",
+			args: args{
+				source: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+				},
+				target: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{},
+				},
+			},
+			want: []any{
+				map[string]any{
+					"a": "a",
+					"b": "b",
+				},
+				map[string]any{},
+			},
+		},
+		{
+			name:        "map slice with empty non-nil value in source",
+			description: "a slice of map values that include an empty non-nil value only in source",
+			args: args{
+				source: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{},
+				},
+				target: []any{
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+					map[string]any{
+						"a": "a",
+						"b": "b",
+					},
+				},
+			},
+			want: []any{
+				map[string]any{
+					"a": "a",
+					"b": "b",
+				},
+				map[string]any{},
+			},
+		},
+		{
+			name:        "nil slice",
+			description: "nil slice should return nil",
+			args: args{
+				source: nil,
+				target: nil,
+			},
+			want: nil,
+		},
+		{
+			name:        "non-nil empty slice",
+			description: "non-nil empty slice should return empty slice",
+			args: args{
+				source: []any{},
+				target: []any{},
+			},
+			want: []any{},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/tests/ci-cluster/gke.ts b/tests/ci-cluster/gke.ts
@@ -45,6 +45,16 @@ export class GkeCluster extends pulumi.ComponentResource {
             },
             project: config.gcpProject,
             location: config.gcpLocation,
+            // Enable network policy addon to test network policy resources.
+            addonsConfig: {
+              networkPolicyConfig: {
+                disabled: false,
+              },
+            },
+            networkPolicy: {
+              enabled: true,
+              provider: "CALICO",
+            },
         }, {parent: this});
         this.cluster = k8sCluster;
 

diff --git a/tests/sdk/nodejs/examples/examples_test.go b/tests/sdk/nodejs/examples/examples_test.go
@@ -43,7 +43,7 @@ func TestAccMinimal(t *testing.T) {
 }
 
 func TestAccGuestbook(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:                  filepath.Join(getCwd(t), "guestbook"),
@@ -134,7 +134,7 @@ func TestAccGuestbook(t *testing.T) {
 }
 
 func TestAccIngress(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	testNetworkingV1 := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:           filepath.Join(getCwd(t), "ingress"),
@@ -164,7 +164,7 @@ func TestAccIngress(t *testing.T) {
 }
 
 func TestAccHelm(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:         filepath.Join(getCwd(t), "helm", "step1"),
@@ -190,7 +190,7 @@ func TestAccHelm(t *testing.T) {
 }
 
 func TestHelmNoDefaultProvider(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:         filepath.Join(getCwd(t), "helm-no-default-provider"),
@@ -204,7 +204,7 @@ func TestHelmNoDefaultProvider(t *testing.T) {
 }
 
 func TestAccHelmApiVersions(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:         filepath.Join(getCwd(t), "helm-api-versions", "step1"),
@@ -247,7 +247,7 @@ func TestAccHelmAllowCRDRendering(t *testing.T) {
 }
 
 func TestAccHelmLocal(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:                  filepath.Join(getCwd(t), "helm-local", "step1"),
@@ -291,7 +291,7 @@ func TestAccHelmLocal(t *testing.T) {
 }
 
 func testAccPrometheusOperator(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:         filepath.Join(getCwd(t), "prometheus-operator"),
@@ -328,7 +328,7 @@ func testAccPrometheusOperator(t *testing.T) {
 
 // TODO: Fix this example.
 //func TestAccMariadb(t *testing.T) {
-//	skipIfShort(t)
+//	tests.SkipIfShort(t)
 //	test := getBaseOptions(t).
 //		With(integration.ProgramTestOptions{
 //			Dir: filepath.Join(getCwd(t), "mariadb"),
@@ -338,7 +338,7 @@ func testAccPrometheusOperator(t *testing.T) {
 //}
 
 func TestAccProvider(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir: filepath.Join(getCwd(t), "provider"),
@@ -348,7 +348,7 @@ func TestAccProvider(t *testing.T) {
 }
 
 func TestHelmRelease(t *testing.T) {
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	validationFunc := func(t *testing.T, stackInfo integration.RuntimeValidationStackInfo) {
 		assert.NotEmpty(t, stackInfo.Outputs["redisMasterClusterIP"].(string))
 		assert.Equal(t, stackInfo.Outputs["status"], "deployed")
@@ -411,7 +411,7 @@ func TestHelmRelease(t *testing.T) {
 func TestHelmReleaseCRD(t *testing.T) {
 	// Validate that Helm charts with CRDs work across create/update/refresh/delete cycles.
 	// https://github.com/pulumi/pulumi-kubernetes/issues/1712
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:                  filepath.Join(getCwd(t), "helm-release-crd", "step1"),
@@ -431,7 +431,7 @@ func TestHelmReleaseCRD(t *testing.T) {
 
 func TestHelmReleaseNamespace(t *testing.T) {
 	// Validate fix for https://github.com/pulumi/pulumi-kubernetes/issues/1710
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:                  filepath.Join(getCwd(t), "helm-release-namespace", "step1"),
@@ -501,7 +501,7 @@ func TestHelmReleaseRedis(t *testing.T) {
 	}
 
 	// Validate fix for https://github.com/pulumi/pulumi-kubernetes/issues/1933
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:                  filepath.Join(getCwd(t), "helm-release-redis", "step1"),
@@ -526,7 +526,7 @@ func TestHelmReleaseRedis(t *testing.T) {
 
 func testRancher(t *testing.T) {
 	// Validate fix for https://github.com/pulumi/pulumi-kubernetes/issues/1848
-	skipIfShort(t)
+	tests.SkipIfShort(t)
 	test := getBaseOptions(t).
 		With(integration.ProgramTestOptions{
 			Dir:         filepath.Join(getCwd(t), "rancher", "step1"),
@@ -557,12 +557,6 @@ func TestCRDs(t *testing.T) {
 	t.Run("testRancher", testRancher)
 }
 
-func skipIfShort(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping long-running test in short mode")
-	}
-}
-
 func getCwd(t *testing.T) string {
 	cwd, err := os.Getwd()
 	if err != nil {

diff --git a/tests/sdk/nodejs/network-policy/step1/Pulumi.yaml b/tests/sdk/nodejs/network-policy/step1/Pulumi.yaml
@@ -0,0 +1,3 @@
+name: network-policy-tests
+description: Tests NetworkPolicy egress/ingress rules
+runtime: nodejs