Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Object creation with generateName should return AlreadyExists instead of a Timeout #104699

Merged
merged 1 commit into from Sep 8, 2021
Merged

Object creation with generateName should return AlreadyExists instead of a Timeout #104699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/registry/core/service/storage/rest.go
View file
Open in desktop
Expand Up @@ -247,7 +247,7 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation

out, err := rs.services.Create(ctx, service, createValidation, options)
if err != nil {
err = rest.CheckGeneratedNameError(rs.strategy, err, service)
err = rest.CheckGeneratedNameError(ctx, rs.strategy, err, service)
}

if err == nil {
Expand Down
10 changes: 7 additions & 3 deletions pkg/registry/core/service/strategy_test.go
View file
Open in desktop
Expand Up @@ -48,19 +48,23 @@ func newStrategy(cidr string, hasSecondary bool) (testStrategy Strategy, testSta
}

func TestCheckGeneratedNameError(t *testing.T) {
ctx := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &genericapirequest.RequestInfo{
Resource: "foos",
})

testStrategy, _ := newStrategy("10.0.0.0/16", false)
expect := errors.NewNotFound(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
if err := rest.CheckGeneratedNameError(ctx, testStrategy, expect, &api.Service{}); err != expect {
t.Errorf("NotFoundError should be ignored: %v", err)
}

expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
if err := rest.CheckGeneratedNameError(ctx, testStrategy, expect, &api.Service{}); err != expect {
t.Errorf("AlreadyExists should be returned when no GenerateName field: %v", err)
}

expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsServerTimeout(err) {
if err := rest.CheckGeneratedNameError(ctx, testStrategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsAlreadyExists(err) {
t.Errorf("expected try again later error: %v", err)
}
}
Expand Down
19 changes: 19 additions & 0 deletions staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go
View file
Open in desktop
Expand Up @@ -170,6 +170,25 @@ func NewAlreadyExists(qualifiedResource schema.GroupResource, name string) *Stat
}}
}

// NewGenerateNameConflict returns an error indicating the server
// was not able to generate a valid name for a resource.
func NewGenerateNameConflict(qualifiedResource schema.GroupResource, name string, retryAfterSeconds int) *StatusError {
return &StatusError{metav1.Status{
Status: metav1.StatusFailure,
Code: http.StatusConflict,
Reason: metav1.StatusReasonAlreadyExists,
Details: &metav1.StatusDetails{
Group: qualifiedResource.Group,
Kind: qualifiedResource.Resource,
Name: name,
RetryAfterSeconds: int32(retryAfterSeconds),
},
Message: fmt.Sprintf(
"%s %q already exists, the server was not able to generate a unique name for the object",
qualifiedResource.String(), name),
}}
}

// NewUnauthorized returns an error indicating the client is not authorized to perform the requested
// action.
func NewUnauthorized(reason string) *StatusError {
Expand Down
9 changes: 8 additions & 1 deletion staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go
View file
Open in desktop
Expand Up @@ -64,7 +64,7 @@ func TestErrorNew(t *testing.T) {
}

if !IsConflict(NewConflict(resource("tests"), "2", errors.New("message"))) {
t.Errorf("expected to be conflict")
t.Errorf("expected to be %s", metav1.StatusReasonAlreadyExists)
}
if !IsNotFound(NewNotFound(resource("tests"), "3")) {
t.Errorf("expected to be %s", metav1.StatusReasonNotFound)
Expand All @@ -88,6 +88,13 @@ func TestErrorNew(t *testing.T) {
t.Errorf("expected to be %s", metav1.StatusReasonMethodNotAllowed)
}

if !IsAlreadyExists(NewGenerateNameConflict(resource("tests"), "3", 1)) {
t.Errorf("expected to be %s", metav1.StatusReasonAlreadyExists)
}
if time, ok := SuggestsClientDelay(NewGenerateNameConflict(resource("tests"), "3", 1)); time != 1 || !ok {
t.Errorf("unexpected %d", time)
}

if time, ok := SuggestsClientDelay(NewServerTimeout(resource("tests"), "doing something", 10)); time != 10 || !ok {
t.Errorf("unexpected %d", time)
}
Expand Down
4 changes: 2 additions & 2 deletions staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go
View file
Open in desktop
Expand Up @@ -404,7 +404,7 @@ func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation
out := e.NewFunc()
if err := e.Storage.Create(ctx, key, obj, out, ttl, dryrun.IsDryRun(options.DryRun)); err != nil {
err = storeerr.InterpretCreateError(err, qualifiedResource, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
err = rest.CheckGeneratedNameError(ctx, e.CreateStrategy, err, obj)
if !apierrors.IsAlreadyExists(err) {
return nil, err
}
Expand Down Expand Up @@ -659,7 +659,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
}
if creating {
err = storeerr.InterpretCreateError(err, qualifiedResource, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, creatingObj)
err = rest.CheckGeneratedNameError(ctx, e.CreateStrategy, err, creatingObj)
} else {
err = storeerr.InterpretUpdateError(err, qualifiedResource, name)
}
Expand Down
18 changes: 15 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/admission"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/features"
"k8s.io/apiserver/pkg/storage/names"
utilfeature "k8s.io/apiserver/pkg/util/feature"
Expand Down Expand Up @@ -109,6 +110,7 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx context.Context, obj runtime.
objectMeta.SetDeletionGracePeriodSeconds(nil)
strategy.PrepareForCreate(ctx, obj)
FillObjectMetaSystemFields(objectMeta)

if len(objectMeta.GetGenerateName()) > 0 && len(objectMeta.GetName()) == 0 {
objectMeta.SetName(strategy.GenerateName(objectMeta.GetGenerateName()))
}
Expand Down Expand Up @@ -145,21 +147,31 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx context.Context, obj runtime.

// CheckGeneratedNameError checks whether an error that occurred creating a resource is due
// to generation being unable to pick a valid name.
func CheckGeneratedNameError(strategy RESTCreateStrategy, err error, obj runtime.Object) error {
func CheckGeneratedNameError(ctx context.Context, strategy RESTCreateStrategy, err error, obj runtime.Object) error {
if !errors.IsAlreadyExists(err) {
return err
}

objectMeta, kind, kerr := objectMetaAndKind(strategy, obj)
objectMeta, gvk, kerr := objectMetaAndKind(strategy, obj)
if kerr != nil {
return kerr
}

if len(objectMeta.GetGenerateName()) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// If we don't have a generated name, return the original error (AlreadyExists).
// When we're here, the user picked a name that is causing a conflict.
return err
}

return errors.NewServerTimeoutForKind(kind.GroupKind(), "POST", 0)
// Get the group resource information from the context, if populated.
gr := schema.GroupResource{}
if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
gr = schema.GroupResource{Group: gvk.Group, Resource: requestInfo.Resource}
}

// If we have a name and generated name, the server picked a name
// that already exists.
return errors.NewGenerateNameConflict(gr, objectMeta.GetName(), 1)
}

// objectMetaAndKind retrieves kind and ObjectMeta from a runtime object, or returns an error.
Expand Down