xds: remove support for v2 Transport API #6013
Conversation
arvindbr8
added
the
Type: Behavior Change
| NodeProto proto.Message | ||
| // NodeProto contains the Node proto to be used in xDS requests. This will be | ||
| // of type *v3corepb.Node. | ||
| NodeProto *v3corepb.Node |
There was a problem hiding this comment.
We should move NodeProto out of ServerConfig and into Config.
NodeProto was part of ServerConfig (even though it contains information about the local node and not the remote management server) because we had to support servers which spoke different transport protocols (v2/v3). Hence we used to marshal the node information in the appropriate version of the NodeProto corresponding to the transport protocol version spoken by the server.
Now that we are going to use v3 for all servers, there is no need to store node information per server. I don't know off the top of my head if this change will be more involved. So, I'm OK if you want to make that change as a follow up PR. If you plan on doing that, please add a TODO here.
There was a problem hiding this comment.
Sure. Actually I think it makes sense to include that in this PR. Adding it in the update
| case version.TransportV3: | ||
| c.XDSServer.NodeProto = v3 | ||
| } | ||
| c.XDSServer.NodeProto = v3 |
There was a problem hiding this comment.
We could completely get rid of this method (or simplify it greatly) if we move the NodeProto out of ServerConfig into Config.
| // TestNewConfigV2ProtoSuccess exercises the functionality in NewConfig with | ||
| // different bootstrap file contents. It overrides the fileReadFunc by returning | ||
| // bootstrap file contents defined in this test, instead of reading from a file. | ||
| func TestNewConfigV2ProtoSuccess(t *testing.T) { |
There was a problem hiding this comment.
Same here. We need to ensure that we don't lose any test coverage.
xds/internal/xdsclient/singleton.go
Outdated
| if node, ok := config.XDSServer.NodeProto.(interface{ GetId() string }); ok { | ||
| nodeID = node.GetId() | ||
| } | ||
| nodeID := config.XDSServer.NodeProto.GetId() |
There was a problem hiding this comment.
Nit: inline this in the log statement.
| @@ -292,6 +261,9 @@ type Config struct { | |||
| // In any of those cases, it is an error if the specified authority is | |||
| // not present in this map. | |||
| Authorities map[string]*Authority | |||
| // NodeProto contains the Node proto to be used in xDS requests. This will be | |||
There was a problem hiding this comment.
Do we need this comment This will be of type *v3corepb.Node.? Given that the field is defined to be of that type now.
| @@ -143,6 +143,9 @@ type Options struct { | |||
| Backoff func(retries int) time.Duration | |||
| // Logger does logging with a prefix. | |||
| Logger *grpclog.PrefixLogger | |||
| // NodeProto contains the Node proto to be used in xDS requests. This will be | |||
| // of type *v3corepb.Node. | |||
There was a problem hiding this comment.
Same comment about the comment here.
arvindbr8
added
Type: Feature
|
Issue states that this is a feature and not a behavior change.. Also need to close and re-open to run new tests in github actions |
We need to remove support for xDS transport API v2 in gRPC. This diff does it.
Fixes #5718
RELEASE NOTES: