manifest: remove forced replacement with x-kubernetes-preserve-unknown-fields (#2437)

BBBmau · web-flow · commit afd774dc67df · 2024-09-20T13:28:34.000+02:00
* Do not force replacement on type changes caused by CRD attributes with `x-kubernetes-preserve-unknown-fields`
* Add warning for when type changes
diff --git a/.changelog/2437.txt b/.changelog/2437.txt
@@ -0,0 +1,3 @@
+```release-note
+`kubernetes_manifest`: add TypeCheck for `x-kubernetes-preserve-unknown-fields` to prevent unnecessary replacement
+```
diff --git a/manifest/openapi/schema.go b/manifest/openapi/schema.go
@@ -114,7 +114,7 @@ func getTypeFromSchema(elem *openapi3.Schema, stackdepth uint64, typeCache *sync
 				return tftypes.String, nil
 			}
 		}
-		return tftypes.DynamicPseudoType, nil
+		return tftypes.DynamicPseudoType, nil // this is where DynamicType is set for when an attribute is tagged as 'x-kubernetes-preserve-unknown-fields'
 
 	case "array":
 		switch {
diff --git a/manifest/provider/plan.go b/manifest/provider/plan.go
@@ -459,8 +459,11 @@ func (s *RawProviderServer) PlanResourceChange(ctx context.Context, req *tfproto
 				if hasChanged {
 					h, ok := hints[morph.ValueToTypePath(ap).String()]
 					if ok && h == manifest.PreserveUnknownFieldsLabel {
-						apm := append(tftypes.NewAttributePath().WithAttributeName("manifest").Steps(), ap.Steps()...)
-						resp.RequiresReplace = append(resp.RequiresReplace, tftypes.NewAttributePathWithSteps(apm))
+						resp.Diagnostics = append(resp.Diagnostics, &tfprotov5.Diagnostic{
+							Severity: tfprotov5.DiagnosticSeverityWarning,
+							Summary:  fmt.Sprintf("The attribute path %v value's type is an x-kubernetes-preserve-unknown-field", morph.ValueToTypePath(ap).String()),
+							Detail:   "Changes to the type may cause some unexpected behavior.",
+						})
 					}
 				}
 				if isComputed {
diff --git a/manifest/test/acceptance/customresource_x_preserve_unknown_fields_test.go b/manifest/test/acceptance/customresource_x_preserve_unknown_fields_test.go
@@ -114,4 +114,39 @@ func TestKubernetesManifest_CustomResource_x_preserve_unknown_fields(t *testing.
 			"baz": interface{}("42"),
 		},
 	})
+
+	tfconfig = loadTerraformConfig(t, "x-kubernetes-preserve-unknown-fields/test-cr-3.tf", tfvars)
+	step1.SetConfig(ctx, string(tfconfig))
+	step1.Apply(ctx)
+
+	s3, err := step1.State(ctx)
+	if err != nil {
+		t.Fatalf("Failed to retrieve terraform state: %q", err)
+	}
+	tfstate3 := tfstatehelper.NewHelper(s3)
+	tfstate3.AssertAttributeValues(t, tfstatehelper.AttributeValues{
+		"kubernetes_manifest.test.object.metadata.name":      name,
+		"kubernetes_manifest.test.object.metadata.namespace": namespace,
+		"kubernetes_manifest.test.object.spec.count":         json.Number("100"),
+		"kubernetes_manifest.test.object.spec.resources": map[string]interface{}{
+			"foo": interface{}([]interface{}{"bar"}),
+			"baz": interface{}("42"),
+		},
+	})
+
+	tfconfig = loadTerraformConfig(t, "x-kubernetes-preserve-unknown-fields/test-cr-4.tf", tfvars)
+	step1.SetConfig(ctx, string(tfconfig))
+	step1.Apply(ctx)
+
+	s4, err := step1.State(ctx)
+	if err != nil {
+		t.Fatalf("Failed to retrieve terraform state: %q", err)
+	}
+	tfstate4 := tfstatehelper.NewHelper(s4)
+	tfstate4.AssertAttributeValues(t, tfstatehelper.AttributeValues{
+		"kubernetes_manifest.test.object.metadata.name":      name,
+		"kubernetes_manifest.test.object.metadata.namespace": namespace,
+		"kubernetes_manifest.test.object.spec.count":         json.Number("100"),
+		"kubernetes_manifest.test.object.spec.resources":     false,
+	})
 }
diff --git a/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/crd/test.tf b/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/crd/test.tf
@@ -4,14 +4,14 @@
 resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_rook_io" {
   manifest = {
     apiVersion = "apiextensions.k8s.io/v1"
-    kind = "CustomResourceDefinition"
+    kind       = "CustomResourceDefinition"
     metadata = {
       name = "${var.plural}.${var.group}"
     }
     spec = {
       group = var.group
       names = {
-        kind = var.kind
+        kind   = var.kind
         plural = var.plural
       }
       scope = "Namespaced"
@@ -24,14 +24,14 @@ resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_roo
                 spec = {
                   properties = {
                     annotations = {
-                      nullable = true
-                      type = "object"
+                      nullable                               = true
+                      type                                   = "object"
                       "x-kubernetes-preserve-unknown-fields" = true
                     }
                     count = {
                       maximum = 100
                       minimum = 1
-                      type = "integer"
+                      type    = "integer"
                     }
                     peers = {
                       properties = {
@@ -45,30 +45,29 @@ resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_roo
                       type = "object"
                     }
                     placement = {
-                      nullable = true
-                      type = "object"
+                      nullable                               = true
+                      type                                   = "object"
                       "x-kubernetes-preserve-unknown-fields" = true
                     }
                     priorityClassName = {
                       type = "string"
                     }
                     resources = {
-                      nullable = true
-                      type = "object"
+                      nullable                               = true
                       "x-kubernetes-preserve-unknown-fields" = true
                     }
                   }
                   type = "object"
                 }
                 status = {
-                  type = "object"
+                  type                                   = "object"
                   "x-kubernetes-preserve-unknown-fields" = true
                 }
               }
               type = "object"
             }
           }
