Clean up protos #138
Clean up protos #138
Conversation
rmehta19
force-pushed
the
merge-proto
branch
2 times, most recently
from
e45b501 to
e4bf9d3
Compare
| @@ -65,6 +66,14 @@ func (m *singleTokenAccessTokenManager) DefaultToken() (string, error) { | |||
| } | |||
|
|
|||
| // Token always returns the token managed by the singleTokenAccessTokenManager. | |||
| func (m *singleTokenAccessTokenManager) Token(*commonpb.Identity) (string, error) { | |||
| func (m *singleTokenAccessTokenManager) Token(identity interface{}) (string, error) { | |||
There was a problem hiding this comment.
Some notes:
- can't use generics here: https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#no-parameterized-methods
- can't use overloading: https://go.dev/doc/faq#overloading
Other option: have two methods: Token(*commonpbv1.Identity) and TokenV2(*commonpb.Identity)
| @@ -53,3 +53,25 @@ enum AlpnProtocol { | |||
| ALPN_PROTOCOL_HTTP2 = 2; | |||
| ALPN_PROTOCOL_HTTP1_1 = 3; | |||
| } | |||
|
|
|||
| message Identity { | |||
There was a problem hiding this comment.
It looks like we have duplicate copies of the Identity message - is that intentional?
There was a problem hiding this comment.
I wonder if it makes sense to move the Identity here, instead of creating a new copy of definition?
There was a problem hiding this comment.
Yes -- duplicating Identity messsage was intentional in common.proto and v2/common.proto, as discussed offline a few month's ago:
"we could also add the Identity message to the V2 files...combining the two common.proto files into one is probably infeasible, I think, because the enums are incompatible". This change aligns the v2 protos with the copy in grpc-proto being used by the S2A Java client: grpc/grpc-proto#149
Re: I wonder if it makes sense to move the Identity here, instead of creating a new copy of definition?
I think this would be possible. We could create
identity.protowhich just contains theIdentitymessage, and this could be used by both v1 and v2. IIRC, the goal as to not share protos between v1 and v2, but if we are ok withidentity.protobeing shared between the two, I don't see a problem.
There was a problem hiding this comment.
Thanks! I had forgotten the details of this discussion. :)
We have a script for proto generation at: Can we use that script and apply any updates if needed? thanks |
Done -- thanks for pointing this out! Latest commit contains result of running script. The only changes are to the example protos. |
internal/record/ticketsender_test.go
Outdated
| default: | ||
| return "", fmt.Errorf("Incorrect identity type: %v", v) | ||
| } | ||
| if identity == nil || cmp.Equal(identity, &commonpbv1.Identity{}, protocmp.Transform()) { |
There was a problem hiding this comment.
Do we need a cmp.Equal check against &commonpb.Identity{} as well, or this alone is sufficient?
There was a problem hiding this comment.
Thanks for catching this -- done!
s2a.go
Outdated
| var marshalledLocalIdentities [][]byte | ||
| for _, localIdentity := range localIdentities { | ||
| marshalledLocalIdentity, err := proto.Marshal(localIdentity) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| marshalledLocalIdentities = append(marshalledLocalIdentities, marshalledLocalIdentity) | ||
| } | ||
| var v2LocalIdentities []*commonpb.Identity | ||
| for _, marshalledLocalIdentity := range marshalledLocalIdentities { | ||
| v2LocalIdentity := &commonpb.Identity{} | ||
| err := proto.Unmarshal(marshalledLocalIdentity, v2LocalIdentity) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| v2LocalIdentities = append(v2LocalIdentities, v2LocalIdentity) | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you think it would be better to have a separate toProtoIdentity() function for the v2 identity? instead of doing the marshal & un-marshal conversion.
My understanding is that at some point in the future, the v1/legacy code can be removed, and the marshal/unmarshal logic will need to be replaced anyway. wdyt?
There was a problem hiding this comment.
My understanding is the same as yours. I have added a new toV2ProtoIdentity function in s2a_options.go, and replaced the marshal/unmarshal logic with this.
Summary of changes:
[1]: example commands to regenerate protos (for reference):