search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
11.09k stars 2.26k forks source link

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

Open aenshin-pp opened 8 months ago

aenshin-pp commented 8 months ago

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

stormqueen1990 commented 8 months ago

/triage accepted /kind bug

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

stormqueen1990 commented 8 months ago

/triage duplicate

Duplicate of #5031

aenshin-pp commented 8 months ago

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

creationTimestamp: "null"
creationTimestamp: null

Both of them were not presented in previous build.

stormqueen1990 commented 8 months ago

/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.

stormqueen1990 commented 8 months ago

/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 commented 8 months ago

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

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/5568#issuecomment-2002134354): >/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: >* https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/c4471defda10f8184173192b9f9df4e411ee5dac/config/webhook/manifests.yaml#L1-L5 >* https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/c4471defda10f8184173192b9f9df4e411ee5dac/config/webhook/manifests.yaml#L47-L51 > >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
stormqueen1990 commented 8 months ago

/remove-triage accepted /remove-kind bug

koba1t commented 8 months ago

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

stormqueen1990 commented 8 months ago

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 commented 8 months ago

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