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

The latest 5.3.0 kustomize adds two new lines comparing with older 4.4.1 one #5568

Open
aenshin-pp opened this issue Mar 11, 2024 · 10 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on. triage/duplicate Indicates an issue is a duplicate of other open issue.

Comments

@aenshin-pp
Copy link

What happened?

Two new lines appeared suddenly. I was not expecting them at all.

What did you expect to happen?

I would expect zero changes in my output with kustomize update.

How can we reproduce it (as minimally and precisely as possible)?

# kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - github.com/kubernetes-sigs/aws-load-balancer-controller/config/default?ref=v2.4.1

The diff will be

> diff /tmp/a /tmp/b
898a899
>   creationTimestamp: "null"
959a961
>   creationTimestamp: null

Two new lines appeared.

Expected output

No response

Actual output

No response

Kustomize version

5.3.0 vs 4.4.1

Operating system

None

@aenshin-pp aenshin-pp added the kind/bug Categorizes issue or PR as related to a bug. label Mar 11, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 11, 2024
@aenshin-pp aenshin-pp changed the title latest 5.3.0 kustomize add two new lines comparing with older 4.4.1 one latest 5.3.0 kustomize adds two new lines comparing with older 4.4.1 one Mar 11, 2024
@aenshin-pp aenshin-pp changed the title latest 5.3.0 kustomize adds two new lines comparing with older 4.4.1 one The latest 5.3.0 kustomize adds two new lines comparing with older 4.4.1 one Mar 11, 2024
@stormqueen1990
Copy link
Member

/triage accepted
/kind bug

This issue should be fixed by #5519 once it merges.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 11, 2024
@stormqueen1990
Copy link
Member

/triage duplicate

Duplicate of #5031

@k8s-ci-robot k8s-ci-robot added the triage/duplicate Indicates an issue is a duplicate of other open issue. label Mar 11, 2024
@aenshin-pp
Copy link
Author

@stormqueen1990 it is not replaced but added two new lines:

creationTimestamp: "null"
creationTimestamp: null

Both of them were not presented in previous build.

@stormqueen1990
Copy link
Member

stormqueen1990 commented Mar 12, 2024

/remove-triage duplicate

After further inspection, I noticed that this is a separate issue from #5031. It seems the extra creationTimestamp: null field is being added solely to the MutatingWebhookConfiguration and ValidatingWebhookConfiguration resources.

@k8s-ci-robot k8s-ci-robot removed the triage/duplicate Indicates an issue is a duplicate of other open issue. label Mar 12, 2024
@stormqueen1990
Copy link
Member

/remove-triage bug

I took a better look at this and noticed that the creationTimestamp: null fields are present in both the source MutatingWebhookConfiguration and ValidatingWebhookConfiguration resources:

I believe the prior behaviour of Kustomize was less desirable than the current one, since it would remove fields present on the source YAMLs. As for the rendering of creationTimestamp as a string value, the merging of #5519 should have fixed it.

@koba1t @varshaprasad96 what are your opinions about this change in behaviour of Kustomize between v4 and v5?

@k8s-ci-robot
Copy link
Contributor

@stormqueen1990: Those labels are not set on the issue: triage/bug

In response to this:

/remove-triage bug

I took a better look at this and noticed that the creationTimestamp: null fields are present in both the source MutatingWebhookConfiguration and ValidatingWebhookConfiguration resources:

I believe the prior behaviour of Kustomize was less desirable than the current one, since it would remove fields present on the source YAMLs. As for the rendering of creationTimestamp as a string value, the merging of #5519 should have fixed it.

@koba1t @varshaprasad96 what are your opinions about this change in behaviour of Kustomize between v4 and v5?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@stormqueen1990
Copy link
Member

/remove-triage accepted
/remove-kind bug

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. kind/bug Categorizes issue or PR as related to a bug. labels Mar 16, 2024
@koba1t
Copy link
Member

koba1t commented Mar 21, 2024

Hi @stormqueen1990

I believe this behavior is caused by the patches transformer.
I don't know why, but if we apply patch two or more times, we can't handle null values.

For example

YAML

# kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - manifests.yaml

patches:
  - path: patch.yaml
  - path: patch2.yaml
# manifests.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  creationTimestamp: null
  name: webhook
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  creationTimestamp: null
  name: webhook
# patch.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: webhook
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  name: webhook
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
# patch2.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: webhook
  annotations:
    secondannotation: "dummythings"
---
# apiVersion: admissionregistration.k8s.io/v1
# kind: ValidatingWebhookConfiguration
# metadata:
#   name: webhook
#   annotations:
#     secondannotation: "dummythings"

result

$ kustomize build .
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
    secondannotation: dummythings
  creationTimestamp: "null"
  name: webhook
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
  creationTimestamp: null
  name: webhook

I think the addition of the creationTimestamp field is a misconfiguration.
But I feel that it contains value for fixing this problem.

/kind bug
/triage accepted

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 21, 2024
@stormqueen1990
Copy link
Member

I think the addition of the creationTimestamp field is a misconfiguration. But I feel that it contains value for fixing this problem.

Hi there, @koba1t!

I was under the impression that the transformation of null values into "null" was fixed with #5519. I rebuilt Kustomize from master after the merge of that PR and wasn't able to reproduce this issue any longer.

@koba1t
Copy link
Member

koba1t commented Mar 21, 2024

Thanks @stormqueen1990. You Are entirely right.
I forgot we haven't released the binary recently....

In the master version, this problem is solved.

$ ~/go/bin/kustomize build .
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
    secondannotation: dummythings
  creationTimestamp: null
  name: webhook
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)
  creationTimestamp: null
  name: webhook

The removal of the creationTimestamp was a problem. I think the current behavior is better than before.

/triage duplicate

@k8s-ci-robot k8s-ci-robot added the triage/duplicate Indicates an issue is a duplicate of other open issue. label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on. triage/duplicate Indicates an issue is a duplicate of other open issue.
Projects
None yet
Development

No branches or pull requests

4 participants