handle generateName as a replacement of name in ObjectMeta v1 #4838

julian7 · 2022-10-20T16:59:47Z

Fixes #641

As per Kubernetes API, ObjectMeta v1 has a "generateName" field which is used by the server as a prefix to make the object's final name from. This field is consumed only when "name" field is not provided. See: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#idempotency

This change makes kyaml understand "generateName", and honoring this field if "name" is not specified.

linux-foundation-easycla · 2022-10-20T16:59:52Z

The committers listed above are authorized under a signed CLA.

✅ login: julian7 / name: Balazs Nagy (09541f2)

k8s-ci-robot · 2022-10-20T16:59:56Z

Welcome @julian7!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-10-20T16:59:58Z

Hi @julian7. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

natasha41575

Fixes #641

Does this PR really provide a complete fix for #641? I can see that it will stop GetValidatedMetadata and GetMeta from throwing errors when a name is missing and it finds a generateName instead, but there are few other issues.

For instance, how does generateName interact with the namePrefix and nameSuffix transformers? What happens when two different resources have the same value for generateName? This fix also only touches kyaml; there are many layers in the api module that expects the name field to be populated and does tracking of the resource based on that.

On the surface, I think the changes in this PR are fine, and probably even quite useful for any third party users of kyaml. But it feels a bit incomplete. It would be helpful to have more examples for kyaml functions that use GetValidatedMetadata and GetMeta to demonstrate where and how this would be used, e.g. in any filters or other external methods. A PR with tests (or even just a comment with a few examples) demonstrating the utility of including GenerateName just in kyaml would be useful, and could potentially be a great first step to fixing #641.

Kustomize, however, is much more than kyaml. For us to say we've "fixed #641", we need a few e2e tests in api/krusty demonstrating that it works correctly in a variety of situations. Getting those tests to pass will likely involve some large changes in the api module, in particular I would expect it to at least involve significant changes to the NameReferenceTransformer among other things. There is ongoing discussion on the best implementation approach in #641, so I would recommend discussing your approach there before working on that PR.

natasha41575 · 2022-10-21T19:14:48Z

kyaml/yaml/rnode.go

@@ -1071,7 +1075,7 @@ func (rn *RNode) GetValidatedMetadata() (ResourceMeta, error) {
 		// A list doesn't require a name.
 		return m, nil
 	}
-	if m.NameMeta.Name == "" {
+	if m.NameMeta.Name == "" && m.NameMeta.GenerateName == "" {
 		return m, fmt.Errorf("missing metadata.name in object %v", m)


nit: we need to change the error message if we do support this

I was more afraid of changing the error message, giving too much hint about generateName :) I'll have a look on it though.

natasha41575 · 2022-10-21T19:15:58Z

kyaml/yaml/rnode_test.go

@@ -406,6 +406,28 @@ func TestRNodeGetValidatedMetadata(t *testing.T) {
 				errMsg: "missing metadata.name",
 			},
 		},
