🌱 Kustomize: Update deprecated syntax #9223

lentzi90 · 2023-08-17T11:30:08Z

What this PR does / why we need it:

This commit updates the following:

patchesStrategicMerge -> patches
patchesJson6902 -> patches
vars and varReference -> replacements
bases -> resources

Most of this is straight forward, but the vars -> replacements change is a bit complicated. I have taken inspiration from kubebuilder for how to do the change. In particular I changed the name of the secret that holds the certificate to be static. Previously it was set partially from a variable. I believe it would be unnecessarily complicated to keep this behavior and that a static name does not take away much.

Here is a link to some relevant code in kubebuilder that I used as inspiration: https://github.com/kubernetes-sigs/kubebuilder/blob/68abac1dfb5550f7159dec4ce600cd9b6db925bb/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/kdefault/kustomization.go#L97

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #8457.

/area dependency

This commit updates the following: - patchesStrategicMerge -> patches - patchesJson6902 -> patches - vars and varReference -> replacements - bases -> resources Most of this is straight forward, but the vars -> replacements change is a bit complicated. I have taken inspiration from kubebuilder for how to do the change. In particular I changed the name of the secret that holds the certificate to be static. Previously it was set partially from a variable. I believe it would be unnecessarily complicated to keep this behavior and that a static name does not take away much.

k8s-ci-robot · 2023-08-17T11:30:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lentzi90 · 2023-08-17T11:31:23Z

bootstrap/kubeadm/config/certmanager/certificate.yaml

-  - $(SERVICE_NAME).$(SERVICE_NAMESPACE).svc
-  - $(SERVICE_NAME).$(SERVICE_NAMESPACE).svc.cluster.local


While these changes to remove $() are not strictly necessary, they do make it clearer that we do not rely on vars. It is also how kubebuilder has done it.

killianmuldoon

Thanks so much for working on this!

Do you want to also include the Kustomize bump in the Makefile so we can close off that issue?

lentzi90 · 2023-08-17T11:34:16Z

bootstrap/kubeadm/config/certmanager/certificate.yaml

  issuerRef:
    kind: Issuer
    name: selfsigned-issuer
-  secretName: $(SERVICE_NAME)-cert # this secret will not be prefixed, since it's not managed by kustomize
+  secretName: webhook-service-cert # this secret will not be prefixed, since it's not managed by kustomize


This is probably the most controversial change, making the secret name static (not including a variable for the prefix). I think it would be possible to use replacements to keep the exact same behavior, but I'm not sure it is worth it. Happy to discuss it though. For now I went with the static name since this is also what kubebuilder has.

I see 3 options:

static name like proposed here

static name including a prefix to make it more obvious where the secret belongs

replicate the exact behavior of vars but using replacements

lentzi90 · 2023-08-17T11:36:56Z

Thanks so much for working on this!

Do you want to also include the Kustomize bump in the Makefile so we can close off that issue?

Sure, I can do that. I was not sure about the first part of the issue though. Something about creationTimestamp, is that done?

lentzi90 · 2023-08-17T11:39:13Z

Hmm this could prove a bit tricky.

