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

🐛 Fix manifest object type mutation #2372

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

arybolovlev
Copy link
Contributor

@arybolovlev arybolovlev commented Dec 18, 2023

Description

This PR fix an issue in the provider that happens when the data source kubernetes_resources receives multiple Kubernetes objects containing tuples with different numbers of elements.

The data source kubernetes_resources takes a Kubernetes object schema and generates a tftypes.Type representation of a Kubernetes resource once. All Kubernetes objects received by kubernetes_resources should conform to this schema. The issue arises when received objects contain a tuple with a different number of elements.

This occurs during the provider's iteration over received Kubernetes objects, invoking the DeepUnknown function. One of the arguments that DeepUnknown receives is the tftypes.Type representation of a Kubernetes resource (object type), which undergoes mutation within the DeepUnknown function. This mutation involves normalizing the number of elements in the tuple to match the number of elements in the Kubernetes objects being processed. If a subsequent Kubernetes object contains a different type of element, the provider throws an error, as the object appears inconsistent with the mutated object type. This pull request addresses the issue by fixing the mutation step, ensuring that the object type remains unchanged.

The following Terraform code should return all Pods in kube-system namespace:

data "kubernetes_resources" "pods" {
  api_version = "v1"
  kind        = "Pod"
  namespace   = "kube-system"
}

Here are two out of eight objects that the provider receives:

Pod coredns:

apiVersion: v1
kind: Pod
metadata:
  ...
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    ...
  - apiVersion: v1
    fieldsType: FieldsV1
    ...
  - apiVersion: v1
    fieldsType: FieldsV1
    ...
  name: coredns-5d78c9869d-drcfj
  namespace: kube-system
  ...

Pod etcd:

apiVersion: v1
kind: Pod
metadata:
  ...
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    ...
  - apiVersion: v1
    fieldsType: FieldsV1
    ...
  name: etcd-kube27-control-plane
  namespace: kube-system
 ...

In the case of the coredns pod, we can see that the metadata.managedFields tuple contains 3 elements. However, the following etcd pod contains only 2 elements, leading the provider to quit with an error at this point.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?

Release Note

Release note for CHANGELOG:

`data_source/kubernetes_resources`: fix an issue where the provider exits with an error when the data source `kubernetes_resources` receives multiple Kubernetes objects containing tuples with different numbers of elements.

References

Fix: #2215

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

@arybolovlev arybolovlev force-pushed the fix-manifest-objectype-mutation branch from 8ce036d to 83d92b6 Compare December 19, 2023 08:28
@github-actions github-actions bot added size/L and removed size/XS labels Dec 19, 2023
@arybolovlev arybolovlev force-pushed the fix-manifest-objectype-mutation branch from f2b971d to a8464b3 Compare December 19, 2023 13:04
@arybolovlev arybolovlev marked this pull request as ready for review December 26, 2023 14:58
@arybolovlev arybolovlev requested a review from a team as a code owner December 26, 2023 14:58
@arybolovlev arybolovlev force-pushed the fix-manifest-objectype-mutation branch from d40184b to 23c78b1 Compare January 2, 2024 13:14
Copy link
Collaborator

@jrhouston jrhouston left a comment

Choose a reason for hiding this comment

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

Nice! 🔧

@@ -130,7 +130,7 @@ func (s *RawProviderServer) ReadPluralDataSource(ctx context.Context, req *tfpro
if err != nil {
resp.Diagnostics = append(resp.Diagnostics, &tfprotov5.Diagnostic{
Severity: tfprotov5.DiagnosticSeverityError,
Summary: "Failed to save resource state",
Summary: "Failed to save resource state", // FIX ME
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these FIXMEs here to add a more meaningful summary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those 4 don't reflect the errors they represent, I noticed this while working on this PR. I will open a new PR to fix it. Next time I will add more meaningful text that just FIX ME. 😔

@cguertin14
Copy link

hey! any updates on this? :)

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.

This makes sense. I also tested against issue #2440 and this fix resolves it.
Apologies for the long delay in getting this in.

@alexsomesan alexsomesan merged commit f83d63a into main Apr 17, 2024
33 checks passed
@alexsomesan alexsomesan deleted the fix-manifest-objectype-mutation branch April 17, 2024 18:24
dduportal referenced this pull request in jenkins-infra/azure May 10, 2024
<Actions>
<action
id="bcd9b70d7c1eb1e07eb5ad8a958f18dc1bbd81461ee1a2604adeea46e3148a47">
        <h3>Bump Terraform `kubernetes` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/kubernetes&#34; updated
from &#34;2.29.0&#34; to &#34;2.30.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>2.30.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-kubernetes/releases/tag/v2.30.0&#xA;BUG
FIXES:&#xD;&#xA;&#xD;&#xA;* `data_source/kubernetes_resources`: fix an
issue where the provider exit with an error when the data source
`kubernetes_resources` receives multiple Kubernetes objects containing
tuples with different numbers of elements.
[[GH-2372](https://github.com/hashicorp/terraform-provider-kubernetes/issues/2372)]&#xD;&#xA;*
`kubernetes_manifest`: fix issue preventing KUBE_PROXY_URL environment
variable from being used in client configuration (#1733)
[[GH-2485](https://github.com/hashicorp/terraform-provider-kubernetes/issues/2485)]&#xD;&#xA;*
`resource/kubernetes_node_taint`: Fix the error check for nonexistant
nodes so that terraform does not fail if there is a taint in the state
file for a node that has been deleted.
[[GH-2402](https://github.com/hashicorp/terraform-provider-kubernetes/issues/2402)]&#xD;&#xA;&#xD;&#xA;DOCS:&#xD;&#xA;&#xD;&#xA;*
Migrate legacy structure to new tfplugindocs template structure
[[GH-2470](https://github.com/hashicorp/terraform-provider-kubernetes/issues/2470)]&#xD;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/161/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error listing K8s resources using data_kubernetes_resources
4 participants