Return InvalidArgument for invalid input entries #5506
Conversation
sorindumitru
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
pkg/server/api/entry/v1/service.go
Outdated
| @@ -658,6 +755,13 @@ func (s *Service) updateEntry(ctx context.Context, e *types.Entry, inputMask *ty | |||
| Hint: inputMask.Hint, | |||
| } | |||
| } | |||
|
|
|||
| if err := validateRegistrationEntryForUpdate(convEntry, mask); err != nil { | |||
There was a problem hiding this comment.
An alternative to this would be to add all the validations here in
Line 92 in 3d158ce
That function already does some validations, but mostly as a necessity of handling errors. It would also simplify things a bit. Let me know if that's better and I can change this.
sorindumitru
force-pushed
the
entry-errors
branch
3 times, most recently
from
a88df74 to
4a907bb
Compare
There was a problem hiding this comment.
Thank you @sorindumitru for this contribution and for the patience in the review.
As discussed in yesterday's sync, I think that having the validations in the datastore layer is probably better, so we make sure that any place in our codebase (as is today, or in the future) is covered by the validation before the data is stored. There shouldn't be a problem to return the proper grpc status code from the datastore layer (we already do that where is applicable).
fixes spiffe#5444 Signed-off-by: Sorin Dumitru <sdumitru@bloomberg.net>
sorindumitru
force-pushed
the
entry-errors
branch
from
4a907bb to
0cf97bb
Compare
| @@ -473,12 +474,11 @@ func (ds *Plugin) CreateOrReturnRegistrationEntry(ctx context.Context, | |||
| func (ds *Plugin) createOrReturnRegistrationEntry(ctx context.Context, | |||
| entry *common.RegistrationEntry, | |||
| ) (registrationEntry *common.RegistrationEntry, existing bool, err error) { | |||
| // TODO: Validations should be done in the ProtoBuf level [https://github.com/spiffe/spire/issues/44] | |||
There was a problem hiding this comment.
I don't think we'll ever do this, so better to remove the TODO.
| if err = validateRegistrationEntry(entry); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I've moved this here because errors withing a withTx block get wrapped into a grpc.Status (I don't know exactly why, maybe it's due to the plugin's history as an actual plugin). Having it in here makes it the handling for create and update be the same. Otherwise one would be wrapped in a grpc Status and one wouldn't so we'd need to handle them differently. I hope this doesn't change observed behaviour by clients too much.
Affected functionality
Returned error codes for BatchCreateEntry/BatchUpdateEntry
Description of change
Return better error codes in case of invalid entries so users can differentiate between entries that can be retried and those that can't
Which issue this PR fixes
fixes #5444