-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
xds: set dial target in request resource_names #3081
xds: set dial target in request resource_names #3081
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the changed, although I don't seem to understand why we are doing these changes. Could you please expand the commit message to give some details. Thanks.
PR description updated. |
node: &basepb.Node{ | ||
Metadata: &structpb.Struct{ | ||
Fields: map[string]*structpb.Value{ | ||
internal.GrpcHostname: { | ||
Kind: &structpb.Value_StringValue{StringValue: serviceName}, | ||
}, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will LRS be eventually moved to the xdsClient and will a nodeProto (read from the bootstrap file) be added to the request there? If not, is OK to not have a nodeProto at all in the LRS request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, LRS will be moved to the same xdsClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a TODO to say that we should add the NodeProto here once we move it to the xdsClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
And stop sending metadata
TRAFFICDIRECTOR_GRPC_HOSTNAME
.Similarly, for LRS, cluster_name is set to the dial target.
The expected resource_names can be in a different format from dial target. (test_service.test_namespace.traffic_director.com vs test_namespace:test_service). The mapping will be done by LDS/RDS/VHDS + CDS.
The workaround was to leave resource_names empty, and send dial target as metadata
TRAFFICDIRECTOR_GRPC_HOSTNAME
.This change sets dial target directly in resource_names, and removes metadata. The server don't need to read the metadata for service name, but needs to support two formats of resrouce_names (test_service.test_namespace.traffic_director.com and test_namespace:test_service).
When all the missing pieces are done, resource_names will be set as the cluster names from CDS response.
For LRS, cluster_name will be set to the same name.