From 4955830fe020dfa87c8d02507ddd1892ae1cf672 Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Thu, 2 Mar 2023 01:21:27 +0000 Subject: [PATCH 1/4] backport of commit e14b4301fad9b41a7fabda0e3b1f00fb1ca09995 --- agent/proxycfg/terminating_gateway.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/agent/proxycfg/terminating_gateway.go b/agent/proxycfg/terminating_gateway.go index cb371ae2bf0..483c79f91dd 100644 --- a/agent/proxycfg/terminating_gateway.go +++ b/agent/proxycfg/terminating_gateway.go @@ -354,8 +354,13 @@ func (s *handlerTerminatingGateway) handleUpdate(ctx context.Context, u UpdateEv // There should only ever be one entry for a service resolver within a namespace if resolver, ok := resp.Entry.(*structs.ServiceResolverConfigEntry); ok { snap.TerminatingGateway.ServiceResolvers[sn] = resolver + snap.TerminatingGateway.ServiceResolversSet[sn] = true + } else { + // we likely have a deleted service resolver, and our cast is a nil + // cast, so clear this out + delete(snap.TerminatingGateway.ServiceResolvers, sn) + snap.TerminatingGateway.ServiceResolversSet[sn] = false } - snap.TerminatingGateway.ServiceResolversSet[sn] = true case strings.HasPrefix(u.CorrelationID, serviceIntentionsIDPrefix): resp, ok := u.Result.(structs.Intentions) From 391d8442fdfd0e8a963a018482154d160eadf09a Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Thu, 2 Mar 2023 20:40:27 +0000 Subject: [PATCH 2/4] backport of commit 525501337d949fc1110a21f080cc2c1854ffc761 --- .../capture.sh | 4 ++ .../service_gateway.hcl | 9 ++++ .../service_s2-v1.hcl | 9 ++++ .../service_s2-v2.hcl | 9 ++++ .../setup.sh | 41 +++++++++++++++ .../vars.sh | 8 +++ .../verify.bats | 50 +++++++++++++++++++ test/integration/connect/envoy/helpers.bash | 18 ++++++- 8 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh create mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh new file mode 100644 index 00000000000..2ef0c41a215 --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh @@ -0,0 +1,4 @@ +#!/bin/bash + +snapshot_envoy_admin localhost:20000 terminating-gateway primary || true +snapshot_envoy_admin localhost:19000 s1 primary || true diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl new file mode 100644 index 00000000000..fe597c235cf --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl @@ -0,0 +1,9 @@ +services { + name = "terminating-gateway" + kind = "terminating-gateway" + port = 8443 + + meta { + version = "v1" + } +} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl new file mode 100644 index 00000000000..fb87dc41d5e --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl @@ -0,0 +1,9 @@ +services { + id = "s2-v1" + name = "s2" + port = 8182 + + meta { + version = "v1" + } +} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl new file mode 100644 index 00000000000..52ab0abd684 --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl @@ -0,0 +1,9 @@ +services { + id = "s2-v2" + name = "s2" + port = 8183 + + meta { + version = "v2" + } +} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh new file mode 100644 index 00000000000..fdd49572ba8 --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh @@ -0,0 +1,41 @@ +#!/bin/bash + +set -euo pipefail + +upsert_config_entry primary ' +kind = "terminating-gateway" +name = "terminating-gateway" +services = [ + { + name = "s2" + } +] +' + +upsert_config_entry primary ' +kind = "proxy-defaults" +name = "global" +config { + protocol = "http" +} +' + +upsert_config_entry primary ' +kind = "service-resolver" +name = "s2" +default_subset = "v1" +subsets = { + "v1" = { + filter = "Service.Meta.version == v1" + } + "v2" = { + filter = "Service.Meta.version == v2" + } +} +' + +register_services primary + +# terminating gateway will act as s2's proxy +gen_envoy_bootstrap s1 19000 +gen_envoy_bootstrap terminating-gateway 20000 primary true diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh new file mode 100644 index 00000000000..9e52629b8be --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh @@ -0,0 +1,8 @@ +#!/bin/bash + +# There is no sidecar proxy for s2-v1, since the terminating gateway acts as the proxy +export REQUIRED_SERVICES=" +s1 s1-sidecar-proxy +s2-v1 +terminating-gateway-primary +" diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats new file mode 100644 index 00000000000..c30b920c281 --- /dev/null +++ b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats @@ -0,0 +1,50 @@ +#!/usr/bin/env bats + +load helpers + +@test "s1 proxy admin is up on :19000" { + retry_default curl -f -s localhost:19000/stats -o /dev/null +} + +@test "terminating proxy admin is up on :20000" { + retry_default curl -f -s localhost:20000/stats -o /dev/null +} + +@test "terminating-gateway-primary listener is up on :8443" { + retry_default nc -z localhost:8443 +} + +@test "s1 proxy listener should be up and have right cert" { + assert_proxy_presents_cert_uri localhost:21000 s1 +} + +@test "s1 upstream should have healthy endpoints for v1.s2" { + assert_upstream_has_endpoints_in_status 127.0.0.1:19000 v1.s2 HEALTHY 1 +} + +@test "terminating-gateway should have healthy endpoints for v1.s2" { + assert_upstream_has_endpoints_in_status 127.0.0.1:20000 v1.s2 HEALTHY 1 +} + +@test "terminating-gateway should have healthy endpoints for v2.s2" { + assert_upstream_has_endpoints_in_status 127.0.0.1:20000 v2.s2 HEALTHY 1 +} + +@test "deleting the service-resolver should be possible" { + delete_config_entry service-resolver s2 +} + +@test "terminating gateway should no longer have v1.s2 endpoints" { + assert_upstream_missing 127.0.0.1:20000 v1.s2 +} + +@test "terminating gateway should no longer have v2.s2 endpoints" { + assert_upstream_missing 127.0.0.1:20000 v2.s2 +} + +@test "terminating gateway should still have s2 endpoints" { + # expected 3 nodes here due to s1, s2, s2-v1, and s2-v2, the latter + # all starting with "s2" + assert_upstream_has_endpoints_in_status 127.0.0.1:20000 s2 HEALTHY 3 +} + diff --git a/test/integration/connect/envoy/helpers.bash b/test/integration/connect/envoy/helpers.bash index c7746d9260d..65bbe3b0070 100755 --- a/test/integration/connect/envoy/helpers.bash +++ b/test/integration/connect/envoy/helpers.bash @@ -383,15 +383,27 @@ function assert_upstream_has_endpoints_in_status_once { GOT_COUNT=$(get_upstream_endpoint_in_status_count $HOSTPORT $CLUSTER_NAME $HEALTH_STATUS) + echo "GOT: $GOT_COUNT" [ "$GOT_COUNT" -eq $EXPECT_COUNT ] } +function assert_upstream_missing_once { + local HOSTPORT=$1 + local CLUSTER_NAME=$2 + + run get_upstream_endpoint $HOSTPORT $CLUSTER_NAME + [ "$status" -eq 0 ] + echo "$output" + [ "" == "$output" ] +} + function assert_upstream_missing { local HOSTPORT=$1 local CLUSTER_NAME=$2 - run retry_default get_upstream_endpoint $HOSTPORT $CLUSTER_NAME + run retry_long assert_upstream_missing_once $HOSTPORT $CLUSTER_NAME echo "OUTPUT: $output $status" - [ "" == "$output" ] + + [ "$status" -eq 0 ] } function assert_upstream_has_endpoints_in_status { @@ -400,6 +412,8 @@ function assert_upstream_has_endpoints_in_status { local HEALTH_STATUS=$3 local EXPECT_COUNT=$4 run retry_long assert_upstream_has_endpoints_in_status_once $HOSTPORT $CLUSTER_NAME $HEALTH_STATUS $EXPECT_COUNT + echo "$output" + [ "$status" -eq 0 ] } From 4f82a0620c107b11894cefe68e7aa1ee575d20db Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Thu, 2 Mar 2023 20:42:59 +0000 Subject: [PATCH 3/4] backport of commit b1b2abc14a473864461a5dccab2911cb84f46225 --- .changelog/16498.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/16498.txt diff --git a/.changelog/16498.txt b/.changelog/16498.txt new file mode 100644 index 00000000000..cdb045d67c9 --- /dev/null +++ b/.changelog/16498.txt @@ -0,0 +1,3 @@ +```release-note:bug +proxycfg: fix a bug where terminating gateways were not cleaning up deleted service resolvers for their referenced services +``` From 6750690cdbf41e7ac2129f31e6b02c26dfd99ffb Mon Sep 17 00:00:00 2001 From: Andrew Stucki Date: Fri, 3 Mar 2023 14:34:47 +0000 Subject: [PATCH 4/4] backport of commit ecaeff26aa1a2723a3b6239c08a125acf48731c9 --- agent/proxycfg/state_test.go | 22 ++++++++ .../capture.sh | 4 -- .../service_gateway.hcl | 9 ---- .../service_s2-v1.hcl | 9 ---- .../service_s2-v2.hcl | 9 ---- .../setup.sh | 41 --------------- .../vars.sh | 8 --- .../verify.bats | 50 ------------------- 8 files changed, 22 insertions(+), 130 deletions(-) delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh delete mode 100644 test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats diff --git a/agent/proxycfg/state_test.go b/agent/proxycfg/state_test.go index 894f2bd4795..bdb2e3c07bc 100644 --- a/agent/proxycfg/state_test.go +++ b/agent/proxycfg/state_test.go @@ -2093,6 +2093,28 @@ func TestState_WatchesAndUpdates(t *testing.T) { require.Equal(t, dbResolver.Entry, snap.TerminatingGateway.ServiceResolvers[db]) }, }, + { + requiredWatches: map[string]verifyWatchRequest{ + "service-resolver:" + db.String(): genVerifyResolverWatch("db", "dc1", structs.ServiceResolver), + }, + events: []UpdateEvent{ + { + CorrelationID: "service-resolver:" + db.String(), + Result: &structs.ConfigEntryResponse{ + Entry: nil, + }, + Err: nil, + }, + }, + verifySnapshot: func(t testing.TB, snap *ConfigSnapshot) { + require.True(t, snap.Valid(), "gateway with service list is valid") + // Finally ensure we cleaned up the resolver + require.Equal(t, []structs.ServiceName{db}, snap.TerminatingGateway.ValidServices()) + + require.False(t, snap.TerminatingGateway.ServiceResolversSet[db]) + require.Nil(t, snap.TerminatingGateway.ServiceResolvers[db]) + }, + }, { events: []UpdateEvent{ { diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh deleted file mode 100644 index 2ef0c41a215..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/capture.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!/bin/bash - -snapshot_envoy_admin localhost:20000 terminating-gateway primary || true -snapshot_envoy_admin localhost:19000 s1 primary || true diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl deleted file mode 100644 index fe597c235cf..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_gateway.hcl +++ /dev/null @@ -1,9 +0,0 @@ -services { - name = "terminating-gateway" - kind = "terminating-gateway" - port = 8443 - - meta { - version = "v1" - } -} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl deleted file mode 100644 index fb87dc41d5e..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v1.hcl +++ /dev/null @@ -1,9 +0,0 @@ -services { - id = "s2-v1" - name = "s2" - port = 8182 - - meta { - version = "v1" - } -} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl deleted file mode 100644 index 52ab0abd684..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/service_s2-v2.hcl +++ /dev/null @@ -1,9 +0,0 @@ -services { - id = "s2-v2" - name = "s2" - port = 8183 - - meta { - version = "v2" - } -} diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh deleted file mode 100644 index fdd49572ba8..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/setup.sh +++ /dev/null @@ -1,41 +0,0 @@ -#!/bin/bash - -set -euo pipefail - -upsert_config_entry primary ' -kind = "terminating-gateway" -name = "terminating-gateway" -services = [ - { - name = "s2" - } -] -' - -upsert_config_entry primary ' -kind = "proxy-defaults" -name = "global" -config { - protocol = "http" -} -' - -upsert_config_entry primary ' -kind = "service-resolver" -name = "s2" -default_subset = "v1" -subsets = { - "v1" = { - filter = "Service.Meta.version == v1" - } - "v2" = { - filter = "Service.Meta.version == v2" - } -} -' - -register_services primary - -# terminating gateway will act as s2's proxy -gen_envoy_bootstrap s1 19000 -gen_envoy_bootstrap terminating-gateway 20000 primary true diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh deleted file mode 100644 index 9e52629b8be..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/vars.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash - -# There is no sidecar proxy for s2-v1, since the terminating gateway acts as the proxy -export REQUIRED_SERVICES=" -s1 s1-sidecar-proxy -s2-v1 -terminating-gateway-primary -" diff --git a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats b/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats deleted file mode 100644 index c30b920c281..00000000000 --- a/test/integration/connect/envoy/case-terminating-gateway-deleted-resolver/verify.bats +++ /dev/null @@ -1,50 +0,0 @@ -#!/usr/bin/env bats - -load helpers - -@test "s1 proxy admin is up on :19000" { - retry_default curl -f -s localhost:19000/stats -o /dev/null -} - -@test "terminating proxy admin is up on :20000" { - retry_default curl -f -s localhost:20000/stats -o /dev/null -} - -@test "terminating-gateway-primary listener is up on :8443" { - retry_default nc -z localhost:8443 -} - -@test "s1 proxy listener should be up and have right cert" { - assert_proxy_presents_cert_uri localhost:21000 s1 -} - -@test "s1 upstream should have healthy endpoints for v1.s2" { - assert_upstream_has_endpoints_in_status 127.0.0.1:19000 v1.s2 HEALTHY 1 -} - -@test "terminating-gateway should have healthy endpoints for v1.s2" { - assert_upstream_has_endpoints_in_status 127.0.0.1:20000 v1.s2 HEALTHY 1 -} - -@test "terminating-gateway should have healthy endpoints for v2.s2" { - assert_upstream_has_endpoints_in_status 127.0.0.1:20000 v2.s2 HEALTHY 1 -} - -@test "deleting the service-resolver should be possible" { - delete_config_entry service-resolver s2 -} - -@test "terminating gateway should no longer have v1.s2 endpoints" { - assert_upstream_missing 127.0.0.1:20000 v1.s2 -} - -@test "terminating gateway should no longer have v2.s2 endpoints" { - assert_upstream_missing 127.0.0.1:20000 v2.s2 -} - -@test "terminating gateway should still have s2 endpoints" { - # expected 3 nodes here due to s1, s2, s2-v1, and s2-v2, the latter - # all starting with "s2" - assert_upstream_has_endpoints_in_status 127.0.0.1:20000 s2 HEALTHY 3 -} -