gRPC API: restart as v1 (and use status codes in error returns)

srenatus commented 5 years ago

In our gRPC handlers in api.go, we return errors.New("bad something") in a few places. For the callers, it would be more informative if those errors were "proper" gRPC errors, i.e. contained status codes.

See the docs of google.golang.org/grpc/codes for available codes and their meanings.

srenatus commented 5 years ago

As mentioned in #1501's comments, it might also be nice to have some external API consumption tests here, or in a different PR. Currently, the gRPC API is tested using it's own generated client code here.

I could imagine using some reflection-based client on this, like https://github.com/fullstorydev/grpcurl... 🤔(We' have to enable gRPC reflection first, of course.)

venezia commented 5 years ago

I personally use grpcurl and the sister project, grpcui - but I do have to provide the api.proto file ... adding reflection shouldn't impact the program that much (I typically add reflection myself, but we could also make it dependent on an environmental variable)

I'm willing to work on this as I was personally working on add a ListClients grpc call (I added it, but then realized that it wasn't implemented using k8s as a store, so I'm working on that now) and I personally believe that using the official grpc errors would be best (common errors across all grpc projects)

Furthermore, if we use official grpc errors we might want to re-evaluate the decision to not support grpc-gateway. In conjunction with grpc error codes you will get a reasonably valid HTTP status code For more context see this

There's also some errors in the documentation about the api (probably outdated) - for example dex will not self generate api certificates if they're not defined - it will just work in insecure mode. I can clean that up and some other things.

Also updating to a release version of golang/protobuf (and using a protoc that isn't 2.5 years old) might be nice changes.

Sorry this got long, but I'm willing to help.

srenatus commented 5 years ago

@venezia Thanks for the message, and thanks for offering help! 🎉

Just my thoughts on a few of points (which doesn't mean much more -- I'm curious what everyone thinks!):

adding reflection shouldn't impact the program that much

Yup!

I personally believe that using the official grpc errors would be best (common errors across all grpc projects)

💯

Furthermore, if we use official grpc errors we might want to re-evaluate the decision to not support grpc-gateway.

I'm not sure I follow. I happen to like grpc-gateway and use it in other places. But I don't see how using proper errors means we should expose a different API 😉 I'd rather have one API less, and the gRPC API still seems like a good choice.

There's also some errors in the documentation about the api (probably outdated) - for example dex will not self generate api certificates if they're not defined - it will just work in insecure mode. I can clean that up and some other things.

This would be great! 👍

Also updating to a release version of golang/protobuf (and using a protoc that isn't 2.5 years old) might be nice changes.

Yes yes yes 😉 Please be aware of some changes overlapping this in #1501, which I might just get time to revisit tomorrow.

Sorry this got long, but I'm willing to help.

Thanks again! All help is appreciated 😃

venezia commented 5 years ago

I'm not sure I follow. I happen to like grpc-gateway and use it in other places. But I don't see how using proper errors means we should expose a different API 😉 I'd rather have one API less, and the gRPC API still seems like a good choice.

Well if you use grpc-gateway it will still proxy the rest call into the grpc service - so you're still only supporting one api (or at least one server implementation) - which I think you're familiar with. I personally like grpc a lot, but I think if we want to make the API more accessible to developers we may want to consider offering the rest gateway. It could be marked experimental and should be only enabled with a documented flag, but I think it could help increase interest in making applications that use dex.

Perhaps for more context, I do not know of a web ui for dex administration (perhaps you know of one - but there is none listed in the docs, etc.) - and drive to add a ListClients api call was to facilitate such a UI. I'm comfortable with talking to services through grpc but others may not. So that's why I was suggesting such a thing - to help make dex more accessible to developers who are not yet familiar with grpc.

venezia commented 5 years ago

Blah I missed your point. In the current documentation there is:

While Google APIs, Open API/Swagger, and gRPC Gateway were evaluated, they often became clunky when trying to use specific HTTP error codes or complex request bodies.

I'm suggesting that if we use the official grpc errors it will create reasonable http status codes - which was some of the rationale - sorry for not being clear about why I was linking the two.

srenatus commented 5 years ago

I'm suggesting that if we use the official grpc errors it will create reasonable http status codes - which was some of the rationale

Ah, right. That criticism no longer seems accurate, indeed.

Your argument for making dex more accessible to developers is persuading... 💭 How about that: let's create an issue for this precise thing, adding grpc-gateway, and see how much traction that gets? What do you think?

I suppose a formal RFC process could help, but we're not there yet :wink:

venezia commented 5 years ago

Sure, let's create an issue for grpc-gateway.

Specifically to this ticket, I had a couple questions:

Right some things that would be an error in many APIs are not treated as errors

For example updating a client that doesn't exist returns no error, just a not_found=true message but this is essentially an error (you asked to update something and it doesn't exist) and indeed there is a grpc status code for this

Similar situations exist for creating something that already exists, etc.

Moving these things to status codes would result in a more concise API and the grpc system did not create these status codes for no reason but it would change the API. What used to be an "OK" status with a "not found" message contents would simply become a NOT_FOUND status code.

This results in an API that is probably more consistent with how grpc intended it to be, but would be a change and I'm unsure if we should change the API to make it more correct.

Should we update dex components to have (better) error libraries?

This perhaps is beyond the scope for initial status codes, but is still important when wanting to create better error codes.

For example, in the storage interface, functions like ListRefreshTokens() and ListClients() are declared, but the kubernetes implementation just tosses up a generic error return nil, errors.New("not implemented") A more specific error like "notImplemented" could be added and used, similar to what exists for not found declared here

The point is that there are grpc error codes for things like Unimplemented that would be a more useful error message then the generic Internal

What do people think?

srenatus commented 5 years ago

Thank you, these are good issues to think about, thanks for bringing them up! 😃

One approach we could take is keeping what we have right now untouched, and adding a new GRPC API, and calling it v1 (or some more fully-qualified name, whatever the convention is). That way, who wants the better errors (including NotFound as you mentioned) can opt into it by using the new API, but we'd not break anybody's stuff for those using the existing API. What do you think?

venezia commented 5 years ago

I think that makes sense - would we probably could do it within the same proto file - just a different service - right now you have Dex, perhaps we create DexV{n} service in the api.proto file and have both of them being served by dex, perhaps noting that the original service is deprecated or something.

Something like:

In api.proto something like:

// Dex represents the dex gRPC service.
service Dex {
  // CreateClient creates a client.
  rpc CreateClient(CreateClientReq) returns (CreateClientResp) {};
...
}

// DexV1 represents the dex v1 gRPC service.
service DexV1 {
  // CreateClient creates a client.
  rpc CreateClient(CreateClientReq) returns (CreateClientV1Resp) {};
...
}

In cmd/dex/serve.go something like:

api.RegisterDexServer(s, server.NewAPI(serverConfig.Storage, logger))
api.RegisterDexV1Server(s, server.NewAPIV1(serverConfig.Storage, logger))

The only thing I'm unsure of is "Does Dex and DexV1" seem somewhat too similar. Sometimes saying "v2" even when a v1 never declared v1 but always was essentially implied as v1, might be more clear to the user

Thoughts?

srenatus commented 5 years ago

Hmm I think I'd prefer if we used this chance to adhere to common approaches, and standards where they exist, here. So instead of adding to dex/api.proto, we could create a new dex/v1/api.proto and have its package directive read package dex.v1 (or v2, I don't mind either way)....

