Support headers #40

mxpv · 2019-05-24T00:21:55Z

This PR adds headers support to ttrpc.

Signed-off-by: Maksym Pavlenko makpav@amazon.com

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

crosbymichael · 2019-05-24T21:22:42Z

LGTM

crosbymichael · 2019-05-24T21:23:20Z

I tested this and have code to update the namespaces package in containerd to pass the context's namespace to ttrpc services

cpuguy83 · 2019-05-24T22:08:38Z

Just a nit, does it make sense to call this Headers when it's not really a header? Seems like metadata to attach to a request.

re: using this for namespaces.
Why wouldn't the namespace be passed as an argument to the rpc request?

mxpv · 2019-05-24T22:42:12Z

Just a nit, does it make sense to call this Headers when it's not really a header? Seems like metadata to attach to a request.

This can be called in many different ways, but the concept remains the same - send arbitrary data with a request. IMO name 'headers' make the intention of this a bit clearer for a user - pass an extra information with a request, similarly to what http headers are used for.

Why wouldn't the namespace be passed as an argument to the rpc request?

That would work, but you'd need to modify each request to add 'namespace' field to use namespaces.
In containerd there is already a good way to do this:
client, err := containerd.New(address, containerd.WithDefaultNamespace("docker")) <-- all requests from this client use docker ns
or

docker = namespaces.WithNamespace(context, "docker")
containerd, err := client.NewContainer(docker, "id")

So it'd be nice to use the same mechanism for ttrpc as well.

cpuguy83 · 2019-05-24T22:56:31Z

Headers are items that are sent at the beginning of a request. These really aren't headers.
Metadata would be fitting, IMO. Also similar to grpc.

I'm not sure I agree with stuffing data into the protocol that changes the behavior of the rpc endpoint.. but I guess that's not really an argument for this repo.

Being able to attach metadata to a request does make sense though, e.g. for distributed tracing.

crosbymichael · 2019-05-24T23:47:28Z

Changing the name from headers to metadata sounds like a good idea Michael Crosby

…

On May 24, 2019, at 6:56 PM, Brian Goff ***@***.***> wrote: Headers are items that are sent at the beginning of a request. These really aren't headers. Metadata would be fitting, IMO. Also similar to grpc. I'm not sure I agree with stuffing data into the protocol that changes the behavior of the rpc endpoint.. but I guess that's not really an argument for this repo. Being able to attach metadata to a request does make sense though, e.g. for distributed tracing. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

mxpv · 2019-05-25T00:00:05Z

@crosbymichael @cpuguy83 done

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

cpuguy83

LGTM

crosbymichael · 2019-05-29T18:57:04Z

LGTM

stevvooe · 2019-06-13T17:09:46Z

metadata.go

+import "context"
+
+// Metadata represents the key-value pairs (similar to http.Header) to be passed to ttrpc server from a client.
+type Metadata map[string]StringList


This type is not well supported by protobuf.

stevvooe · 2019-06-13T17:11:09Z

metadata_test.go

