Skip to content

Commit

Permalink
Catalog V2 Container Based Integration Test (#17674)
Browse files Browse the repository at this point in the history
* Implement the Catalog V2 controller integration container tests

This now allows the container tests to import things from the root module. However for now we want to be very restrictive about which packages we allow importing.

* Add an upgrade test for the new catalog

Currently this should be dormant and not executed. However its put in place to detect breaking changes in the future and show an example of how to do an upgrade test with integration tests structured like catalog v2.

* Make testutil.Retry capable of performing cleanup operations

These cleanup operations are executed after each retry attempt.

* Move TestContext to taking an interface instead of a concrete testing.T

This allows this to be used on a retry.R or generally anything that meets the interface.

* Move to using TestContext instead of background contexts

Also this forces all test methods to implement the Cleanup method now instead of that being an optional interface.


Co-authored-by: Daniel Upton <[email protected]>
  • Loading branch information
mkeeler and boxofrad authored Jun 16, 2023
1 parent 5352ccf commit 37636ea
Show file tree
Hide file tree
Showing 18 changed files with 613 additions and 113 deletions.
21 changes: 14 additions & 7 deletions GNUmakefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

SHELL = bash


GO_MODULES := $(shell find . -name go.mod -exec dirname {} \; | grep -v "proto-gen-rpc-glue/e2e" | sort)

###
Expand Down Expand Up @@ -72,6 +73,7 @@ CI_DEV_DOCKER_NAMESPACE?=hashicorpdev
CI_DEV_DOCKER_IMAGE_NAME?=consul
CI_DEV_DOCKER_WORKDIR?=bin/
################
CONSUL_VERSION?=$(shell cat version/VERSION)

TEST_MODCACHE?=1
TEST_BUILDCACHE?=1
Expand Down Expand Up @@ -188,6 +190,8 @@ dev-docker: linux dev-build
@docker buildx use default && docker buildx build -t 'consul:local' -t '$(CONSUL_DEV_IMAGE)' \
--platform linux/$(GOARCH) \
--build-arg CONSUL_IMAGE_VERSION=$(CONSUL_IMAGE_VERSION) \
--label org.opencontainers.image.version=$(CONSUL_VERSION) \
--label version=$(CONSUL_VERSION) \
--load \
-f $(CURDIR)/build-support/docker/Consul-Dev-Multiarch.dockerfile $(CURDIR)/pkg/bin/

Expand All @@ -208,6 +212,8 @@ remote-docker: check-remote-dev-image-env
@docker buildx use consul-builder && docker buildx build -t '$(REMOTE_DEV_IMAGE)' \
--platform linux/amd64,linux/arm64 \
--build-arg CONSUL_IMAGE_VERSION=$(CONSUL_IMAGE_VERSION) \
--label org.opencontainers.image.version=$(CONSUL_VERSION) \
--label version=$(CONSUL_VERSION) \
--push \
-f $(CURDIR)/build-support/docker/Consul-Dev-Multiarch.dockerfile $(CURDIR)/pkg/bin/

Expand Down Expand Up @@ -351,16 +357,17 @@ lint/%:
@echo "--> Running enumcover ($*)"
@cd $* && GOWORK=off enumcover ./...

# check that the test-container module only imports allowlisted packages
# from the root consul module. Generally we don't want to allow these imports.
# In a few specific instances though it is okay to import test definitions and
# helpers from some of the packages in the root module.
.PHONY: lint-container-test-deps
lint-container-test-deps:
@echo "--> Checking container tests for bad dependencies"
@cd test/integration/consul-container && ( \
found="$$(go list -m all | grep -c '^github.com/hashicorp/consul ')" ; \
if [[ "$$found" != "0" ]]; then \
echo "test/integration/consul-container: This project should not depend on the root consul module" >&2 ; \
exit 1 ; \
fi \
)
@cd test/integration/consul-container && \
$(CURDIR)/build-support/scripts/check-allowed-imports.sh \
github.com/hashicorp/consul \
internal/catalog/catalogtest

# Build the static web ui inside a Docker container. For local testing only; do not commit these assets.
ui: ui-docker
Expand Down
124 changes: 124 additions & 0 deletions build-support/scripts/check-allowed-imports.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
#!/usr/bin/env bash
# Copyright (c) HashiCorp, Inc.
# SPDX-License-Identifier: MPL-2.0


readonly SCRIPT_NAME="$(basename ${BASH_SOURCE[0]})"
readonly SCRIPT_DIR="$(dirname "${BASH_SOURCE[0]}")"
readonly SOURCE_DIR="$(dirname "$(dirname "${SCRIPT_DIR}")")"
readonly FN_DIR="$(dirname "${SCRIPT_DIR}")/functions"

source "${SCRIPT_DIR}/functions.sh"


set -uo pipefail

usage() {
cat <<-EOF
Usage: ${SCRIPT_NAME} <module root> [<allowed relative package path>...]
Description:
Verifies that only the specified packages may be imported from the given module
Options:
-h | --help Print this help text.
EOF
}

function err_usage {
err "$1"
err ""
err "$(usage)"
}

function main {
local module_root=""
declare -a allowed_packages=()
while test $# -gt 0
do
case "$1" in
-h | --help )
usage
return 0
;;
* )
if test -z "$module_root"
then
module_root="$1"
else
allowed_packages+="$1"
fi
shift
esac
done

# If we could guarantee this ran with bash 4.2+ then the final argument could
# be just ${allowed_packages[@]}. However that with older versions of bash
# in combination with set -u causes bash to emit errors about using unbound
# variables when no allowed packages have been specified (i.e. the module should
# generally be disallowed with no exceptions). This syntax is very strange
# but seems to be the prescribed workaround I found.
check_imports "$module_root" ${allowed_packages[@]+"${allowed_packages[@]}"}
return $?
}

function check_imports {
local module_root="$1"
shift
local allowed_packages="$@"

module_imports=$( go list -test -f '{{join .TestImports "\n"}}' ./... | grep "$module_root" | sort | uniq)
module_test_imports=$( go list -test -f '{{join .TestImports "\n"}}' ./... | grep "$module_root" | sort | uniq)

any_error=0

for imp in $module_imports
do
is_import_allowed "$imp" "$module_root" $allowed_packages
allowed=$?

if test $any_error -ne 1
then
any_error=$allowed
fi
done

if test $any_error -eq 1
then
echo "Only the following direct imports are allowed from module $module_root:"
for pkg in $allowed_packages
do
echo " * $pkg"
done
fi

return $any_error
}

function is_import_allowed {
local pkg_import=$1
shift
local module_root=$1
shift
local allowed_packages="$@"

# check if the import path is a part of the module we are restricting imports for
if test "$( go list -f '{{.Module.Path}}' $pkg_import)" != "$module_root"
then
return 0
fi

for pkg in $allowed_packages
do
if test "${module_root}/$pkg" == "$pkg_import"
then
return 0
fi
done

err "Import of package $pkg_import is not allowed"
return 1
}

main "$@"
exit $?
51 changes: 32 additions & 19 deletions internal/resource/resourcetest/builder.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package resourcetest