💡This also could be an opportunity to revisit method names where they are confusing. I'm thinking about the Password-related methods that are for (dex-internal) users.

Please don't feel pressured into this bigger piece of work -- I'm happy to help, and we can approach this piecewise. In the end, we'll have a better v1 gRPC API that is enjoyable to use 😉 It's probably a good idea to rename this issue, and re-purpose it for the new API. (OTOH, while I think this is a valuable enhancement, I'm very likely not able to get this done alone in a timely manner.)

Should we update dex components to have (better) error libraries?

That's a good idea. I think by introducing error constants, we can make this happen

without changing the existing gRPC API responses, and still
having a chance to translate them into more descriptive status codes in the v1 (or v2) API.

venezia commented 5 years ago

Sure that seems just fine - I do feel bad that I originally said "Sure I'll help add grpc status codes" and now we're at "Hey, let's create a new api" because it seems like a lot of scope creep ... but at the same time I think it does make sense to revisit things and say "If we could redo this over again, how would we change that grpc api to be more best-practices"

We probably should split up the efforts though - at least to make it less waterfall-y. Error constants to improve packages like the storage package/interface can be done independently of a better grpc api. Similarly adding grpc gateway or reflection can also be done in parallel.

Perhaps we should have an issue documenting what we think the right messages and rpc's should be.
You mention the Password-releated methods, but changing them would be more than an API change - the storage interface calls them Password and that would mean a bit more work to rename it (but shouldn't be that difficult) ... the documentation suggests that the storage interface isn't stable yet, so perhaps changing things now is OK (side thought, could we ever add claims to this construct? I know its silly but it would seem to give it more parity)

I don't mind doing a fair bit of work on dex - I just want to help update it, etc.

srenatus commented 5 years ago

Cool, reflection is done ✔️

I've rename the issue to track our "proper new version" plan that shines through the discussion above. It's -- in total -- not a low-hanging-fruit; but each small step could be:

[x] add gRPC reflection
[ ] change the storage interface to return error constants or types
- I'd prefer types, following the naming convention for them (type FooError struct { /* ... */ })
[ ] add a stub api/v1/dex.proto file, wire it into the build and CI process
[ ] start porting existing methods:
- fixing the error returns
- eventually changing a method name or two to be less confusing

@venezia what do you think?

venezia commented 5 years ago

As far as the newer api, I worked some on a v1alpha1 api - you can see it https://github.com/venezia/dex/tree/api_v1alpha1 - more to go obviously, but its a stab at it.

venezia commented 5 years ago

Can we create a separate ticket for tracking the storage interface error change? There's a lot of providers and it might be easier to create something like:

[ ] Create storage class error types/variables
[ ] For each storage provider:
- [ ] Update interface methods to return interface errors when appropriate
- [ ] Declare and use provider-specific errors if and when appropriate

Also as far as testing - do people have any aversion to something like ginkgo? I just found it easy to use and it obviously works alongside basic tests (go test ./... still works)

Thanks

srenatus commented 5 years ago

Can we create a separate ticket for tracking the storage interface error change?

Sure! 😃

Also as far as testing - do people have any aversion to something like ginkgo?

So far, I've appreciated the minimal approach dex has taken when it comes to testing, mostly relying on the stdlib (and testify in a few recently-touched places). What extra value does ginkgo bring that you'd rather not do without? 😉

venezia commented 5 years ago

So far, I've appreciated the minimal approach dex has taken when it comes to testing, mostly relying on the stdlib (and testify in a few recently-touched places). What extra value does ginkgo bring that you'd rather not do without? 😉

Obviously anything ginkgo can do you can ultimately do with go test, and Testify isn't bad either - I think Testify is better than standard go test. Testify gives you suites which is nice as well, etc.

What I liked about Ginkgo (and BDD in general, for example goblin as another BDD framework in go) is that the tests were clear to anyone (especially people unfamiliar with the project) to understand what was being tested and why. Tests are somewhat constructed in English-readable sentences and that might make it more enticing (or at least perhaps lower the barrier of entry) for people to create / maintain tests.

Like I might be aware of what is going on in kubernetes' storage tests - but is it really clear to the developer why these were the tests chosen or how to place additional tests?

Also gomega is a pretty good assertion library, especially with async assertions, etc.

Its just my opinion so I'm fine continuing with what we've got, but I figured I could ask.

venezia commented 5 years ago

change the storage interface to return error constants or types I'd prefer types, following the naming convention for them (type FooError struct { /* ... */ })

@srenatus Can you look at this branch - I moved to an error type (storage.Error) with some error codes (storage.ErrorCode) and implemented this within the kubernetes storage provider.

The kubernetes storage provider is a bit gnarly (I personally feel that having both client and Client as types in kubernetes is a bit painful) so I tried to stuff things in more logical places (existing storage.go file only contained storage.Storage interface methods, client.go no longer had funcs/structs that weren't client - but there's still some things that are somewhat haphazardly located, move methods into the order specified by the interface, etc., etc.

Essentially we have

type Error struct {
    Code    ErrorCode
    Details string
}

which will let us be able to allow consumers to make more informed decisions about the errors.

Before I implement this across other storage providers, I wanted feedback, etc. Let me know what you think.

Thanks

srenatus commented 5 years ago

Thanks for working on this. As you might notice, I'm having a bit of a hard time keeping up here 😅

I haven't looked at the k8s changes, but the error struct bits look solid.

I think I'd propose changing the type handling a bit here:

type errorCode string // private

const (
    ErrNotFound errorCode = "not found" // exported
    ErrAlreadyExists      = "already exists"
    // ...
)

that way, storage implementations shouldn't be able to make up new error codes, but would be restricted to using the constants defined here.

However, we can't have a simple IsErrorCode function then since it's not an exported type... 🤔 I think your approach is good 👍

dexidp / dex

gRPC API: restart as v1 (and use status codes in error returns) #1499

Right some things that would be an error in many APIs are not treated as errors

Should we update dex components to have (better) error libraries?