+	"testing"
+)
+
+func TestMetadata_Get(t *testing.T) {


Please don't use underscores in test names.

stevvooe

NOT LGTM

Sorry for coming in late here, but this needs a few fixes on the protocol serialization/support.

Metadata needs to be defined as part of types.go. That documents the entire protocol. If there are metadata helpers, you can include them in a separate file.
The StringList approach is incompatible with the way grpc represents maps. You really need to have a list of key-value pairs. This does repeat keys, but repeated keys is not a common use case, so its not worth breaking generating compatibility. I'd recommend separating the serialization types from the user types.
GetMetadataValue needs to return multiple strings, so be compatible with grpc's headers, which allow multiple values.
~~4. We should use a model closer to net/http headers than the metadata interface in grpc-go. net/http headers let's you control set vs add, etc.~~ User type looks good here.

We should get another PR on this before we can't go back on it.

stevvooe · 2019-06-13T17:26:10Z

server_test.go

@@ -61,6 +61,7 @@ func (tc *testingClient) Test(ctx context.Context, req *testPayload) (*testPaylo
 type testPayload struct {
 	Foo      string `protobuf:"bytes,1,opt,name=foo,proto3"`
 	Deadline int64  `protobuf:"varint,2,opt,name=deadline,proto3"`
+	Metadata string `protobuf:"bytes,3,opt,name=metadata,proto3"`


I'm not sure this line adds any value.

stevvooe · 2019-06-13T18:19:52Z

#43 covers the concerns here.

mxpv · 2019-06-14T17:27:52Z

#43 covers the concerns here.

@stevvooe a bit late for the party :( Let me know if anything left to address.

full diff: containerd/ttrpc@699c4e4...1fb3814 changes included: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#43 metadata as KeyValue type Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: containerd/ttrpc@699c4e4...92c8520 changes: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#43 metadata as KeyValue type - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#44 Fix method full name generation - containerd/ttrpc#46 Client.Call(): do not return error if no Status is set (gRPC v1.23 and up) - containerd/ttrpc#49 Handle ok status Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: containerd/ttrpc@699c4e4...92c8520 changes: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#43 metadata as KeyValue type - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#44 Fix method full name generation - containerd/ttrpc#46 Client.Call(): do not return error if no Status is set (gRPC v1.23 and up) - containerd/ttrpc#49 Handle ok status Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 8769255d1bb9c469d4f2966e7e9869a9f126f9e9 Component: engine

full diff: containerd/ttrpc@699c4e4...92c8520 changes: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#43 metadata as KeyValue type - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#44 Fix method full name generation - containerd/ttrpc#46 Client.Call(): do not return error if no Status is set (gRPC v1.23 and up) - containerd/ttrpc#49 Handle ok status Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 8769255) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: containerd/ttrpc@699c4e4...92c8520 changes: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#43 metadata as KeyValue type - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#44 Fix method full name generation - containerd/ttrpc#46 Client.Call(): do not return error if no Status is set (gRPC v1.23 and up) - containerd/ttrpc#49 Handle ok status Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 8769255d1bb9c469d4f2966e7e9869a9f126f9e9) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 525e8ed3febff46d07cb01961601824b5f8b301b Component: engine

full diff: containerd/ttrpc@699c4e4...92c8520 changes: - containerd/ttrpc#37 Handle EOF to prevent file descriptor leak - containerd/ttrpc#38 Improve connection error handling - containerd/ttrpc#40 Support headers - containerd/ttrpc#41 Add client and server unary interceptors - containerd/ttrpc#43 metadata as KeyValue type - containerd/ttrpc#42 Refactor close handling for ttrpc clients - containerd/ttrpc#44 Fix method full name generation - containerd/ttrpc#46 Client.Call(): do not return error if no Status is set (gRPC v1.23 and up) - containerd/ttrpc#49 Handle ok status Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>

Support headers

5926a92

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

samuelkarp approved these changes May 24, 2019

View reviewed changes

Rename headers to metadata

04523b9

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>

cpuguy83 approved these changes May 25, 2019

View reviewed changes

cpuguy83 requested a review from crosbymichael May 29, 2019 17:46

crosbymichael merged commit a5bd8ce into containerd:master May 29, 2019

mxpv deleted the headers branch May 29, 2019 19:49

stevvooe reviewed Jun 13, 2019

View reviewed changes

metadata_test.go

"testing"

)

func TestMetadata_Get(t *testing.T) {

Copy link

Member

stevvooe Jun 13, 2019

Choose a reason for hiding this comment

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

Please don't use underscores in test names.

stevvooe reviewed Jun 13, 2019

View reviewed changes

thaJeztah mentioned this pull request Aug 9, 2019

bump containerd and dependencies to v1.3.0 moby/moby#39713

Merged

thaJeztah mentioned this pull request Aug 27, 2019

bump google.golang.org/grpc v1.23.0 (CVE-2019-9512, CVE-2019-9514, CVE-2019-9515) moby/moby#39798

Merged

thaJeztah mentioned this pull request Aug 28, 2019

[19.03 backport] bump google.golang.org/grpc v1.23.0 (CVE-2019-9512, CVE-2019-9514, CVE-2019-9515) docker-archive/engine#340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support headers #40

Support headers #40

mxpv commented May 24, 2019

crosbymichael commented May 24, 2019

crosbymichael commented May 24, 2019

cpuguy83 commented May 24, 2019

mxpv commented May 24, 2019 •

edited

Loading

cpuguy83 commented May 24, 2019

crosbymichael commented May 24, 2019 via email

mxpv commented May 25, 2019

cpuguy83 left a comment

crosbymichael commented May 29, 2019

stevvooe Jun 13, 2019

stevvooe Jun 13, 2019

stevvooe left a comment •

edited

Loading

stevvooe Jun 13, 2019

stevvooe commented Jun 13, 2019

mxpv commented Jun 14, 2019

Support headers #40

Support headers #40

Conversation

mxpv commented May 24, 2019

crosbymichael commented May 24, 2019

crosbymichael commented May 24, 2019

cpuguy83 commented May 24, 2019

mxpv commented May 24, 2019 • edited Loading

cpuguy83 commented May 24, 2019

crosbymichael commented May 24, 2019 via email

mxpv commented May 25, 2019

cpuguy83 left a comment

Choose a reason for hiding this comment

crosbymichael commented May 29, 2019

stevvooe Jun 13, 2019

Choose a reason for hiding this comment

stevvooe Jun 13, 2019

Choose a reason for hiding this comment

stevvooe left a comment • edited Loading

Choose a reason for hiding this comment

stevvooe Jun 13, 2019

Choose a reason for hiding this comment

stevvooe commented Jun 13, 2019

mxpv commented Jun 14, 2019

mxpv commented May 24, 2019 •

edited

Loading

stevvooe left a comment •

edited

Loading