import (
"context"
"strings"

"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/internal/storage"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/oklog/ulid/v2"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -119,26 +121,37 @@ func (b *resourceBuilder) ID() *pbresource.ID {
func (b *resourceBuilder) Write(t T, client pbresource.ResourceServiceClient) *pbresource.Resource {
t.Helper()

ctx := testutil.TestContext(t)

res := b.resource

rsp, err := client.Write(context.Background(), &pbresource.WriteRequest{
Resource: res,
})
var rsp *pbresource.WriteResponse
var err error

require.NoError(t, err)
// Retry any writes where the error is a UID mismatch and the UID was not specified. This is indicative
// of using a follower to rewrite an object who is not perfectly in-sync with the leader.
retry.Run(t, func(r *retry.R) {
rsp, err = client.Write(ctx, &pbresource.WriteRequest{
Resource: res,
})

if err == nil || res.Id.Uid != "" || status.Code(err) != codes.FailedPrecondition {
return
}

if strings.Contains(err.Error(), storage.ErrWrongUid.Error()) {
r.Fatalf("resource write failed due to uid mismatch - most likely a transient issue when talking to a non-leader")
} else {
// other errors are unexpected and should cause an immediate failure
r.Stop(err)
}
})

if !b.dontCleanup {
cleaner, ok := t.(CleanupT)
require.True(t, ok, "T does not implement a Cleanup method and cannot be used with automatic resource cleanup")
cleaner.Cleanup(func() {
_, err := client.Delete(context.Background(), &pbresource.DeleteRequest{
Id: rsp.Resource.Id,
})

// ignore not found errors
if err != nil && status.Code(err) != codes.NotFound {
t.Fatalf("Failed to delete resource %s of type %s: %v", rsp.Resource.Id.Name, resource.ToGVK(rsp.Resource.Id.Type), err)
}
id := proto.Clone(rsp.Resource.Id).(*pbresource.ID)
id.Uid = ""
t.Cleanup(func() {
NewClient(client).MustDelete(t, id)
})
}

Expand All @@ -151,15 +164,15 @@ func (b *resourceBuilder) Write(t T, client pbresource.ResourceServiceClient) *p
ObservedGeneration: rsp.Resource.Generation,
Conditions: original.Conditions,
}
_, err := client.WriteStatus(context.Background(), &pbresource.WriteStatusRequest{
_, err := client.WriteStatus(ctx, &pbresource.WriteStatusRequest{
Id: rsp.Resource.Id,
Key: key,
Status: status,
})
require.NoError(t, err)
}

readResp, err := client.Read(context.Background(), &pbresource.ReadRequest{
readResp, err := client.Read(ctx, &pbresource.ReadRequest{
Id: rsp.Resource.Id,
})

Expand Down
40 changes: 30 additions & 10 deletions internal/resource/resourcetest/client.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package resourcetest

import (
"context"
"fmt"
"math/rand"
"time"

"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/stretchr/testify/require"
"golang.org/x/exp/slices"
Expand Down Expand Up @@ -41,6 +42,8 @@ func (client *Client) retry(t T, fn func(r *retry.R)) {
}

func (client *Client) PublishResources(t T, resources []*pbresource.Resource) {
ctx := testutil.TestContext(t)

// Randomize the order of insertion. Generally insertion order shouldn't matter as the
// controllers should eventually converge on the desired state. The exception to this
// is that you cannot insert resources with owner refs before the resource they are
Expand Down Expand Up @@ -75,12 +78,17 @@ func (client *Client) PublishResources(t T, resources []*pbresource.Resource) {
}

t.Logf("Writing resource %s with type %s", res.Id.Name, resource.ToGVK(res.Id.Type))
_, err := client.Write(context.Background(), &pbresource.WriteRequest{
rsp, err := client.Write(ctx, &pbresource.WriteRequest{
Resource: res,
})
require.NoError(t, err)

// track the number o
id := rsp.Resource.Id
t.Cleanup(func() {
client.MustDelete(t, id)
})

// track the number of resources published
published += 1
written = append(written, res.Id)
}
Expand All @@ -102,7 +110,7 @@ func (client *Client) PublishResources(t T, resources []*pbresource.Resource) {
func (client *Client) RequireResourceNotFound(t T, id *pbresource.ID) {
t.Helper()

rsp, err := client.Read(context.Background(), &pbresource.ReadRequest{Id: id})
rsp, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{Id: id})
require.Error(t, err)
require.Equal(t, codes.NotFound, status.Code(err))
require.Nil(t, rsp)
Expand All @@ -111,7 +119,7 @@ func (client *Client) RequireResourceNotFound(t T, id *pbresource.ID) {
func (client *Client) RequireResourceExists(t T, id *pbresource.ID) *pbresource.Resource {
t.Helper()

rsp, err := client.Read(context.Background(), &pbresource.ReadRequest{Id: id})
rsp, err := client.Read(testutil.TestContext(t), &pbresource.ReadRequest{Id: id})
require.NoError(t, err, "error reading %s with type %s", id.Name, resource.ToGVK(id.Type))
require.NotNil(t, rsp)
return rsp.Resource
Expand Down Expand Up @@ -227,10 +235,22 @@ func (client *Client) ResolveResourceID(t T, id *pbresource.ID) *pbresource.ID {
}

func (client *Client) MustDelete(t T, id *pbresource.ID) {
_, err := client.Delete(context.Background(), &pbresource.DeleteRequest{Id: id})
if status.Code(err) == codes.NotFound {
return
}
t.Helper()
ctx := testutil.TestContext(t)

require.NoError(t, err)
client.retry(t, func(r *retry.R) {
_, err := client.Delete(ctx, &pbresource.DeleteRequest{Id: id})
if status.Code(err) == codes.NotFound {
return
}

// codes.Aborted indicates a CAS failure and that the delete request should
// be retried. Anything else should be considered an unrecoverable error.
if err != nil && status.Code(err) != codes.Aborted {
r.Stop(fmt.Errorf("failed to delete the resource: %w", err))
return
}

require.NoError(r, err)
})
}
4 changes: 0 additions & 4 deletions internal/resource/resourcetest/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,5 @@ type T interface {
Errorf(format string, args ...interface{})
Fatalf(format string, args ...interface{})
FailNow()
}

type CleanupT interface {
T
Cleanup(func())
}
8 changes: 6 additions & 2 deletions sdk/testutil/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ package testutil

import (
"context"
"testing"
)

func TestContext(t *testing.T) context.Context {
type CleanerT interface {
Helper()
Cleanup(func())
}

func TestContext(t CleanerT) context.Context {
t.Helper()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
Expand Down
Loading

0 comments on commit 37636ea

Please sign in to comment.