Object creation with generateName should return AlreadyExists instead of a Timeout

[vincepri]

Signed-off-by: Vince Prignano vincepri@vmware.com

What type of PR is this?

/kind cleanup
/kind api-change
Marking this as api-change given that it's a behavioral change for clients, feel free to remove if it's not appropriate.

What this PR does / why we need it:

This PR solves a long standing issue where we now allow objects being created to have both name and generateName set. If the object already exists, the CheckGeneratedNameError function is called which can then throw a timeout error (500) instead of actually informing the user that the name generated has caused a conflict.

In alternative to the approach proposed in this PR, I've also considered allowing the both generatedName and name to be set, but give precedence to the name; this assumes user intent and we should probably discuss if that's the best way to move forward or not. If this is the case, we can also opt to completely remove the CheckGeneratedNameError like @lavalamp suggested in the related issue.

The current function throws the timeout error in two cases today:

• If the name was generated and it caused a conflict.
• If both generatedName and name are provided by the user and the object already exists.

Which issue(s) this PR fixes:

Fixes #32220

Special notes for your reviewer:

• The current behavior might break a lot of clients out there, I'm happy to consider an option where we keep the current behavior and find a more backward compatible solution.
• The timeout error is very confusing, the underlying code is a 500, which leads users to think there is something wrong with the api-server or underlying infrastructure.

Does this PR introduce a user-facing change?

When creating an object with generateName, if a conflict occurs the server now returns an AlreadyExists error with a retry option.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[vincepri]

/sig api-machinery

[vincepri]

Once we agree on a path forward, I'll make sure to either update or add new tests.

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[dims]

cc @deads2k @liggitt

[fedebongio]

/assign @lavalamp @liggitt
thank you for your comments even before our triage!
/triage accepted

[vincepri]

PTAL

[liggitt]

this looks reasonable to me... will defer lgtm to @lavalamp

[vincepri]

@liggitt One follow-up question, if the user provides both name and generateName the code above is going to use name portion, but the error checking logic is going to ask for a retry. Should we opt to clear generateName when name is present as well?

[liggitt]

Should we opt to clear generateName when name is present as well?

where are you suggesting that be cleared, server-side or client-side?

[vincepri]

Yes, right after

kubernetes/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go

Lines 114 to 116 in 8a9d612

	if len(objectMeta.GetGenerateName()) > 0 && len(objectMeta.GetName()) == 0 {
	objectMeta.SetName(strategy.GenerateName(objectMeta.GetGenerateName()))
	}

— We're already ignoring generateName there if name is set, we could add a branch of that if that sets generateName to an empty string field if name is set.

[liggitt]

I did a quick sweep, and it doesn't look like there are any in-tree places we're setting both name and generateName. I guess I'd want to understand how widespread setting both was for out-of-tree clients, and how dropping the generateName data would impact them.

[vincepri]

@liggitt Yeah that makes sense, truthfully we arrived to this issue because we were setting both GenerateName and Name in Cluster API, which caused the timeout-cryptic error. With the changes above at least the error returned is in line with what a user would expect

[lavalamp]

My only remaining thought is, can we make the message less misleading if the user sets both?

What if we do something like this:

if gn, n := objectMeta.GetGenerateName(), objectMeta.GetName(); len(gn) == 0 || !strings.HasPrefix(n, gn) {

It's not perfect, but this should at least detect cases where user has set obviously conflicting generateName & name?

We can do it in a followup if this is making it too complicated.

[vincepri]

This is a great idea, although we might need more signal that the above comparison, because the generateName can still be a prefix of name. In most cases, users setting both have probably gotten current objects through kubectl or other clients, and then tried to recreate them.

[lavalamp]

Yeah, people in that situation will have to ask themselves, "hm why does the server choose the same name every time??"

I'd fix that too but I think it requires more plumbing and I've already made enough requests on this PR :)

[vincepri]

Yeah, let's merge this as-is and iterate on it as we go?

[lavalamp]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/registry/OWNERS [lavalamp]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [lavalamp]
• staging/src/k8s.io/apiserver/OWNERS [lavalamp]

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