terraform-provider-kubernetes icon indicating copy to clipboard operation
terraform-provider-kubernetes copied to clipboard

manifest: change `computed_field` logic to mark all as unknown instead of only on changes

Open BBBmau opened this issue 1 year ago • 6 comments

Description

The idea is to treat all values in computed_field as an UnknownValue, before the values would only be treated as an UnknownValue when a change was present. This will insure that values in annotations/labels are covered especially in cases where the user did not set the label/annotation themselves.

Fixes #1591

Acceptance tests

  • [x] Have you added an acceptance test for the functionality being added?
  • [x] Have you run the acceptance tests on this branch?

Output from acceptance testing:

┌─(~/Dev/terraform-provider-kubernetes/manifest)───────────────────────────────────────────────(mau@mau-JKDT676NCP:s077)─┐
└─(13:48:26 on computed_fields_allUnknown ✹ ✭)──> make testacc TESTARGS="-run TestKubernetesManifest_ComputedDeploymentFields"
go test -count=1 -tags acceptance "./test/acceptance" -v -run TestKubernetesManifest_ComputedDeploymentFields -timeout 120m
2024/03/12 13:49:29 Testing against Kubernetes API version: v1.27.3
=== RUN   TestKubernetesManifest_ComputedDeploymentFields
--- PASS: TestKubernetesManifest_ComputedDeploymentFields (2.87s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/manifest/test/acceptance     3.534s

Release Note

Release note for CHANGELOG:

kubernetes_manifest: Fix unexpected annotation by setting values in `computed_fields` to be Unknown always

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

BBBmau avatar Feb 21 '24 22:02 BBBmau

latest commit solves the issue from referenced issue, however we still run into this error if the user chooses to set computed_fields and not include annotations/labels in the list of paths. However this does still follow the intended design from the docs:

IMPORTANT: By default, metadata.labels and metadata.annotations are already included in the list. You don't have to set them explicitly in the computed_fields list. To turn off these defaults, set the value of computed_fields to an empty list or a concrete list of other fields. For example computed_fields = [].

If the solution to this looks good then i think the only thing left is to add a test for this case to check that annotations/labels are seen as UnknownValues always

BBBmau avatar Feb 21 '24 22:02 BBBmau

I've attempted to run the test on my machine but running into issues, could be because im not using kind and instead using minikube:

└─(17:43:54 on computed_fields_allUnknown ✹)──> make testacc TESTARGS="-run TestKubernetesManifest_ComputedFields"                                   2 ↵ ──(Fri,Mar01)─┘
go test -count=1 -tags acceptance "./test/acceptance" -v -run TestKubernetesManifest_ComputedFields -timeout 120m
2024/03/01 17:44:13 Testing against Kubernetes API version: v1.29.0-eks-c417bb3
=== RUN   TestKubernetesManifest_ComputedFields
2024-03-01T17:45:04.939-0800 [ERROR] [ApplyResourceChange][Apply]: API error="Internal error occurred: failed calling webhook \"tf-acc-test-wbezqriecd.hashicorp.com\": failed to call webhook: Post \"https://tf-acc-test-wbezqriecd.tf-acc-test-cnbarqjfjb.svc:443/mutate?timeout=10s\": no endpoints available for service \"tf-acc-test-wbezqriecd\"" API response=<nil>
    computed_attr_test.go:78: Error when trying to get resource tf-acc-test-cnbarqjfjb/tf-acc-test-wbezqriecd: configmaps "tf-acc-test-wbezqriecd" not found
--- FAIL: TestKubernetesManifest_ComputedFields (109.39s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x103e41168]

goroutine 70 [running]:
testing.tRunner.func1.2({0x104420dc0, 0x1056a9360})
        /opt/homebrew/Cellar/go/1.22.0/libexec/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
        /opt/homebrew/Cellar/go/1.22.0/libexec/src/testing/testing.go:1634 +0x33c
panic({0x104420dc0?, 0x1056a9360?})
        /opt/homebrew/Cellar/go/1.22.0/libexec/src/runtime/panic.go:770 +0x124
github.com/hashicorp/terraform-provider-kubernetes/manifest/test/helper/state.getAttributesValuesFromResource(0x103ef09c3?, {0x140012d1e78, 0x18})
        /Users/mau/Dev/terraform-provider-kubernetes/manifest/test/helper/state/state_helper.go:37 +0x28
github.com/hashicorp/terraform-provider-kubernetes/manifest/test/helper/state.(*Helper).GetAttributeValue(0x1400089bac8, 0x140004309c0, {0x103ef09c3, 0x3f})
        /Users/mau/Dev/terraform-provider-kubernetes/manifest/test/helper/state/state_helper.go:113 +0x60
github.com/hashicorp/terraform-provider-kubernetes/manifest/test/helper/state.(*Helper).AssertAttributeValues(0x140018cdac8, 0x140004309c0, 0x1400089ba98)
        /Users/mau/Dev/terraform-provider-kubernetes/manifest/test/helper/state/state_helper.go:147 +0x98
github.com/hashicorp/terraform-provider-kubernetes/manifest/test/acceptance.TestKubernetesManifest_ComputedFields(0x140004309c0)
        /Users/mau/Dev/terraform-provider-kubernetes/manifest/test/acceptance/computed_attr_test.go:85 +0xb34
testing.tRunner(0x140004309c0, 0x104701c68)
        /opt/homebrew/Cellar/go/1.22.0/libexec/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 1
        /opt/homebrew/Cellar/go/1.22.0/libexec/src/testing/testing.go:1742 +0x318
FAIL    github.com/hashicorp/terraform-provider-kubernetes/manifest/test/acceptance     112.438s
FAIL
make: *** [testacc] Error 1
┌─(~/Dev/terraform-provider-kubernetes/manifest)───────────────────────────────────────────────────────────────────────────────────────
┌─(~/Dev/terraform-provider-kubernetes/manifest)──────────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s023)─┐
└─(17:46:52 on computed_fields_allUnknown ✹)──> git commit -m "cleanup plan code and add computedFields test when setting computedFields"
[computed_fields_allUnknown a86a18182] cleanup plan code and add computedFields test when setting computedFields
 4 files changed, 62 insertions(+), 9 deletions(-)