Error: trouble configuring builtin PatchTransformer with config: `
path: cluster-ipv6.yaml
`: unable to parse SM or JSON patch from [---
apiVersion: cluster.x-k8s.io/v1beta1
kind: Cluster
metadata:
  name: '${CLUSTER_NAME}'

I guess the envsubst variables are messing up the kustomize patch. Will need to investigate.

killianmuldoon · 2023-08-17T11:39:15Z

Sure, I can do that. I was not sure about the first part of the issue though. Something about creationTimestamp, is that done?

Yeah - the creationTimestamp issue was a bug which was fixed in controller tools - ref kubernetes-sigs/controller-tools#800

lentzi90 · 2023-08-17T12:33:26Z

The test errors seem to be because of kubernetes-sigs/kustomize#5049. It is possible to work around them by splitting the strategic merge patches into multiple files, but I think the better approach is to just wait for this issue to be resolved. There is work on-going to fix it and if that lands we will not need any additional changes I hope.

/hold

killianmuldoon · 2023-08-17T12:36:29Z

Thanks for digging into the errors! Let's track the status of that issue - if it's fixed relatively soon we can update. . AFAIK we only kustomize for generating test yamls so I'm not sure if we have any real urgency for bumping the version for CAPI.

k8s-ci-robot · 2023-08-31T07:05:53Z

PR needs rebase.

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.

kranurag7 · 2023-12-11T10:54:01Z

Hey, thanks for this update.
I bumped into this PR yesterday.

The issue mentioned above is fixed upstream. I checked out your branch locally and even with the latest kustomize version (v5.0.3), I'm getting the parsing error mentioned above.

It is possible to work around them by splitting the strategic merge patches into multiple files.

I think this still holds true. When I comment out a patch from the files where we have more than one patches, it works.

lentzi90 · 2023-12-11T13:15:33Z

I guess you mean v5.3.0? That is strange 🤔
I will try to get some time to check it this week

kranurag7 · 2023-12-11T13:30:04Z

I guess you mean v5.3.0? That is strange 🤔

Yes, that was a typo. :(
Let me know (slack or here) if I can be helpful. Happy to help in upgrading this.

kranurag7

@lentzi90 sorry for the previous comment, I was using wrong version indeed. I was not able to catch it because I was using direnv in a directory and that updated the PATH.

Your changes looks good.
On this branch, I no longer see warnings w.r.t. bases, vars, patchesStrategicMerge.

$ kustomize build config/default/ > /dev/null
# Warning: 'commonLabels' is deprecated. Please use 'labels' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

The commonLabels is deprecated now and the following is the preferred way to use labels.

-commonLabels:
-  cluster.x-k8s.io/provider: "cluster-api"
+labels:
+- includeSelectors: true
+  pairs:
+    cluster.x-k8s.io/provider: cluster-api

You can use the above pattern or you can also run kustomize edit fix but after running that some more manual changes will be required.

k8s-ci-robot · 2024-01-26T17:34:36Z

@lentzi90: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-e2e-informing-main	`e502aec`	link	false	`/test pull-cluster-api-e2e-informing-main`
pull-cluster-api-e2e-main	`e502aec`	link	true	`/test pull-cluster-api-e2e-main`
pull-cluster-api-e2e-blocking-main	`e502aec`	link	true	`/test pull-cluster-api-e2e-blocking-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

chrischdi · 2024-03-08T08:07:42Z

@lentzi90 , kindly want to ask if you have time to get back on this or should someone else takeover :-)

lentzi90 · 2024-03-08T10:09:07Z

Sorry, I have not had time to get back to this. @peppi-lotta volunteered to take over though 🎉
Hopefully she will be able take this forward next week 🙂

lentzi90 · 2024-03-08T10:09:23Z

/assign @peppi-lotta

k8s-ci-robot · 2024-03-08T10:09:26Z

@lentzi90: GitHub didn't allow me to assign the following users: peppi-lotta.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @peppi-lotta

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.

chrischdi · 2024-03-08T10:58:13Z

Thanks @lentzi90 and @peppi-lotta :-)

killianmuldoon · 2024-03-21T11:15:03Z

Let's close this in favor of #10294

k8s-ci-robot added area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 17, 2023

k8s-ci-robot requested review from chrischdi and elmiko August 17, 2023 11:30

lentzi90 commented Aug 17, 2023

View reviewed changes

killianmuldoon reviewed Aug 17, 2023

View reviewed changes

lentzi90 commented Aug 17, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023

kranurag7 suggested changes Dec 11, 2023

View reviewed changes

peppi-lotta mentioned this pull request Mar 20, 2024

🌱 Update Kustomize deprecated syntax #10294

Merged

killianmuldoon closed this Mar 21, 2024

lentzi90 deleted the lentzi90/kustomize-deprecations branch March 21, 2024 13:41

peppi-lotta mentioned this pull request Apr 25, 2024

🌱 Update Kustomize deprecated syntax metal3-io/cluster-api-provider-metal3#1625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Kustomize: Update deprecated syntax #9223

🌱 Kustomize: Update deprecated syntax #9223

lentzi90 commented Aug 17, 2023 •

edited

Loading

k8s-ci-robot commented Aug 17, 2023

lentzi90 Aug 17, 2023

killianmuldoon left a comment

lentzi90 Aug 17, 2023

lentzi90 commented Aug 17, 2023

lentzi90 commented Aug 17, 2023

killianmuldoon commented Aug 17, 2023

lentzi90 commented Aug 17, 2023

killianmuldoon commented Aug 17, 2023

k8s-ci-robot commented Aug 31, 2023

kranurag7 commented Dec 11, 2023

lentzi90 commented Dec 11, 2023

kranurag7 commented Dec 11, 2023

kranurag7 left a comment

k8s-ci-robot commented Jan 26, 2024

chrischdi commented Mar 8, 2024

lentzi90 commented Mar 8, 2024

lentzi90 commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

chrischdi commented Mar 8, 2024

killianmuldoon commented Mar 21, 2024

		- $(SERVICE_NAME).$(SERVICE_NAMESPACE).svc
		- $(SERVICE_NAME).$(SERVICE_NAMESPACE).svc.cluster.local

🌱 Kustomize: Update deprecated syntax #9223

🌱 Kustomize: Update deprecated syntax #9223

Conversation

lentzi90 commented Aug 17, 2023 • edited Loading

k8s-ci-robot commented Aug 17, 2023

lentzi90 Aug 17, 2023

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

lentzi90 Aug 17, 2023

Choose a reason for hiding this comment

lentzi90 commented Aug 17, 2023

lentzi90 commented Aug 17, 2023

killianmuldoon commented Aug 17, 2023

lentzi90 commented Aug 17, 2023

killianmuldoon commented Aug 17, 2023

k8s-ci-robot commented Aug 31, 2023

kranurag7 commented Dec 11, 2023

lentzi90 commented Dec 11, 2023

kranurag7 commented Dec 11, 2023

kranurag7 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 26, 2024

chrischdi commented Mar 8, 2024

lentzi90 commented Mar 8, 2024

lentzi90 commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

chrischdi commented Mar 8, 2024

killianmuldoon commented Mar 21, 2024

lentzi90 commented Aug 17, 2023 •

edited

Loading