+		"generateNameConfigMap": {


Besides GetMeta and GetValidatedMetadata specifically, does this change impact any other functions or methods in kyaml? If so, we should have a few tests in those areas to demonstrate any behavioral changes.

It shouldn't, in fact, I don't see too much value of generateName outside of its field.

julian7 · 2022-10-21T20:12:34Z

For instance, how does generateName interact with the namePrefix and nameSuffix transformers? What happens when two different resources have the same value for generateName? This fix also only touches kyaml; there are many layers in the api module that expects the name field to be populated and does tracking of the resource based on that.

In generateName's case, there is no way to reference to the name until creation. You can create the same object over and over, all of them will yield a different name:

❯ cat configmap.yml
apiVersion: v1
kind: ConfigMap
metadata:
  generateName: configmap-
data:
  hello: hallo

❯ k apply -f configmap.yml
error: from configmap-: cannot use generate name with apply

❯ k create -f configmap.yml
configmap/configmap-wgtnm created

❯ k create -f configmap.yml
configmap/configmap-gxwp9 created

❯ k create -f configmap.yml
configmap/configmap-pdnjf created

❯ k create -f configmap.yml
configmap/configmap-xkndv created

There's very little I think kustomize can do with this concept. I don't see too much benefit of even namePrefix, let alone nameSuffix in this case.

In my opinion, this generateName is a one-trick pony. I don't have an idea how else it can be useful for kustomize, nevertheless it is invaluable for the only thing it is good for: defining a single-run job on every deployment.

Kustomize, however, is much more than kyaml. For us to say we've "fixed #641", we need a few e2e tests in api/krusty demonstrating that it works correctly in a variety of situations. Getting those tests to pass will likely involve some large changes in the api module, in particular I would expect it to at least involve significant changes to the NameReferenceTransformer among other things. There is ongoing discussion on the best implementation approach in #641, so I would recommend discussing your approach there before working on that PR.

Since the final name of the resource will be determined by the server, I don't see a way to make any references to it, therefore transformers like NameReferenceTransformer aren't, and shouldn't apply.

natasha41575 · 2022-10-21T20:23:53Z

Since the final name of the resource will be determined by the server, I don't see a way to make any references to it, therefore transformers like NameReferenceTransformer aren't, and shouldn't apply.

Kustomize is client-side only. At the very least, for #641, we need to have e2e tests in api/krusty demonstrating that objects containing generateName but no name actually get processed by kustomize without throwing an error. Without that, we can't be confident that kustomize "handles generateName".

natasha41575 · 2022-10-21T20:29:15Z

In generateName's case, there is no way to reference to the name until creation. You can create the same object over and over, all of them will yield a different name

I know what generateName does. I'm saying that we need to define the behavior for how it interacts with name transformers. If a user uses namePrefix, they might expect it to modify the value of generateName, or maybe they expect it to do nothing with generateName. Regardless of what we actually decide to do with the interaction, we should have tests that define the behavior.

julian7 · 2022-10-22T17:49:10Z

I know what generateName does. I'm saying that we need to define the behavior for how it interacts with name transformers. If a user uses namePrefix, they might expect it to modify the value of generateName, or maybe they expect it to do nothing with generateName. Regardless of what we actually decide to do with the interaction, we should have tests that define the behavior.

I don't know. I mean, my gut feeling is that we should better off not applying namePrefix nor nameSuffix for generateName, but I'm open for any suggestion.

k8s-ci-robot · 2022-10-22T20:26:21Z

@julian7: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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 · 2022-10-22T20:26:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: julian7
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval by writing /assign @knverey in a comment. 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

fix issue number

julian7 · 2022-10-28T09:54:15Z

@natasha41575 @mengqiy @phanimarupaka could you please have a look?

natasha41575 · 2022-10-28T19:09:14Z

I think before we can accept this PR, we need to agree on several details. Again, some more things that come to mind:

How generateName interacts with namePrefix/nameSuffix. I see above you suggest that generateName should not interact with namePrefix or nameSuffix. That is a valid opinion, and allowing name transformations on generateName would complicate name references, but I need to think a bit more about pros/cons.
How generateName interacts with patches? How does a patch target such a resource? For example, some options that I can think of:
- Add a new field to the patch targets that allow selection based on the generateName field.
- Keep patch targets as is, and only allow such resources to be targeted by their GVK, labels, and/or annotations.
- We will also need to think about if we should allow the patch to change the value of generateName. Patches are allowed to change the name field, so it may be expected that we eventually support this too.
Same as the above, but with replacements.
What should kustomize do if there are multiple objects with the same generateName? Should kustomize allow this, and if so, how can we differentiate between the identical resource IDs? For example, reswrangler doesn't let you add two resources with the same ID. I haven't looked carefully at your tests yet, but we should make sure that we have tests covering this.
This PR doesn't seem to touch reswrangler or resource at all. That surprises me, as that is where a lot of resource-identifying code is. Maybe you're right that we actually don't need to touch it at all, but I think it would be helpful to take a closer look at that code and see if it makes sense to add support for generateName there.

I plan to bring this PR up with the other kustomize maintainers but if you have thoughts on these points feel free to share here.

@KnVerey can you think of anything else we need to make sure we think about?

Edited to add: I talked with the other maintainers and I think we need to see tests that show the use of generateName with all the other generators/transformers fields of kustomize so that we can see how it would behave.

natasha41575 · 2022-11-02T17:19:15Z

api/filters/nameref/nameref.go

+		f.Referrer.GetGenerateName()); err != nil {
+		if err = checkEqual(
+			"Name", meta.Name, f.Referrer.GetName()); err != nil {
+			return err


Could you explain why you needed to make this change? What behavior does it affect?

Yikes. It seemed to be a good idea at the time. All tests run just fine if you leave this change out. Originally, we had to protect GetName if there was no name, and no generateName. However, RNode's GetName does the check better, to generate a suffix checksum only if GetGenerateName exists.

This needs a force push, this would look awful in git history.

No, I was wrong (while switching back to checkEqual-ing Name only):

--- FAIL: TestGenerateName (0.00s) harness.go:100: node-referrerOriginal 'Name' mismatch '' != 'job-[0a07560513]' --- FAIL: TestBase (0.00s) harness.go:100: node-referrerOriginal 'Name' mismatch '' != 'job-[320078361f]' --- FAIL: TestMidLevelA (0.01s) harness.go:100: node-referrerOriginal 'Name' mismatch '' != 'job-[d66b9a897d]' --- FAIL: TestMidLevelB (0.01s) harness.go:100: node-referrerOriginal 'Name' mismatch '' != 'job-[36d25ef670]' --- FAIL: TestMultibasesNoConflict (0.01s) harness.go:100: node-referrerOriginal 'Name' mismatch '' != 'job-[36d25ef670]'

I remember now just a bit better: if metadata.name is empty, but metadata.generateName is not, rnode.GetName() is returning a name artificially generated (for identification purposes). In cases like this, we should better check metadata.generateName instead.

I enclosed calling rnode.GetName() with rnode.GetGenerateName(), which is rather simple, to save some computation time by not doing a checksum on the rendered resource for this if statement.

k8s-triage-robot · 2023-01-31T22:20:22Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2023-03-02T22:50:43Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

jmorcar · 2023-03-13T17:12:40Z

Sorry is there any update¿?

k8s-triage-robot · 2023-04-21T21:53:38Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-04-21T21:53:42Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

julian7 · 2023-04-22T10:28:10Z

The sad truth is that the more I think about this, the less it makes sense. Apply with prune and force does a much better job.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 20, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 20, 2022

k8s-ci-robot requested review from mengqiy and phanimarupaka October 20, 2022 17:00

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 20, 2022

natasha41575 reviewed Oct 21, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2022

julian7 force-pushed the issue-641 branch from eaf7206 to 7ea56ab Compare October 23, 2022 14:55

julian7 added 4 commits October 28, 2022 11:52

handle generateName in NameMeta

beec1f8

straightforward error for missing name-like metadata

af19b2a

test simple generateName metadata behavior

5f84e6a

fix issue number

distinguish individual resources with generateName

6544629

julian7 force-pushed the issue-641 branch from 7ea56ab to 6544629 Compare October 28, 2022 09:52

rnode: generate name only if there is no name but generateName metadata

f89589e

natasha41575 reviewed Nov 2, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 31, 2023

KnVerey mentioned this pull request Feb 2, 2023

Allow metadata.name or metadata.generateName in GetValidatedMetadata() #4981

Closed

hrobertson mentioned this pull request Feb 3, 2023

Kustomize doesn't support metadata.generateName #641

Open

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 2, 2023

k8s-ci-robot closed this Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle generateName as a replacement of name in ObjectMeta v1 #4838

handle generateName as a replacement of name in ObjectMeta v1 #4838

julian7 commented Oct 20, 2022 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2022 •

edited

Loading

k8s-ci-robot commented Oct 20, 2022

k8s-ci-robot commented Oct 20, 2022

natasha41575 left a comment •

edited

Loading

natasha41575 Oct 21, 2022

julian7 Oct 21, 2022

natasha41575 Oct 21, 2022 •

edited

Loading

julian7 Oct 21, 2022

julian7 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022 •

edited

Loading

julian7 commented Oct 22, 2022

k8s-ci-robot commented Oct 22, 2022

k8s-ci-robot commented Oct 22, 2022

julian7 commented Oct 28, 2022

natasha41575 commented Oct 28, 2022 •

edited

Loading

natasha41575 Nov 2, 2022

julian7 Nov 2, 2022

julian7 Nov 2, 2022

k8s-triage-robot commented Jan 31, 2023

k8s-triage-robot commented Mar 2, 2023

jmorcar commented Mar 13, 2023

k8s-triage-robot commented Apr 21, 2023

k8s-ci-robot commented Apr 21, 2023

julian7 commented Apr 22, 2023

handle generateName as a replacement of name in ObjectMeta v1 #4838

handle generateName as a replacement of name in ObjectMeta v1 #4838

Conversation

julian7 commented Oct 20, 2022 • edited Loading

linux-foundation-easycla bot commented Oct 20, 2022 • edited Loading

k8s-ci-robot commented Oct 20, 2022

k8s-ci-robot commented Oct 20, 2022

natasha41575 left a comment • edited Loading

Choose a reason for hiding this comment

natasha41575 Oct 21, 2022

Choose a reason for hiding this comment

julian7 Oct 21, 2022

Choose a reason for hiding this comment

natasha41575 Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

julian7 Oct 21, 2022

Choose a reason for hiding this comment

julian7 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022

natasha41575 commented Oct 21, 2022 • edited Loading

julian7 commented Oct 22, 2022

k8s-ci-robot commented Oct 22, 2022

k8s-ci-robot commented Oct 22, 2022

julian7 commented Oct 28, 2022

natasha41575 commented Oct 28, 2022 • edited Loading

natasha41575 Nov 2, 2022

Choose a reason for hiding this comment

julian7 Nov 2, 2022

Choose a reason for hiding this comment

julian7 Nov 2, 2022

Choose a reason for hiding this comment

k8s-triage-robot commented Jan 31, 2023

k8s-triage-robot commented Mar 2, 2023

jmorcar commented Mar 13, 2023

k8s-triage-robot commented Apr 21, 2023

k8s-ci-robot commented Apr 21, 2023

julian7 commented Apr 22, 2023

julian7 commented Oct 20, 2022 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2022 •

edited

Loading

natasha41575 left a comment •

edited

Loading

natasha41575 Oct 21, 2022 •

edited

Loading

natasha41575 commented Oct 21, 2022 •

edited

Loading

natasha41575 commented Oct 28, 2022 •

edited

Loading