-          served = true
+          served  = true
           storage = true
           subresources = {
             status = {}
diff --git a/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-1.tf b/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-1.tf
@@ -4,16 +4,16 @@
 resource "kubernetes_manifest" "test" {
   manifest = {
     apiVersion = var.group_version
-    kind = var.kind
+    kind       = var.kind
     metadata = {
-      name = var.name
+      name      = var.name
       namespace = var.namespace
     }
     spec = {
-        count = 100
-        resources = {
-            foo = "bar"
-        }
+      count = 100
+      resources = {
+        foo = "bar"
+      }
     }
   }
 }
diff --git a/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-2.tf b/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-2.tf
@@ -4,17 +4,17 @@
 resource "kubernetes_manifest" "test" {
   manifest = {
     apiVersion = var.group_version
-    kind = var.kind
+    kind       = var.kind
     metadata = {
-      name = var.name
+      name      = var.name
       namespace = var.namespace
     }
     spec = {
-        count = 100
-        resources = {
-            foo = "bar"
-            baz = "42"
-        }
+      count = 100
+      resources = {
+        foo = "bar"
+        baz = "42"
+      }
     }
   }
 }
diff --git a/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-3.tf b/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-3.tf
@@ -0,0 +1,20 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+resource "kubernetes_manifest" "test" {
+  manifest = {
+    apiVersion = var.group_version
+    kind       = var.kind
+    metadata = {
+      name      = var.name
+      namespace = var.namespace
+    }
+    spec = {
+      count = 100
+      resources = {
+        foo = ["bar"]
+        baz = "42"
+      }
+    }
+  }
+}
diff --git a/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-4.tf b/manifest/test/acceptance/testdata/x-kubernetes-preserve-unknown-fields/test-cr-4.tf
@@ -0,0 +1,17 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+resource "kubernetes_manifest" "test" {
+  manifest = {
+    apiVersion = var.group_version
+    kind       = var.kind
+    metadata = {
+      name      = var.name
+      namespace = var.namespace
+    }
+    spec = {
+      count     = 100
+      resources = false
+    }
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	+```release-note
	`2`	+`kubernetes_manifest`: add TypeCheck for `x-kubernetes-preserve-unknown-fields` to prevent unnecessary replacement
	`3`	+```
Original file line number	Diff line number	Diff line change
`@@ -114,7 +114,7 @@ func getTypeFromSchema(elem openapi3.Schema, stackdepth uint64, typeCache sync`
`114`	`114`	`return tftypes.String, nil`
`115`	`115`	`}`
`116`	`116`	`}`
`117`		`- return tftypes.DynamicPseudoType, nil`
	`117`	`+ return tftypes.DynamicPseudoType, nil // this is where DynamicType is set for when an attribute is tagged as 'x-kubernetes-preserve-unknown-fields'`
`118`	`118`
`119`	`119`	`case "array":`
`120`	`120`	`switch {`
Original file line number	Diff line number	Diff line change
`@@ -459,8 +459,11 @@ func (s RawProviderServer) PlanResourceChange(ctx context.Context, req tfproto`
`459`	`459`	`if hasChanged {`
`460`	`460`	`h, ok := hints[morph.ValueToTypePath(ap).String()]`
`461`	`461`	`if ok && h == manifest.PreserveUnknownFieldsLabel {`
`462`		`- apm := append(tftypes.NewAttributePath().WithAttributeName("manifest").Steps(), ap.Steps()...)`
`463`		`- resp.RequiresReplace = append(resp.RequiresReplace, tftypes.NewAttributePathWithSteps(apm))`
	`462`	`+ resp.Diagnostics = append(resp.Diagnostics, &tfprotov5.Diagnostic{`
	`463`	`+ Severity: tfprotov5.DiagnosticSeverityWarning,`
	`464`	`+ Summary: fmt.Sprintf("The attribute path %v value's type is an x-kubernetes-preserve-unknown-field", morph.ValueToTypePath(ap).String()),`
	`465`	`+ Detail: "Changes to the type may cause some unexpected behavior.",`
	`466`	`+ })`
`464`	`467`	`}`
`465`	`468`	`}`
`466`	`469`	`if isComputed {`
Original file line number	Diff line number	Diff line change
`@@ -4,14 +4,14 @@`
`4`	`4`	`resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_rook_io" {`
`5`	`5`	`manifest = {`
`6`	`6`	`apiVersion = "apiextensions.k8s.io/v1"`
`7`		`- kind = "CustomResourceDefinition"`
	`7`	`+ kind = "CustomResourceDefinition"`
`8`	`8`	`metadata = {`
`9`	`9`	`name = "${var.plural}.${var.group}"`
`10`	`10`	`}`
`11`	`11`	`spec = {`
`12`	`12`	`group = var.group`
`13`	`13`	`names = {`
`14`		`- kind = var.kind`
	`14`	`+ kind = var.kind`
`15`	`15`	`plural = var.plural`
`16`	`16`	`}`
`17`	`17`	`scope = "Namespaced"`
`@@ -24,14 +24,14 @@ resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_roo`
`24`	`24`	`spec = {`
`25`	`25`	`properties = {`
`26`	`26`	`annotations = {`
`27`		`- nullable = true`
`28`		`- type = "object"`
	`27`	`+ nullable = true`
	`28`	`+ type = "object"`
`29`	`29`	`"x-kubernetes-preserve-unknown-fields" = true`
`30`	`30`	`}`
`31`	`31`	`count = {`
`32`	`32`	`maximum = 100`
`33`	`33`	`minimum = 1`
`34`		`- type = "integer"`
	`34`	`+ type = "integer"`
`35`	`35`	`}`
`36`	`36`	`peers = {`
`37`	`37`	`properties = {`
`@@ -45,30 +45,29 @@ resource "kubernetes_manifest" "customresourcedefinition_cephrbdmirrors_ceph_roo`
`45`	`45`	`type = "object"`
`46`	`46`	`}`
`47`	`47`	`placement = {`
`48`		`- nullable = true`
`49`		`- type = "object"`
	`48`	`+ nullable = true`
	`49`	`+ type = "object"`
`50`	`50`	`"x-kubernetes-preserve-unknown-fields" = true`
`51`	`51`	`}`
`52`	`52`	`priorityClassName = {`
`53`	`53`	`type = "string"`
`54`	`54`	`}`
`55`	`55`	`resources = {`
`56`		`- nullable = true`
`57`		`- type = "object"`
	`56`	`+ nullable = true`
`58`	`57`	`"x-kubernetes-preserve-unknown-fields" = true`
`59`	`58`	`}`
`60`	`59`	`}`
`61`	`60`	`type = "object"`
`62`	`61`	`}`
`63`	`62`	`status = {`
`64`		`- type = "object"`
	`63`	`+ type = "object"`
`65`	`64`	`"x-kubernetes-preserve-unknown-fields" = true`
`66`	`65`	`}`
`67`	`66`	`}`
`68`	`67`	`type = "object"`
`69`	`68`	`}`
`70`	`69`	`}`
`71`		`- served = true`
	`70`	`+ served = true`
`72`	`71`	`storage = true`
`73`	`72`	`subresources = {`
`74`	`73`	`status = {}`
Original file line number	Diff line number	Diff line change
`@@ -4,16 +4,16 @@`
`4`	`4`	`resource "kubernetes_manifest" "test" {`
`5`	`5`	`manifest = {`
`6`	`6`	`apiVersion = var.group_version`
`7`		`- kind = var.kind`
	`7`	`+ kind = var.kind`
`8`	`8`	`metadata = {`
`9`		`- name = var.name`
	`9`	`+ name = var.name`
`10`	`10`	`namespace = var.namespace`
`11`	`11`	`}`
`12`	`12`	`spec = {`
`13`		`- count = 100`
`14`		`- resources = {`
`15`		`- foo = "bar"`
`16`		`- }`
	`13`	`+ count = 100`
	`14`	`+ resources = {`
	`15`	`+ foo = "bar"`
	`16`	`+ }`
`17`	`17`	`}`
`18`	`18`	`}`
`19`	`19`	`}`