kustomize icon indicating copy to clipboard operation
kustomize copied to clipboard

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

Open aenshin-pp opened this issue 1 year ago • 12 comments

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 avatar Mar 11 '24 14:03 aenshin-pp

/triage accepted /kind bug

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

stormqueen1990 avatar Mar 11 '24 14:03 stormqueen1990

/triage duplicate

Duplicate of #5031

stormqueen1990 avatar Mar 11 '24 14:03 stormqueen1990

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

creationTimestamp: "null"
creationTimestamp: null

Both of them were not presented in previous build.

aenshin-pp avatar Mar 11 '24 14:03 aenshin-pp

/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 avatar Mar 12 '24 02:03 stormqueen1990

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

stormqueen1990 avatar Mar 16 '24 21:03 stormqueen1990

@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:

  • 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Mar 16 '24 21:03 k8s-ci-robot

/remove-triage accepted /remove-kind bug

stormqueen1990 avatar Mar 16 '24 21:03 stormqueen1990

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

koba1t avatar Mar 21 '24 13:03 koba1t

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.

stormqueen1990 avatar Mar 21 '24 14:03 stormqueen1990

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

koba1t avatar Mar 21 '24 14:03 koba1t