Support terminating gateways #503

adilyse · 2020-06-18T00:22:41Z

Following up from #491 after a tragic git accident.

This creates all of the necessary templates to support terminating
gateways in the helm chart. Using terminatingGateways.defaults and
terminatingGateways.gateways, it's possible to define multiple
gateways. Names must be provided for each and they must be unique.

Any gateway-specific values will override the default value with the
exception of annotations. Annotations will apply both the defaults
and gateway-specific annotations.

Note: This is ready for a static code review, but has not been tested
yet because of a bug in the gateway implementation. It's also targeting
the ingress gateway branch for now to make it easier for settings and
tests that overlap between gateway implementations.

Replaces and builds on #452

This creates all of the necessary templates to support terminating gateways in the helm chart. Using `terminatingGateways.defaults` and `terminatingGateways.gateways`, it's possible to define multiple gateways. Names must be provided for each and they must be unique. Any gateway-specific values will override the default value with the exception of annotations. Annotations will apply both the defaults and gateway-specific annotations.

With the addition of an id in the service registration, this is necessary to correctly associate the gateway pod with the specific service instance.

ishustava

This looks awesome!!! Awesome work on everything 🎉

In my testing, I almost didn't find any issues at all, everything worked great! There was a small issue when running with namespaces and ACLs, which I've reported in hashicorp/consul-k8s#279.

I have also left a couple of comments about some missing test cases and made a couple suggestions for the values docs.

ishustava · 2020-06-18T00:51:49Z

test/unit/terminating-gateways-deployment.bats

