Update service defaults #502

thisisnotashwin · 2021-04-22T19:00:40Z

Changes proposed in this PR:

Add support for Mode and UpstreamConfigs to Service Defaults.
Error out in validation if Mode is set as we expect users to use annotations for the same

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

…th checks (#472) remove health checks controller and use endpoints controller for health checks.

- Update the spec of ServiceDefaults and ProxyDefaults to support transparent proxy changes that are introduced as a part of Consul 1.10

kschoche

👍🏽

ishustava

This looks good, Ashwin! The only thing is that I'm not sure that it works because we don't convert UpstreamConfig in the ToConsul function. Maybe having a test that sets all possible values of the service default and checks that it was written to Consul would be good as a validation since acceptance tests also don't set these values?

ishustava · 2021-04-26T21:59:28Z

api/v1alpha1/servicedefaults_types.go

+	// will be ignored if a discovery chain is active.
+	EnvoyClusterJSON string `json:"envoyClusterJSON,omitempty"`
+	// Protocol describes the upstream's service protocol. Valid values are "tcp",
+	// "http" and "grpc". Anything else is treated as tcp. The enables protocol


Suggested change

// "http" and "grpc". Anything else is treated as tcp. The enables protocol

// "http" and "grpc". Anything else is treated as tcp. This enables protocol

ishustava · 2021-04-26T22:01:42Z

api/v1alpha1/servicedefaults_types.go

+	// PassiveHealthCheck configuration determines how upstream proxy instances will
+	// be monitored for removal from the load balancing pool.
+	PassiveHealthCheck *PassiveHealthCheck `json:"passiveHealthCheck,omitempty"`
+	// MeshGatewayConfig controls how Mesh Gateways are configured and used


Suggested change

// MeshGatewayConfig controls how Mesh Gateways are configured and used

// MeshGatewayConfig controls how Mesh Gateways are configured and used.

ishustava · 2021-04-26T22:08:01Z

api/v1alpha1/servicedefaults_types.go

@@ -153,12 +240,20 @@ func (in *ServiceDefaults) Validate(namespacesEnabled bool) error {
 	var allErrs field.ErrorList


should ToConsul function convert UpstreamConfig?

Maybe we should also add a test that sets all possible values set for service defaults and verifies that it gets written to consul.

Yeah..this was definitely a massive oversight by me

api/v1alpha1/servicedefaults_types.go

ishustava · 2021-04-26T22:19:58Z

config/crd/bases/consul.hashicorp.com_servicedefaults.yaml

@@ -80,16 +80,133 @@ spec:
                    description: Mode is the mode that should be used for the upstream connection. One of none, local, or remote.
                    type: string
                type: object
+              mode:
+                description: 'Mode can be one of "direct" or "transparent". "transparent" represents that inbound and outbound application traffic is being captured and redirected through the proxy. This mode does not enable the traffic redirection itself. Instead it signals Consul to configure Envoy as if traffic is already being redirected. "direct" represents that the proxy''s listeners must be dialed directly by the local application and other proxies. Note: This cannot be set using the CRD and should be set using annotations on the services that are part of the mesh.'


It's weird that it added another single quote here to the proxy's and now it's proxy''s Don't know if there's a way to escape that.

yeah..i poked around and couldnt really find a way to prevent that

ah ok, oh well. Thanks for looking!

thisisnotashwin · 2021-04-27T13:48:30Z

@ishustava I completely forgot about to Consul 😞 . The last few changes were all to ensure that things dont get set it Consul that it just slipped my mind. Will fix that up right away!!

thisisnotashwin · 2021-04-27T15:58:13Z

I updated the acceptance test fixture locally with some upstream configs and verified that they got set

not be set via the CRD

ishustava

🎉

ishustava · 2021-04-27T16:01:17Z

config/crd/bases/consul.hashicorp.com_servicedefaults.yaml

@@ -80,16 +80,133 @@ spec:
                    description: Mode is the mode that should be used for the upstream connection. One of none, local, or remote.
                    type: string
                type: object
+              mode:
+                description: 'Mode can be one of "direct" or "transparent". "transparent" represents that inbound and outbound application traffic is being captured and redirected through the proxy. This mode does not enable the traffic redirection itself. Instead it signals Consul to configure Envoy as if traffic is already being redirected. "direct" represents that the proxy''s listeners must be dialed directly by the local application and other proxies. Note: This cannot be set using the CRD and should be set using annotations on the services that are part of the mesh.'


ah ok, oh well. Thanks for looking!

kschoche and others added 3 commits April 22, 2021 15:01

remove health checks controller and use endpoints controller for heal…

6146b10

…th checks (#472) remove health checks controller and use endpoints controller for health checks.

Update ProxyDefaults and ServiceDefaults with Transparent Proxy

097d08a

- Update the spec of ServiceDefaults and ProxyDefaults to support transparent proxy changes that are introduced as a part of Consul 1.10

Add new fields for mode and upstreamConfigs for service defaults

cd685e2

thisisnotashwin force-pushed the update-service-defaults branch from 9db5a5a to cd685e2 Compare April 22, 2021 19:02

thisisnotashwin marked this pull request as ready for review April 22, 2021 19:05

thisisnotashwin requested review from ishustava, a team and kschoche and removed request for a team April 22, 2021 20:59

thisisnotashwin added type/enhancement New feature or request theme/tproxy Items related to transparent proxy labels Apr 26, 2021

thisisnotashwin force-pushed the update-service-defaults branch 2 times, most recently from 329fc0f to a73c9b0 Compare April 26, 2021 17:22

kschoche approved these changes Apr 26, 2021

View reviewed changes

ishustava reviewed Apr 26, 2021

View reviewed changes

thisisnotashwin force-pushed the update-service-defaults branch 2 times, most recently from 170ff02 to 9747805 Compare April 27, 2021 15:06

thisisnotashwin requested a review from ishustava April 27, 2021 15:52

Add documentation to inform users that Transparent Proxy and Mesh should

8daac6a

not be set via the CRD

thisisnotashwin force-pushed the update-service-defaults branch from 9747805 to 8daac6a Compare April 27, 2021 20:21

ishustava approved these changes Apr 27, 2021

View reviewed changes

thisisnotashwin merged commit ecff4b8 into master Apr 28, 2021

thisisnotashwin deleted the update-service-defaults branch April 28, 2021 13:31

ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021

Update default Envoy image version and OS (hashicorp#502)

c05d9a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update service defaults #502

Update service defaults #502

thisisnotashwin commented Apr 22, 2021 •

edited

Loading

kschoche left a comment

ishustava left a comment •

edited

Loading

ishustava Apr 26, 2021

ishustava Apr 26, 2021

ishustava Apr 26, 2021

ishustava Apr 26, 2021

thisisnotashwin Apr 27, 2021

ishustava Apr 26, 2021

thisisnotashwin Apr 27, 2021

ishustava Apr 27, 2021

thisisnotashwin commented Apr 27, 2021

thisisnotashwin commented Apr 27, 2021

ishustava left a comment

ishustava Apr 27, 2021

	// "http" and "grpc". Anything else is treated as tcp. The enables protocol
	// "http" and "grpc". Anything else is treated as tcp. This enables protocol

	// MeshGatewayConfig controls how Mesh Gateways are configured and used
	// MeshGatewayConfig controls how Mesh Gateways are configured and used.

		@@ -153,12 +240,20 @@ func (in *ServiceDefaults) Validate(namespacesEnabled bool) error {
		var allErrs field.ErrorList

Update service defaults #502

Update service defaults #502

Conversation

thisisnotashwin commented Apr 22, 2021 • edited Loading

kschoche left a comment

Choose a reason for hiding this comment

ishustava left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin commented Apr 27, 2021

thisisnotashwin commented Apr 27, 2021

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin commented Apr 22, 2021 •

edited

Loading

ishustava left a comment •

edited

Loading