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.

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

credentials: add RequestInfo to context passed to GetRequestMetadata #3057

Merged
merged 12 commits into from Oct 4, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
25 changes: 25 additions & 0 deletions credentials/credentials.go
Expand Up @@ -34,6 +34,7 @@ import (

"github.com/golang/protobuf/proto"
"google.golang.org/grpc/credentials/internal"
ginternal "google.golang.org/grpc/internal"
)

// PerRPCCredentials defines the common interface for the credentials which need to
Expand Down Expand Up @@ -334,3 +335,27 @@ func cloneTLSConfig(cfg *tls.Config) *tls.Config {

return cfg.Clone()
}

// RequestInfo contains request data to be attached to the context passed along to GetRequestMetadata calls. This API is experimental.
Copy link
Member

Choose a reason for hiding this comment

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

  • s/to be//
  • s/along//

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Please put "This API is experimental" on a new line, with a blank comment line between it and the rest of the comments. (And below; see Bundle in this file for an example.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Let's also document on GetRequestMetadata in the PerRPCCredentials interface that the context will contain a RequestInfo retrievable via RequestInfoFromContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Let me know if you would like to change the wording.

type RequestInfo struct {
// The method passed to Invoke or NewStream for this RPC. (For proto methods, this has the format "/some.Service/Method")
Method string
}

// requestInfoKey is a struct to be used as the key when attaching a RequestInfo to a context object.
type requestInfoKey struct{}

// RequestInfoFromContext extracts the RequestInfo from the context. This API is experimental.
func RequestInfoFromContext(ctx context.Context) RequestInfo {
Copy link
Member

Choose a reason for hiding this comment

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

Bah, I missed this before, sorry: it's typical to also return an ok bool here, in case the RequestInfo is not present in ctx. Please add this. The comment can be updated to match peer.FromContext's wording by simply adding "if it exists".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

ri, ok := ctx.Value(requestInfoKey{}).(RequestInfo)
if !ok {
return RequestInfo{}
}
return ri
}

func init() {
ginternal.NewRequestInfoContext = func(ctx context.Context, ri RequestInfo) context.Context {
return context.WithValue(ctx, requestInfoKey{}, ri)
}
}
3 changes: 3 additions & 0 deletions internal/internal.go
Expand Up @@ -47,6 +47,9 @@ var (
// call to proto.Clone(). The returned Status proto should not be mutated by
// the caller.
StatusRawProto interface{} // func (*status.Status) *spb.Status
// NewRequestInfoContext creates a new context based on the argument context attaching
// the passed in RequestInfo to the new context.
NewRequestInfoContext interface{} // func(context.Context, credentials.RequestInfo) context.Context
)

// HealthChecker defines the signature of the client-side LB channel health checking function.
Expand Down
5 changes: 5 additions & 0 deletions internal/transport/http2_client.go
Expand Up @@ -35,6 +35,7 @@ import (

"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/channelz"
"google.golang.org/grpc/internal/syscall"
"google.golang.org/grpc/keepalive"
Expand Down Expand Up @@ -547,6 +548,10 @@ func (t *http2Client) getCallAuthData(ctx context.Context, audience string, call
// streams.
func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Stream, err error) {
ctx = peer.NewContext(ctx, t.getPeer())
ri := credentials.RequestInfo{
Method: callHdr.Method,
}
ctx = internal.NewRequestInfoContext.(func(context.Context, credentials.RequestInfo) context.Context)(ctx, ri)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't consider this initially, but.. This will leave the RequestInfo in the context for the life of the stream. Since RequestInfo is in the credentials package, let's not keep it around except when calling the credentials. If it's needed elsewhere, we might want to move RequestInfo somewhere else. I would say: let's create a new context the same place where createAudience is called, and only pass this to the get*AuthData functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Though I had to remove the unit test that tested via NewStream. So the only test coverage is via the e2e test. That said it appears that is the case for everything in the method that calls createAudience. ¯_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

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

I prefer API-level tests to unit tests for most things, so that's totally fine by me.

headerFields, err := t.createHeaderFields(ctx, callHdr)
if err != nil {
return nil, err
Expand Down
26 changes: 26 additions & 0 deletions internal/transport/transport_test.go
Expand Up @@ -37,6 +37,7 @@ import (
"golang.org/x/net/http2"
"golang.org/x/net/http2/hpack"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/internal/leakcheck"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -1809,3 +1810,28 @@ func TestHeaderTblSize(t *testing.T) {
t.Fatalf("expected len(limits) = 2 within 10s, got != 2")
}
}

// TestRequestInfoFoundInStreamContext tests whether data passed in as callheader data gets
// propagated into the context object as a credentials.RequestInfo object.
func TestRequestInfoFoundInStreamContext(t *testing.T) {
serverConfig := &ServerConfig{}
server, client, cancel := setUpWithOptions(t, 0, serverConfig, suspended, ConnectOptions{})
defer cancel()
defer server.stop()
defer client.Close()

ch := &CallHdr{
Method: "someService/Method",
}
ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Second)
defer ctxCancel()

s, err := client.NewStream(ctx, ch)
if err != nil {
t.Fatalf("client.NewStream() failed: %v", err)
}
ri := credentials.RequestInfoFromContext(s.ctx)
if ch.Method != ri.Method {
t.Fatalf("RequestInfo.Method == %s; want %s", ri.Method, ch.Method)
}
}
27 changes: 27 additions & 0 deletions test/end2end_test.go
Expand Up @@ -7484,3 +7484,30 @@ func parseCfg(s string) serviceconfig.Config {
}
return c
}

type methodTestCreds struct{}

func (m methodTestCreds) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) {
ri := credentials.RequestInfoFromContext(ctx)
return nil, status.Errorf(codes.Unknown, ri.Method)
}

func (m methodTestCreds) RequireTransportSecurity() bool {
return false
}

func (s) TestGRPCMethodAccessibleToCredsViaContextRequestInfo(t *testing.T) {
const wantMethod = "/grpc.testing.TestService/EmptyCall"
ss := &stubServer{}
if err := ss.Start(nil, grpc.WithPerRPCCredentials(methodTestCreds{})); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()

if _, err := ss.client.EmptyCall(ctx, &testpb.Empty{}); status.Convert(err).Message() != wantMethod {
t.Fatalf("ss.client.EmptyCall(_, _) = _, %v; want _, _.Message()=%q", err, wantMethod)
}
}