BBBmau avatar Mar 02 '24 01:03 BBBmau

Latest commit addresses the case where API changes the config value set by the user.

How it appears in config:

...
resources = {
     limits = {
       cpu    = "0.25"
       memory = "512Mi"
}

How it appears in YAML

...
    Limits:
      cpu:     250m
      memory:  512Mi

BBBmau avatar Apr 03 '24 20:04 BBBmau

~~I played around more with this and found this config from the original issue to cause a change despite no change actually happening. The original panic doesn't happen however.~~ This is actually the designed behavior now, this can be ignored! TFCONFIG:

resource "kubernetes_manifest" "daemonset_prometheus_daemonset" {
  manifest = {
    apiVersion = "apps/v1"
    kind       = "DaemonSet"
    metadata = {
      name      = "prometheus-daemonset"
      namespace = "default"
    }
    spec = {
      selector = {
        matchLabels = {
          name = "prometheus-exporter"
          tier = "monitoring"
        }
      }
      template = {
        metadata = {
          labels = {
            name = "prometheus-exporter"
            tier = "monitoring"
          }
        }
        spec = {
          containers = [
            {
              image = "prom/node-exporter"
              name  = "prometheus"
              ports = [
                {
                  containerPort = 80
                },
              ]
            },
          ]
        }
      }
    }
  }
}
  # kubernetes_manifest.daemonset_prometheus_daemonset will be updated in-place
  ~ resource "kubernetes_manifest" "daemonset_prometheus_daemonset" {
      ~ object   = {
          ~ metadata   = {
              ~ annotations                = {
                  - "deprecated.daemonset.template.generation" = "1"
                } -> (known after apply)
              + labels                     = (known after apply)
                name                       = "prometheus-daemonset"
                # (12 unchanged attributes hidden)
            }
            # (3 unchanged attributes hidden)
        }
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Changes to the findBackfillValue could resolve this.

BBBmau avatar Apr 03 '24 21:04 BBBmau