upstream: add load_assigment field to Cluster #3261
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import "envoy/api/v2/core/health_check.proto"; | |
| import "envoy/api/v2/core/protocol.proto"; | ||
| import "envoy/api/v2/cluster/circuit_breaker.proto"; | ||
| import "envoy/api/v2/cluster/outlier_detection.proto"; | ||
| import "envoy/api/v2/endpoint/endpoint.proto"; | ||
| import "envoy/type/percent.proto"; | ||
|
|
||
| import "google/api/annotations.proto"; | ||
|
|
@@ -150,12 +151,24 @@ message Cluster { | |
| // when picking a host in the cluster. | ||
| LbPolicy lb_policy = 6 [(validate.rules).enum.defined_only = true]; | ||
|
|
||
| // Setting this as a requirement for clusters with discovery type | ||
|
|
||
| // :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`, | ||
| // :ref:`STRICT_DNS<envoy_api_enum_value_Cluster.DiscoveryType.STRICT_DNS>` | ||
| // or :ref:`LOGICAL_DNS<envoy_api_enum_value_Cluster.DiscoveryType.LOGICAL_DNS>` | ||
| // is deprecated. Set the :ref:`endpoints<envoy_api_field_Cluster.endpoints>` field | ||
|
There was a problem hiding this comment. End the sentence after the LOGICAL_DNS reference. And then just "This field is deprecated. Set..." |
||
| // instead. | ||
| repeated core.Address hosts = 7 [deprecated = true]; | ||
|
There was a problem hiding this comment. @htuch what's our policy on docs and deprecation? I'd be inclined to say when we deprecate a field we should update the relevant docs (i.e. updating use of hosts in v2_overview.html) so folks cribbing configuration will be cribbing the new way of doing things and not the deprecated way of doing things. |
||
|
|
||
| // Setting this is required for specifying members of | ||
| // :ref:`STATIC<envoy_api_enum_value_Cluster.DiscoveryType.STATIC>`, | ||
|
There was a problem hiding this comment. Can you update to indicate that the interpretation of theAddress depends on the cluster type. For static, it will be expected to be a direct IP address (or something resolvable by the specified resolver in the Address). For logical/static DNS, it will be expected to be a hostname, resolved via DNS.
This is actually a bit more complicated thinking about it. See the wording at envoy/api/envoy/api/v2/core/address.proto Line 36 in 71c0fd6 Clusters. @mattklein123 WDYT on this? Seems a rough edge in the API.
There was a problem hiding this comment. Yeah I don't know. That's a good question. Should we just say that custom resolvers can't be used at all with DNS discovery types and block it? I don't think it really makes sense and is confusing. If we do that, I think we can have error checking / error messages to guide people in the right direction... There was a problem hiding this comment. This would be reasonable. I think if we had the custom resolvers able to operate async, and some hooks to detect DNS changes, we could replace the hardcoded DNS cluster types with just resolver extensions. But, moving the resolver extension model to be expressive enough and then plumbing this would be a fair chunk of work. |
||
| // :ref:`STRICT_DNS<envoy_api_enum_value_Cluster.DiscoveryType.STRICT_DNS>` | ||
| // or :ref:`LOGICAL_DNS<envoy_api_enum_value_Cluster.DiscoveryType.LOGICAL_DNS>` clusters. | ||
| // This field supersedes :ref:`hosts<envoy_api_field_Cluster.hosts>` field. | ||
| // | ||
| // .. attention:: | ||
| // | ||
| // Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values. | ||
|
There was a problem hiding this comment. I have a higher level API issue I'd like to raise if we're going to add this. From time-to-time, folks wonder if we can provide the full set of EDS capabilities embedded in a CDS response (i.e. in a There was a problem hiding this comment. @htuch @mattklein123 could we keep both of them (( Reason is, I'm not sure if it makes sense: by having A config accommodating endpoints:
lb_endpoints:
- endpoint:
address:
socket_address:
address: localhost1
port_value: 11001
health_check_config:
port_value: 8000Which is not only deep but it also has that There was a problem hiding this comment. Agree with @htuch that this is a good opportunity to fix this inconsistency. In general, I agree with @dio that the lb_endpoints construct is more confusing, however, we also optimize for machine generated config. My feeling is to support lb_endpoints, and just have a good example in the docs of a sample configuration. I think with that it should be pretty straightforward for people to use. There was a problem hiding this comment. Changed to About the docs (e.g. example in getting started etc), should I update it together in this PR or I can do that later? There was a problem hiding this comment. @htuch/@mattklein123 should it be There was a problem hiding this comment. @htuch can we consider this, in EDS context, as a config update ( There was a problem hiding this comment. @dio not sure if I fully grok the question. Are you asking if we can use There was a problem hiding this comment. @htuch yes, I meant to take out the logic from eds and use that for other clusters. I have another question, while refactoring the There was a problem hiding this comment. @dio those assertions are just an artifact of the old code not supporting localities. If you add support the expectations are likely to change. There was a problem hiding this comment. An interesting aspect of this PR is that we now support logical DNS in embedded ClusterLoadAssignments. This didn't use to work, we had a number of questions about this IIRC. |
||
| endpoint.LocalityLbEndpoints endpoints = 31; | ||
|
|
||
| // Optional :ref:`active health checking <arch_overview_health_checking>` | ||
| // configuration for the cluster. If no | ||
|
|
||
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.
It's quite a bit more than just the health check config, even if this was the original rationale :) Might be worth pointing out that CDS assignments can now contained embedded EDS equivalent endpoint assignments.
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.
@htuch yes, In general, I haven't touched the locality yet. Still trying to group some functionalities.