+#--------------------------------------------------------------------
+# global.tls.enabled
+
+@test "terminatingGateways/Deployment: sets TLS flags when global.tls.enabled" {


Suggested change

@test "terminatingGateways/Deployment: sets TLS flags when global.tls.enabled" {

@test "terminatingGateways/Deployment: sets TLS env variables when global.tls.enabled" {

Updated in all the gateways.

ishustava · 2020-06-18T00:53:21Z

test/unit/terminating-gateways-deployment.bats

+
+  local actual=$(echo $env | jq -r '. | select(.name == "CONSUL_CACERT") | .value' | tee /dev/stderr)
+  [ "${actual}" = "/consul/tls/ca/tls.crt" ]
+}


Could we also add a test that the correct volumes and volumeMounts are present?

There are a series of tests under the extraVolumes test heading below (starting L248 before any changes were made to this file).

Ah, yes those tests are great. I was referring to the TLS volumes for the Consul client, e.g. consul-ca-cert. Sorry, that wasn't clear, but I meant it in the context of global.tls.enabled.

test/unit/terminating-gateways-deployment.bats

ishustava · 2020-06-18T01:27:15Z

values.yaml

+# include both the default annotations and any additional ones defined
+# for a specific gateway.
+# Requirements: consul >= 1.8.0 and consul-k8s >= 0.16.0 if using
+# global.acls.manageSystemACLs.


For the lifecycle-sidecar command

Suggested change

# global.acls.manageSystemACLs.

# global.acls.manageSystemACLs and >= 0.10.0 if not using global.acls.manageSystemACLs.

ishustava · 2020-06-18T01:29:45Z

values.yaml

+# Requirements: consul >= 1.8.0 and consul-k8s >= 0.16.0 if using
+# global.acls.manageSystemACLs.
+terminatingGateways:
+  # Enable terminating gateway deployment. Requires `connectInject.enabled=true`.


Should we inlcude client.enabled and client.grpc as a requirement as well?

Added client.enabled, but client.rpc is enabled by default so leaving it off.

ishustava · 2020-06-18T01:31:02Z

values.yaml

+
+  # Defaults sets default values for all gateway fields. With the exception
+  # of annotations, defining any of these values in the `gateways` list
+  # will override the default values provided here.


I thought it's worth repeating the same thing as you said in the comment for terminatingGateways here for clarity.

Suggested change

# will override the default values provided here.

# will override the default values provided here. Annotations will

# include both the default annotations and any additional ones defined

# for a specific gateway.

ishustava · 2020-06-18T01:31:29Z

values.yaml

+
+    # By default, we set an anti affinity so that two of the same gateway pods
+    # won't be on the same node. NOTE: Gateways require that Consul client agents are
+    # also running on the nodes alongside each gateway Pod.


Suggested change

# also running on the nodes alongside each gateway Pod.

# also running on the nodes alongside each gateway pod.

Fixed on all 3 gateways.

ishustava · 2020-06-18T01:31:57Z

values.yaml

+        memory: "100Mi"
+        cpu: "100m"
+
+    # By default, we set an anti affinity so that two of the same gateway pods


Suggested change

# By default, we set an anti affinity so that two of the same gateway pods

# By default, we set an anti-affinity so that two of the same gateway pods

Fixed on all 3 gateways.

adilyse

Many of these comments apply to things that are present (or lacking) in other components as well. How aggressive do we expect folks to be about fixing things like this across other, unchanged components?

adilyse · 2020-06-18T16:20:45Z

test/unit/terminating-gateways-deployment.bats

+#--------------------------------------------------------------------
+# global.tls.enabled
+
+@test "terminatingGateways/Deployment: sets TLS flags when global.tls.enabled" {


Updated in all the gateways.

test/unit/terminating-gateways-deployment.bats

adilyse · 2020-06-18T16:51:31Z

test/unit/terminating-gateways-deployment.bats

+
+  local actual=$(echo $env | jq -r '. | select(.name == "CONSUL_CACERT") | .value' | tee /dev/stderr)
+  [ "${actual}" = "/consul/tls/ca/tls.crt" ]
+}


There are a series of tests under the extraVolumes test heading below (starting L248 before any changes were made to this file).

test/unit/terminating-gateways-deployment.bats

adilyse · 2020-06-18T17:37:23Z

values.yaml

+# include both the default annotations and any additional ones defined
+# for a specific gateway.
+# Requirements: consul >= 1.8.0 and consul-k8s >= 0.16.0 if using
+# global.acls.manageSystemACLs.


adilyse · 2020-06-18T17:38:17Z

values.yaml

+# Requirements: consul >= 1.8.0 and consul-k8s >= 0.16.0 if using
+# global.acls.manageSystemACLs.
+terminatingGateways:
+  # Enable terminating gateway deployment. Requires `connectInject.enabled=true`.


Added client.enabled, but client.rpc is enabled by default so leaving it off.

adilyse · 2020-06-18T17:40:53Z

values.yaml

+
+  # Defaults sets default values for all gateway fields. With the exception
+  # of annotations, defining any of these values in the `gateways` list
+  # will override the default values provided here.


adilyse · 2020-06-18T17:41:36Z

values.yaml

+        memory: "100Mi"
+        cpu: "100m"
+
+    # By default, we set an anti affinity so that two of the same gateway pods


Fixed on all 3 gateways.

adilyse · 2020-06-18T17:42:23Z

values.yaml

+
+    # By default, we set an anti affinity so that two of the same gateway pods
+    # won't be on the same node. NOTE: Gateways require that Consul client agents are
+    # also running on the nodes alongside each gateway Pod.


Fixed on all 3 gateways.

ishustava

Looks good! Thanks so much for adding tests 🙏

I've left a comment about a test for the TLS volumes/volumeMounts (not the extraVolumes for the gateway, but the volumes for the gateway to talk to the client over TLS). I won't block the approval on that though.

test/unit/terminating-gateways-deployment.bats

ishustava · 2020-06-18T18:09:05Z

test/unit/terminating-gateways-deployment.bats

+
+  local actual=$(echo $env | jq -r '. | select(.name == "CONSUL_CACERT") | .value' | tee /dev/stderr)
+  [ "${actual}" = "/consul/tls/ca/tls.crt" ]
+}


Ah, yes those tests are great. I was referring to the TLS volumes for the Consul client, e.g. consul-ca-cert. Sorry, that wasn't clear, but I meant it in the context of global.tls.enabled.

adilyse added 6 commits June 17, 2020 16:57

Update service registration to remove unnecessary settings

2e4cdf7

Update based on PR feedback, add unique consul service id field

4dca8c4

Add proxy-id flag to consul connect envoy command

dc99419

With the addition of an id in the service registration, this is necessary to correctly associate the gateway pod with the specific service instance.

Add terminating-gateway-name label to all gateway components

15a91f5

Update namespace comments to remove inaccurate outcome

12229e1

adilyse changed the base branch from master to release/0.22.0 June 18, 2020 00:23

ishustava reviewed Jun 18, 2020

View reviewed changes

ishustava added area/gateways Related to gateway components enhancement New feature or request labels Jun 18, 2020

adilyse added 2 commits June 18, 2020 10:36

Update tests based on PR review

6028c7c

Update values file comments based on PR review

c00639e

ishustava mentioned this pull request Jun 18, 2020

Update default Envoy image version and OS #502

Merged

adilyse commented Jun 18, 2020

View reviewed changes

adilyse requested a review from ishustava June 18, 2020 17:56

ishustava approved these changes Jun 18, 2020

View reviewed changes

adilyse merged commit 139ffef into release/0.22.0 Jun 18, 2020

adilyse deleted the gateways-terminating branch June 18, 2020 18:24

adilyse mentioned this pull request Jun 24, 2020

Add bats tests for tls volumes #513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support terminating gateways #503

Support terminating gateways #503

adilyse commented Jun 18, 2020

ishustava left a comment

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

ishustava Jun 18, 2020

adilyse Jun 18, 2020

adilyse left a comment

adilyse Jun 18, 2020

adilyse Jun 18, 2020

adilyse Jun 18, 2020

adilyse Jun 18, 2020

adilyse Jun 18, 2020

adilyse Jun 18, 2020

adilyse Jun 18, 2020

ishustava left a comment

ishustava Jun 18, 2020

	@test "terminatingGateways/Deployment: sets TLS flags when global.tls.enabled" {
	@test "terminatingGateways/Deployment: sets TLS env variables when global.tls.enabled" {

	# global.acls.manageSystemACLs.
	# global.acls.manageSystemACLs and >= 0.10.0 if not using global.acls.manageSystemACLs.

	# also running on the nodes alongside each gateway Pod.
	# also running on the nodes alongside each gateway pod.

	# By default, we set an anti affinity so that two of the same gateway pods
	# By default, we set an anti-affinity so that two of the same gateway pods

Support terminating gateways #503

Support terminating gateways #503

Conversation

adilyse commented Jun 18, 2020

ishustava left a 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

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

adilyse left a 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

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment