From f72aeadfdac618b18067463a2677737fd6945435 Mon Sep 17 00:00:00 2001 From: Eric Date: Tue, 26 Sep 2023 20:03:45 -0400 Subject: [PATCH] fixup! Add traffic permissions integration tests. --- .github/workflows/test-integrations.yml | 18 +++--- Makefile | 2 +- .../assets/Dockerfile-consul-dataplane | 6 +- .../libs/cluster/dataplane.go | 47 +++++++------- .../test/traffic_permissions/tcp_test.go | 63 +++++-------------- 5 files changed, 53 insertions(+), 83 deletions(-) diff --git a/.github/workflows/test-integrations.yml b/.github/workflows/test-integrations.yml index 9dd7d3a3a35bd..0020ac3cb9f9f 100644 --- a/.github/workflows/test-integrations.yml +++ b/.github/workflows/test-integrations.yml @@ -32,14 +32,14 @@ concurrency: jobs: conditional-skip: - runs-on: ubuntu-latest + runs-on: ubuntu-latest name: Get files changed and conditionally skip CI outputs: skip-ci: ${{ steps.read-files.outputs.skip-ci }} steps: - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 with: - fetch-depth: 0 + fetch-depth: 0 - name: Get changed files id: read-files run: ./.github/scripts/filter_changed_files_go_test.sh @@ -116,7 +116,7 @@ jobs: --packages="./command/agent/consul" \ --junitfile $TEST_RESULTS_DIR/results.xml -- \ -run TestConsul - + # NOTE: ENT specific step as we store secrets in Vault. - name: Authenticate to Vault if: ${{ endsWith(github.repository, '-enterprise') }} @@ -257,12 +257,12 @@ jobs: - name: Generate Envoy Job Matrix id: set-matrix env: - # this is further going to multiplied in envoy-integration tests by the + # this is further going to multiplied in envoy-integration tests by the # other dimensions in the matrix. Currently TOTAL_RUNNERS would be # multiplied by 8 based on these values: # envoy-version: ["1.24.10", "1.25.9", "1.26.4", "1.27.0"] # xds-target: ["server", "client"] - TOTAL_RUNNERS: 4 + TOTAL_RUNNERS: 4 JQ_SLICER: '[ inputs ] | [_nwise(length / $runnercount | floor)]' run: | NUM_RUNNERS=$TOTAL_RUNNERS @@ -281,7 +281,7 @@ jobs: | jq --raw-input --argjson runnercount "$NUM_RUNNERS" "$JQ_SLICER" \ | jq --compact-output 'map(join("|"))' } >> "$GITHUB_OUTPUT" - + envoy-integration-test: runs-on: ${{ fromJSON(needs.setup.outputs.compute-large) }} needs: @@ -384,7 +384,7 @@ jobs: contents: read env: ENVOY_VERSION: "1.25.4" - CONSUL_DATAPLANE_IMAGE: "docker.io/hashicorppreview/consul-dataplane:1.3-dev" + CONSUL_DATAPLANE_IMAGE: "docker.io/hashicorppreview/consul-dataplane:1.3-dev-ubi" steps: - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 # NOTE: This step is specifically needed for ENT. It allows us to access the required private HashiCorp repos. @@ -417,7 +417,7 @@ jobs: if: steps.buildConsulEnvoyImage.outcome == 'failure' run: docker build -t consul-envoy:target-version --build-arg CONSUL_IMAGE=${{ env.CONSUL_LATEST_IMAGE_NAME }}:local --build-arg ENVOY_VERSION=${{ env.ENVOY_VERSION }} -f ./test/integration/consul-container/assets/Dockerfile-consul-envoy ./test/integration/consul-container/assets - name: Build consul-dataplane:local image - run: docker build -t consul-dataplane:local --build-arg CONSUL_DATAPLANE_IMAGE=${{ env.CONSUL_DATAPLANE_IMAGE }} -f ./test/integration/consul-container/assets/Dockerfile-consul-dataplane ./test/integration/consul-container/assets + run: docker build -t consul-dataplane:local --build-arg CONSUL_IMAGE=${{ env.CONSUL_LATEST_IMAGE_NAME }}:local --build-arg CONSUL_DATAPLANE_IMAGE=${{ env.CONSUL_DATAPLANE_IMAGE }} -f ./test/integration/consul-container/assets/Dockerfile-consul-dataplane ./test/integration/consul-container/assets - name: Configure GH workaround for ipv6 loopback if: ${{ !endsWith(github.repository, '-enterprise') }} run: | @@ -489,7 +489,7 @@ jobs: test-integrations-success: - needs: + needs: - conditional-skip - setup - dev-build diff --git a/Makefile b/Makefile index 30363e42aae13..4ee07faef5003 100644 --- a/Makefile +++ b/Makefile @@ -349,7 +349,7 @@ test-compat-integ-setup: dev-docker @docker run --rm -t $(CONSUL_COMPAT_TEST_IMAGE):local consul version @# 'consul-envoy:target-version' is needed by compatibility integ test @docker build -t consul-envoy:target-version --build-arg CONSUL_IMAGE=$(CONSUL_COMPAT_TEST_IMAGE):local --build-arg ENVOY_VERSION=${ENVOY_VERSION} -f ./test/integration/consul-container/assets/Dockerfile-consul-envoy ./test/integration/consul-container/assets - @docker build -t consul-dataplane:local --build-arg CONSUL_DATAPLANE_IMAGE=${CONSUL_DATAPLANE_IMAGE} -f ./test/integration/consul-container/assets/Dockerfile-consul-dataplane ./test/integration/consul-container/assets + @docker build -t consul-dataplane:local --build-arg CONSUL_IMAGE=$(CONSUL_COMPAT_TEST_IMAGE):local --build-arg CONSUL_DATAPLANE_IMAGE=${CONSUL_DATAPLANE_IMAGE} -f ./test/integration/consul-container/assets/Dockerfile-consul-dataplane ./test/integration/consul-container/assets .PHONY: test-compat-integ test-compat-integ: test-compat-integ-setup ## Test compat integ diff --git a/test/integration/consul-container/assets/Dockerfile-consul-dataplane b/test/integration/consul-container/assets/Dockerfile-consul-dataplane index 812241dc85599..1394a2bfcd683 100644 --- a/test/integration/consul-container/assets/Dockerfile-consul-dataplane +++ b/test/integration/consul-container/assets/Dockerfile-consul-dataplane @@ -2,6 +2,10 @@ # SPDX-License-Identifier: BUSL-1.1 ARG CONSUL_DATAPLANE_IMAGE +ARG CONSUL_IMAGE + +# Docker doesn't support expansion in COPY --copy, so we need to create an intermediate image. +FROM ${CONSUL_IMAGE} as consul FROM ${CONSUL_DATAPLANE_IMAGE} as consuldataplane @@ -11,7 +15,7 @@ RUN microdnf install -y iptables sudo nc \ && usermod -a -G wheel consul-dataplane \ && echo 'consul-dataplane ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers -COPY --from=consul:local /bin/consul /bin/consul +COPY --from=consul /bin/consul /bin/consul COPY tproxy-startup.sh /bin/tproxy-startup.sh RUN chmod +x /bin/tproxy-startup.sh && chown root:root /bin/tproxy-startup.sh diff --git a/test/integration/consul-container/libs/cluster/dataplane.go b/test/integration/consul-container/libs/cluster/dataplane.go index 96a0fd3b55a55..a9f8498acebf0 100644 --- a/test/integration/consul-container/libs/cluster/dataplane.go +++ b/test/integration/consul-container/libs/cluster/dataplane.go @@ -92,37 +92,16 @@ func NewConsulDataplane(ctx context.Context, proxyID string, serverAddresses str copy(exposedPorts, appPortStrs) exposedPorts = append(exposedPorts, adminPortStr) - command := []string{ - "consul-dataplane", - "-addresses", serverAddresses, - fmt.Sprintf("-grpc-port=%d", grpcPort), - fmt.Sprintf("-proxy-id=%s", proxyID), - "-proxy-namespace=default", - "-proxy-partition=default", - "-log-level=info", - "-log-json=false", - "-envoy-concurrency=2", - "-tls-disabled", - fmt.Sprintf("-envoy-admin-bind-port=%d", internalAdminPort), - } - - if bootstrapToken != "" { - command = append(command, - "-credential-type=static", - fmt.Sprintf("-static-token=%s", bootstrapToken)) - } - - command = append(command, containerArgs...) - req := testcontainers.ContainerRequest{ Image: "consul-dataplane:local", WaitingFor: wait.ForLog("").WithStartupTimeout(60 * time.Second), AutoRemove: false, Name: containerName, - Cmd: command, Env: map[string]string{}, } + var command []string + if tproxy { req.Entrypoint = []string{"sh", "/bin/tproxy-startup.sh"} req.Env["REDIRECT_TRAFFIC_ARGS"] = strings.Join( @@ -142,8 +121,30 @@ func NewConsulDataplane(ctx context.Context, proxyID string, serverAddresses str " ", ) req.CapAdd = append(req.CapAdd, "NET_ADMIN") + command = append(command, "consul-dataplane") } + command = append(command, + "-addresses", serverAddresses, + fmt.Sprintf("-grpc-port=%d", grpcPort), + fmt.Sprintf("-proxy-id=%s", proxyID), + "-proxy-namespace=default", + "-proxy-partition=default", + "-log-level=info", + "-log-json=false", + "-envoy-concurrency=2", + "-tls-disabled", + fmt.Sprintf("-envoy-admin-bind-port=%d", internalAdminPort), + ) + + if bootstrapToken != "" { + command = append(command, + "-credential-type=static", + fmt.Sprintf("-static-token=%s", bootstrapToken)) + } + + req.Cmd = append(command, containerArgs...) + info, err := LaunchContainerOnNode(ctx, node, req, exposedPorts) if err != nil { return nil, err diff --git a/test/integration/consul-container/test/traffic_permissions/tcp_test.go b/test/integration/consul-container/test/traffic_permissions/tcp_test.go index 0a703881befbe..24553a8266aed 100644 --- a/test/integration/consul-container/test/traffic_permissions/tcp_test.go +++ b/test/integration/consul-container/test/traffic_permissions/tcp_test.go @@ -43,14 +43,20 @@ type trafficPermissionsCase struct { client2EchoSuccess bool } +// We are using tproxy to test traffic permissions now because explicitly specifying destinations +// doesn't work when multiple downstreams specify the same destination yet. In the future, we will need +// to update this to use explicit destinations once we infer tproxy destinations from traffic permissions. +// +// This also explicitly uses virtual IPs and virtual ports because Consul DNS doesn't support v2 resources yet. +// We should update this to use Consul DNS when it is working. func runTrafficPermissionsTests(t *testing.T, aclsEnabled bool, cases map[string]trafficPermissionsCase) { t.Parallel() cluster, resourceClient := createCluster(t, aclsEnabled) - serverService, serverDataplane := createServerResources(t, resourceClient, cluster, cluster.Agents[1]) - client1Dataplane := createClientResources(t, resourceClient, cluster, serverService, cluster.Agents[2], 1) - client2Dataplane := createClientResources(t, resourceClient, cluster, serverService, cluster.Agents[3], 2) + serverDataplane := createServerResources(t, resourceClient, cluster, cluster.Agents[1]) + client1Dataplane := createClientResources(t, resourceClient, cluster, cluster.Agents[2], 1) + client2Dataplane := createClientResources(t, resourceClient, cluster, cluster.Agents[3], 2) assertDataplaneContainerState(t, client1Dataplane, "running") assertDataplaneContainerState(t, client2Dataplane, "running") @@ -318,7 +324,6 @@ func TestTrafficPermission_TCP_DefaultAllow(t *testing.T) { client2TCPSuccess: true, client2EchoSuccess: true, }, - // TODO I don't like this behavior. "allow one protocol doesnt impact the other protocol": { tp1: &pbauth.TrafficPermissions{ Destination: &pbauth.Destination{ @@ -426,8 +431,8 @@ func storeStaticServerTrafficPermissions(t *testing.T, resourceClient *rtest.Cli } } -func createServerResources(t *testing.T, resourceClient *rtest.Client, cluster *libcluster.Cluster, node libcluster.Agent) (*pbresource.Resource, *libcluster.ConsulDataplaneContainer) { - serverService := rtest.ResourceID(&pbresource.ID{ +func createServerResources(t *testing.T, resourceClient *rtest.Client, cluster *libcluster.Cluster, node libcluster.Agent) *libcluster.ConsulDataplaneContainer { + rtest.ResourceID(&pbresource.ID{ Name: "static-server-service", Type: pbcatalog.ServiceType, }). @@ -483,10 +488,10 @@ func createServerResources(t *testing.T, resourceClient *rtest.Client, cluster * serverDataplane, err := createServiceAndDataplane(t, node, cluster, "static-server-workload", "static-server", 8080, 8079, []int{}) require.NoError(t, err) - return serverService, serverDataplane + return serverDataplane } -func createClientResources(t *testing.T, resourceClient *rtest.Client, cluster *libcluster.Cluster, staticServerRef *pbresource.Resource, node libcluster.Agent, idx int) *libcluster.ConsulDataplaneContainer { +func createClientResources(t *testing.T, resourceClient *rtest.Client, cluster *libcluster.Cluster, node libcluster.Agent, idx int) *libcluster.ConsulDataplaneContainer { prefix := fmt.Sprintf("static-client-%d", idx) rtest.ResourceID(&pbresource.ID{ Name: prefix + "-service", @@ -580,30 +585,10 @@ func assertDataplaneContainerState(t *testing.T, dataplane *libcluster.ConsulDat func httpRequestToVirtualAddress(dp *libcluster.ConsulDataplaneContainer) (string, error) { addr := fmt.Sprintf("%s:%d", staticServerVIP, tcpPort) - // Test that we can make a request to the virtual ip to reach the upstream. - // - // NOTE(pglass): This uses a workaround for DNS because I had trouble modifying - // /etc/resolv.conf. There is a --dns option to docker run, but it - // didn't seem to be exposed via testcontainers. I'm not sure if it would - // do what I want. In any case, Docker sets up /etc/resolv.conf for certain - // functionality so it seems better to leave DNS alone. - // - // But, that means DNS queries aren't redirected to Consul out of the box. - // As a workaround, we `dig @localhost:53` which is iptables-redirected to - // localhost:8600 where the Consul client responds with the virtual ip. - // - // In tproxy tests, Envoy is not configured with a unique listener for each - // upstream. This means the usual approach for non-tproxy tests doesn't - // work - where we send the request to a host address mapped in to Envoy's - // upstream listener. Instead, we exec into the container and run curl. - // - // We must make this request with a non-envoy user. The envoy and consul - // users are excluded from traffic redirection rules, so instead we - // make the request as root. out, err := dp.Exec( context.Background(), []string{"sudo", "sh", "-c", fmt.Sprintf(` -set -e + set -e curl -s "%s/debug?env=dump" `, addr), }, @@ -622,26 +607,6 @@ set -e } func echoToVirtualAddress(dp *libcluster.ConsulDataplaneContainer) (string, error) { - // Test that we can make a request to the virtual ip to reach the upstream. - // - // NOTE(pglass): This uses a workaround for DNS because I had trouble modifying - // /etc/resolv.conf. There is a --dns option to docker run, but it - // didn't seem to be exposed via testcontainers. I'm not sure if it would - // do what I want. In any case, Docker sets up /etc/resolv.conf for certain - // functionality so it seems better to leave DNS alone. - // - // But, that means DNS queries aren't redirected to Consul out of the box. - // As a workaround, we `dig @localhost:53` which is iptables-redirected to - // localhost:8600 where the Consul client responds with the virtual ip. - // - // In tproxy tests, Envoy is not configured with a unique listener for each - // upstream. This means the usual approach for non-tproxy tests doesn't - // work - where we send the request to a host address mapped in to Envoy's - // upstream listener. Instead, we exec into the container and run curl. - // - // We must make this request with a non-envoy user. The envoy and consul - // users are excluded from traffic redirection rules, so instead we - // make the request as root. out, err := dp.Exec( context.Background(), []string{"sudo", "sh", "-c", fmt.Sprintf(`