Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove cluster.hosts #4959

Closed
wants to merge 10 commits into from
Closed

Remove cluster.hosts #4959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
db13dc0
Initial attempt on cluster.hosts removal
dio Nov 4, 2018
08f1700
Fix mutability
dio Nov 4, 2018
a534afc
Fix
dio Nov 4, 2018
3a030ad
Fix missing first arg
dio Nov 4, 2018
4674de3
Update filter-example pinned commit hash
dio Nov 5, 2018
215c0ae
Remove cluster.hosts
dio Nov 5, 2018
3e9e1fa
Merge remote-tracking branch 'upstream/master'
dio Nov 5, 2018
b49ac98
Revert required constraint for load_assignment
dio Nov 5, 2018
f12c103
Fix format
dio Nov 5, 2018
055d6d4
cluster_2 should be localhost
dio Nov 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions api/XDS_PROTOCOL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,15 @@ static_resources:
- name: ads_cluster
connect_timeout: { seconds: 5 }
type: STATIC
hosts:
- socket_address:
address: <ADS management server IP address>
port_value: <ADS management server port>
load_assignment:
cluster_name: ads_cluster
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
address: <ADS management server IP address>
port_value: <ADS management server port>
lb_policy: ROUND_ROBIN
http2_protocol_options: {}
admin:
Expand Down
22 changes: 4 additions & 18 deletions api/envoy/api/v2/cds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,34 +156,20 @@ message Cluster {
// when picking a host in the cluster.
LbPolicy lb_policy = 6 [(validate.rules).enum.defined_only = true];

// If the service discovery type is
// :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>`,
// then hosts is required.
//
// .. attention::
//
// **This field is deprecated**. Set the
// :ref:`load_assignment<envoy_api_field_Cluster.load_assignment>` field instead.
//
repeated core.Address hosts = 7 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

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

??? Can you set DNS addresses in load assignment ?
And how long has this been implemented? i.e. was it available in Envoy as of June 2018? The reason I am asking is because we can update this in Istio as well as long as we can support Envoys from around istio 1.0 (which is around june)

Copy link
Member

Choose a reason for hiding this comment

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

For istio-1.0 releases, we already switched to forked Envoy with necessary patches
https://github.com/istio/proxy/blob/release-1.0/WORKSPACE#L38

So I don't think this will affect that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you set DNS addresses in load assignment

I think yes. It is implemented since this: #3864

The relevant discussion probably here: #3261 (comment)

reserved 7; // hosts is deprecated by :ref:`load_assignment
// <envoy_api_field_Cluster.load_assignment>`

// Setting this is required for specifying members of
// :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>` clusters.
// This field supersedes :ref:`hosts<envoy_api_field_Cluster.hosts>` field.
// [#comment:TODO(dio): Deprecate the hosts field and add it to DEPRECATED.md
// once load_assignment is implemented.]
//
// .. attention::
// .. note::
//
// Setting this allows non-EDS cluster types to contain embedded EDS equivalent
// :ref:`endpoint assignments<envoy_api_msg_ClusterLoadAssignment>`.
// Setting this overrides :ref:`hosts<envoy_api_field_Cluster.hosts>` values.
//
ClusterLoadAssignment load_assignment = 33;
ClusterLoadAssignment load_assignment = 33 [(gogoproto.nullable) = false];

// Optional :ref:`active health checking <arch_overview_health_checking>`
// configuration for the cluster. If no
Expand Down
4 changes: 2 additions & 2 deletions ci/build_setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@ if [ "$1" != "-nofetch" ]; then
then
git clone https://github.com/envoyproxy/envoy-filter-example.git "${ENVOY_FILTER_EXAMPLE_SRCDIR}"
fi

# This is the hash on https://github.com/envoyproxy/envoy-filter-example.git we pin to.
(cd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" && git fetch origin && git checkout -f 3e5b73305b961526ffcee7584251692a9a3ce4b3)
(cd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" && git fetch origin && git checkout -f 15f8608a515ed92ce2497f2449d4f2f86b2bea5d)
cp -f "${ENVOY_SRCDIR}"/ci/WORKSPACE.filter.example "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/WORKSPACE
fi

Expand Down
47 changes: 31 additions & 16 deletions configs/envoy_double_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,20 +92,30 @@ static_resources:
type: STATIC
connect_timeout: 0.25s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: 127.0.0.1
port_value: 8125
load_assignment:
cluster_name: statsd
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: 127.0.0.1
port_value: 8125
- name: backhaul
type: STRICT_DNS
connect_timeout: 1s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: front-proxy.yourcompany.net
port_value: 9400
load_assignment:
cluster_name: backhaul
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: front-proxy.yourcompany.net
port_value: 9400
# There are so few connections going back
# that we can get some imbalance. Until we come up
# with a better solution just limit the requests
Expand All @@ -128,11 +138,16 @@ static_resources:
type: LOGICAL_DNS
connect_timeout: 1s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: collector-grpc.lightstep.com
port_value: 443
load_assignment:
cluster_name: lightstep_saas
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: collector-grpc.lightstep.com
port_value: 443
http2_protocol_options: {}
tls_context:
common_tls_context:
Expand All @@ -144,7 +159,7 @@ static_resources:
flags_path: "/etc/envoy/flags"
stats_sinks:
- name: envoy.statsd
config:
config:
tcp_cluster_name: statsd
tracing:
http:
Expand Down
45 changes: 30 additions & 15 deletions configs/envoy_front_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,29 +97,44 @@ static_resources:
type: STRICT_DNS
connect_timeout: 0.25s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: disccovery.yourcompany.net
port_value: 80
load_assignment:
Copy link
Member

Choose a reason for hiding this comment

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

Have you validated all of these modified YAML?

Copy link
Member Author

Choose a reason for hiding this comment

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

I randomly tested them manually. Will have another test session today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, I think @srikailash or @junr03 probably could give some tips on testing these templates here. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

@dio reached out over slack. For the v2 templates I suggest doing what I asked @srikailash to do when we converted them from v1 to v2: render the templates into config files and run them over the validation server, and the config_test

cluster_name: sds
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: discovery.yourcompany.net
port_value: 80
- name: statsd
type: STATIC
connect_timeout: 0.25s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: 127.0.0.1
port_value: 8125
load_assignment:
cluster_name: statsd
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: 127.0.0.1
port_value: 8125
- name: lightstep_saas
type: LOGICAL_DNS
connect_timeout: 1s
lb_policy: ROUND_ROBIN
hosts:
- socket_address:
protocol: TCP
address: collector-grpc.lightstep.com
port_value: 443
load_assignment:
cluster_name: lightstep_saas
endpoints:
- lb_endpoints:
- endpoint:
address:
socket_address:
protocol: TCP
address: collector-grpc.lightstep.com
port_value: 443
http2_protocol_options: {}
{% for service, options in clusters.iteritems() -%}
- {{ helper.internal_cluster_definition(service, options)|indent(2) }}
Expand Down
Loading