Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Sep 20, 2024

Conversation

BBBmau
Copy link
Contributor

@BBBmau BBBmau commented Mar 4, 2024

Description

This check will prevent x-kubernetes-preserve-unknown-fields to cause a replacement whenever the value is changed. The idea is that it should ONLY cause a replacement if the type itself is changed and not the value.

Fixes #2371
Fixes #2375
Fixes #1928
Fixes #2410

Acceptance tests

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

Output from acceptance testing:

┌─(~/Dev/terraform-provider-kubernetes/manifest)───────────────────────────────────────────────(mau@mau-JKDT676NCP:s077)─┐
└─(14:48:09 on fix-unknownFieldsLogic-manifest ✹ ✭)──> make testacc TESTARGS="-run TestKubernetesManifest_CustomResource_x_preserve_unknown_fields"
go test -count=1 -tags acceptance "./test/acceptance" -v -run TestKubernetesManifest_CustomResource_x_preserve_unknown_fields -timeout 120m
2024/03/12 14:48:28 Testing against Kubernetes API version: v1.27.3
=== RUN   TestKubernetesManifest_CustomResource_x_preserve_unknown_fields
--- PASS: TestKubernetesManifest_CustomResource_x_preserve_unknown_fields (14.23s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/manifest/test/acceptance     14.832s```

Release Note

Release note for CHANGELOG:

`kubernetes_manifest`: add TypeCheck for `x-kubernetes-preserve-unknown-fields` to prevent unnecessary replacement

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 BBBmau added the manifest label Mar 4, 2024
@BBBmau BBBmau requested a review from a team as a code owner March 4, 2024 21:19
@BBBmau
Copy link
Contributor Author

BBBmau commented Mar 4, 2024

@alexsomesan

I'll need some feedback since when I run it through the debug. the plan produces a plannedchange of just the value and not the entire resource. However if we were to run it without debug mode it still produces the same replacement action, which goes against the logic that's been applied in this PR.

After resolving that we'll look into adding a test for this. 💯

@BBBmau BBBmau requested a review from alexsomesan March 4, 2024 21:21
@BBBmau
Copy link
Contributor Author

BBBmau commented Apr 3, 2024

Adding a note here that when adding a value into the tuple it will cause a recreation despite the type being the same:

          valuesObject = {
            "mau": ["test"] -> ["test", "test2"]
            }
Plan: 1 to add, 0 to change, 1 to destroy.
╷
│ Warning: The attribute path AttributeName("spec").AttributeName("sources").ElementKeyInt(-1).AttributeName("helm").AttributeName("valuesObject") value's type has changed
│ 
│   with kubernetes_manifest.raw,
│   on main.tf line 3, in resource "kubernetes_manifest" "raw":
│    3: resource "kubernetes_manifest" "raw" {
│ 
│ Changes to the type will cause a forced replacement.

An extra check is needed before we can get this merged.

@BBBmau
Copy link
Contributor Author

BBBmau commented Apr 3, 2024

Adding a note here that when adding a value into the tuple it will cause a recreation despite the type being the same:

          valuesObject = {
            "mau": ["test"] -> ["test", "test2"]
            }
Plan: 1 to add, 0 to change, 1 to destroy.
╷
│ Warning: The attribute path AttributeName("spec").AttributeName("sources").ElementKeyInt(-1).AttributeName("helm").AttributeName("valuesObject") value's type has changed
│ 
│   with kubernetes_manifest.raw,
│   on main.tf line 3, in resource "kubernetes_manifest" "raw":
│    3: resource "kubernetes_manifest" "raw" {
│ 
│ Changes to the type will cause a forced replacement.

An extra check is needed before we can get this merged.

Recent commit addresses this check by using Diff method from tftypes.Value which returns an Error if a change in type occurs on values.

_, diff := wasCfg.(tftypes.Value).Diff(nowCfg.(tftypes.Value)) // passing values of two different types will result in an error.

Prior we would check by using the Equal method:
typeChanged := !(wasCfg.(tftypes.Value).Type().Equal(nowCfg.(tftypes.Value).Type())) this didn't work when adding to the tuple despite the type still staying the same. Could be an issue with the Equal method type. From the docs: Equal performs deep type equality checks, including attribute/element types and whether attributes are optional or not.

@BBBmau
Copy link
Contributor Author

BBBmau commented Apr 4, 2024

In the past, type changes was something that terraform panic to. This was early in the development of terraform core and appears it is not an issue anymore. Because of this we can remove the forced replacement for x-kubernetes-preserve-unknown-fields but will add a warning when users are intending to change types.

@BBBmau BBBmau changed the title manifest: add TypeChange check for x-kubernetes-preserve-unknown-fields manifest: remove forced replacement with x-kubernetes-preserve-unknown-fields Apr 4, 2024
@hongshaoyang
Copy link

This PR fixes #1829

@TessaIO
Copy link

TessaIO commented Jun 27, 2024

Any update on this PR?

@CORVU5
Copy link

CORVU5 commented Jul 24, 2024

Any update on when we can expect this to be released?

@FarhadF
Copy link

FarhadF commented Sep 3, 2024

@BBBmau @alexsomesan whats blocking this PR?

@github-actions github-actions bot added size/M and removed size/S labels Sep 20, 2024
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.
I added an extra test case, just to be on the safe side.

@alexsomesan alexsomesan merged commit afd774d into main Sep 20, 2024
64 checks passed
@alexsomesan alexsomesan deleted the fix-unknownFieldsLogic-manifest branch September 20, 2024 11:28
@BBBmau BBBmau added this to the v2.33.0 milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment