Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CNI Accptance test on Kind #1445

Merged
merged 10 commits into from
Aug 25, 2022
100 changes: 76 additions & 24 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ commands:
wget https://get.helm.sh/helm-v3.7.0-linux-amd64.tar.gz
tar -zxvf helm-v3.7.0-linux-amd64.tar.gz
sudo mv linux-amd64/helm /usr/local/bin/helm

create-kind-clusters:
parameters:
version:
Expand All @@ -59,6 +58,19 @@ commands:
command: |
kind create cluster --name dc1 --image kindest/node:<< parameters.version >>
kind create cluster --name dc2 --image kindest/node:<< parameters.version >>
create-kind-cni-clusters:
parameters:
version:
type: string
steps:
- run:
name: Create CNI kind clusters
command: |
kind create cluster --config=acceptance/framework/environment/cni-kind/kind.config --name dc1 --image kindest/node:<< parameters.version >>
make kind-cni-calico
sleep 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to sleep here? wouldn't finish setup while the other kind cluster is being created?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

kind create cluster --config=acceptance/framework/environment/cni-kind/kind.config --name dc2 --image kindest/node:<< parameters.version >>
make kind-cni-calico
run-acceptance-tests:
parameters:
failfast:
Expand Down Expand Up @@ -878,6 +890,42 @@ jobs:
fail_only: true
failure_message: "Acceptance tests against Kind with Kubernetes v1.23 failed. Check the logs at: ${CIRCLE_BUILD_URL}"

acceptance-kind-cni-1-23:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add a job for the test-and-build pipeline as well so that it runs on every PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

parallelism: 6
environment:
- TEST_RESULTS: /tmp/test-results
machine:
image: ubuntu-2004:202010-01
resource_class: xlarge
steps:
- checkout
- install-prereqs
- create-kind-cni-clusters:
version: "v1.23.0"
- restore_cache:
keys:
- consul-helm-modcache-v2-{{ checksum "acceptance/go.mod" }}
- run:
name: go mod download
working_directory: *acceptance-mod-path
command: go mod download
- save_cache:
key: consul-helm-modcache-v2-{{ checksum "acceptance/go.mod" }}
paths:
- ~/.go_workspace/pkg/mod
- run: mkdir -p $TEST_RESULTS
- run-acceptance-tests:
additional-flags: -use-kind -kubecontext="kind-dc1" -secondary-kubecontext="kind-dc2" -enable-transparent-proxy -enable-cni
- store_test_results:
path: /tmp/test-results
- store_artifacts:
path: /tmp/test-results
- slack/status:
# temporarily sending to #cni-acceptance-tests channel
Copy link
Contributor Author

@curtbushko curtbushko Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove before merge.

channel: C03V3K0040G
fail_only: true
failure_message: "Acceptance tests for CNI against Kind with Kubernetes v1.23 failed. Check the logs at: ${CIRCLE_BUILD_URL}"

acceptance-kind-1-23-consul-nightly-1-11:
environment:
- TEST_RESULTS: /tmp/test-results
Expand Down Expand Up @@ -1012,13 +1060,13 @@ workflows:
requires:
- dev-upload-docker
nightly-acceptance-tests:
triggers:
- schedule:
cron: "0 0 * * *"
filters:
branches:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix before merge.

only:
- main
# triggers:
# - schedule:
# cron: "0 0 * * *"
# filters:
# branches:
# only:
# - main
jobs:
- build-distro:
OS: "linux"
Expand All @@ -1027,29 +1075,33 @@ workflows:
- dev-upload-docker:
requires:
- build-distros-linux
- cleanup-gcp-resources
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix before merge.

- cleanup-azure-resources
- cleanup-eks-resources
# - cleanup-gcp-resources
# - cleanup-azure-resources
# - cleanup-eks-resources
# Disable until we can use UBI images.
# - acceptance-openshift:
# requires:
# - cleanup-azure-resources
- acceptance-gke-1-20:
requires:
- cleanup-gcp-resources
- dev-upload-docker
- acceptance-eks-1-19:
requires:
- cleanup-eks-resources
- dev-upload-docker
- acceptance-aks-1-21:
requires:
- cleanup-azure-resources
- dev-upload-docker
- acceptance-kind-1-23:
# - acceptance-gke-1-20:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix before merge.

# requires:
# - cleanup-gcp-resources
# - dev-upload-docker
# - acceptance-eks-1-19:
# requires:
# - cleanup-eks-resources
# - dev-upload-docker
# - acceptance-aks-1-21:
# requires:
# - cleanup-azure-resources
# - dev-upload-docker
# - acceptance-kind-1-23:
# requires:
# - dev-upload-docker
- acceptance-kind-cni-1-23:
requires:
- dev-upload-docker


# nightly-acceptance-tests-consul:
# triggers:
# - schedule:
Expand Down
20 changes: 17 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,14 @@ cni-plugin-lint:
ctrl-generate: get-controller-gen ## Run CRD code generation.
cd control-plane; $(CONTROLLER_GEN) object:headerFile="build-support/controller/boilerplate.go.txt" paths="./..."


# Helper target for doing local cni acceptance testing
kind-cni:
kind delete cluster --name dc1
kind delete cluster --name dc2
kind create cluster --config=$(CURDIR)/acceptance/framework/environment/cni-kind/kind.config --name dc1 --image kindest/node:v1.23.6
make kind-cni-calico
kind create cluster --config=$(CURDIR)/acceptance/framework/environment/cni-kind/kind.config --name dc2 --image kindest/node:v1.23.6
make kind-cni-calico


# ===========> CLI Targets
Expand All @@ -75,13 +82,20 @@ cli-lint: ## Run linter in the control-plane directory.
cd cli; golangci-lint run -c ../.golangci.yml




# ===========> Acceptance Tests Targets

acceptance-lint: ## Run linter in the control-plane directory.
cd acceptance; golangci-lint run -c ../.golangci.yml

# For CNI acceptance tests, the calico CNI pluging needs to be installed on Kind. Our consul-cni plugin will not work
# without another plugin installed first
kind-cni-calico:
kubectl create namespace calico-system ||true
kubectl create -f https://docs.projectcalico.org/archive/v3.24/manifests/tigera-operator.yaml
# Sleeps are needed as installs can happen too quickly for Kind to handle it
@sleep 30
kubectl create -f https://docs.projectcalico.org/archive/v3.24/manifests/custom-resources.yaml
@sleep 20

# ===========> Shared Targets

Expand Down
7 changes: 7 additions & 0 deletions acceptance/framework/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type TestConfig struct {

EnablePodSecurityPolicies bool

EnableCNI bool

EnableTransparentProxy bool

DisablePeering bool
Expand Down Expand Up @@ -83,6 +85,11 @@ func (t *TestConfig) HelmValuesFromConfig() (map[string]string, error) {
setIfNotEmpty(helmValues, "global.enablePodSecurityPolicies", "true")
}

if t.EnableCNI {
setIfNotEmpty(helmValues, "connectInject.enabled", "true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should set connectInject. For tproxy we're only setting the tproxy value so that for tests that don't use connect inject you don't end up running it because of the global flag.

Copy link
Contributor Author

@curtbushko curtbushko Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

Changed.

setIfNotEmpty(helmValues, "connectInject.cni.enabled", "true")
}

setIfNotEmpty(helmValues, "connectInject.transparentProxy.defaultEnabled", strconv.FormatBool(t.EnableTransparentProxy))

setIfNotEmpty(helmValues, "global.image", t.ConsulImage)
Expand Down
12 changes: 11 additions & 1 deletion acceptance/framework/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,17 @@ func TestConfig_HelmValuesFromConfig(t *testing.T) {
"connectInject.transparentProxy.defaultEnabled": "true",
},
},
{
"sets connectInject.cni.enabled helm value to true when -enable-cni is set",
TestConfig{
EnableCNI: true,
},
map[string]string{
"connectInject.enabled": "true",
"connectInject.cni.enabled": "true",
"connectInject.transparentProxy.defaultEnabled": "false",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -146,7 +157,6 @@ func TestConfig_HelmValuesFromConfig_EntImage(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.consulImage, func(t *testing.T) {

// Write values.yaml to a temp dir which will then get parsed.
valuesYAML := fmt.Sprintf(`global:
image: %s
Expand Down
10 changes: 10 additions & 0 deletions acceptance/framework/environment/cni-kind/kind.config
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
networking:
# Calicos default subnet. Needed for Calico to run on kind
podSubnet: 192.168.0.0/16
serviceSubnet: 10.110.0.0/16
# The default kind.net CNI plugin will not be installed
disableDefaultCNI: true
nodes:
- role: control-plane
8 changes: 8 additions & 0 deletions acceptance/framework/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ type TestFlags struct {

flagEnablePodSecurityPolicies bool

flagEnableCNI bool

flagEnableTransparentProxy bool

flagConsulImage string
Expand Down Expand Up @@ -85,6 +87,10 @@ func (t *TestFlags) init() {
flag.BoolVar(&t.flagEnablePodSecurityPolicies, "enable-pod-security-policies", false,
"If true, the test suite will run tests with pod security policies enabled.")

flag.BoolVar(&t.flagEnableCNI, "enable-cni", false,
"If true, the test suite will run tests with consul-cni plugin enabled. "+
"In general, this will only run against tests that are mesh related (connect, dns, mesh-gateway, peering, etc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dns test is not a mesh test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


flag.BoolVar(&t.flagEnableTransparentProxy, "enable-transparent-proxy", false,
"If true, the test suite will run tests with transparent proxy enabled. "+
"This applies only to tests that enable connectInject.")
Expand Down Expand Up @@ -142,6 +148,8 @@ func (t *TestFlags) TestConfigFromFlags() *config.TestConfig {

EnablePodSecurityPolicies: t.flagEnablePodSecurityPolicies,

EnableCNI: t.flagEnableCNI,

EnableTransparentProxy: t.flagEnableTransparentProxy,

DisablePeering: t.flagDisablePeering,
Expand Down
18 changes: 15 additions & 3 deletions acceptance/tests/connect/connect_inject_namespaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const staticServerNamespace = "ns1"
const StaticClientNamespace = "ns2"
const (
staticServerNamespace = "ns1"
StaticClientNamespace = "ns2"
)

// Test that Connect works with Consul Enterprise namespaces.
// These tests currently only test non-secure and secure without auto-encrypt installations
Expand Down Expand Up @@ -71,6 +73,7 @@ func TestConnectInjectNamespaces(t *testing.T) {
helmValues := map[string]string{
"global.enableConsulNamespaces": "true",
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to set it here? wouldn't it be set by the framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. I am setting this all over the place and there is no need. I only discovered my mistake after I started the GKE acceptance tests.

I will remove and shrink this PR by quite a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have been removed.

// When mirroringK8S is set, this setting is ignored.
"connectInject.consulNamespaces.consulDestinationNamespace": c.destinationNamespace,
"connectInject.consulNamespaces.mirroringK8S": strconv.FormatBool(c.mirrorK8S),
Expand Down Expand Up @@ -226,7 +229,15 @@ func TestConnectInjectNamespaces(t *testing.T) {
// from server, which is the case when a connection is unsuccessful due to intentions in other tests.
logger.Log(t, "checking that connection is unsuccessful")
if cfg.EnableTransparentProxy {
k8s.CheckStaticServerConnectionMultipleFailureMessages(t, staticClientOpts, StaticClientName, false, []string{"curl: (56) Recv failure: Connection reset by peer", "curl: (52) Empty reply from server", "curl: (7) Failed to connect to static-server.ns1 port 80: Connection refused"}, "", fmt.Sprintf("http://static-server.%s", staticServerNamespace))
k8s.CheckStaticServerConnectionMultipleFailureMessages(
t,
staticClientOpts,
StaticClientName,
false,
[]string{"curl: (56) Recv failure: Connection reset by peer", "curl: (52) Empty reply from server", "curl: (7) Failed to connect to static-server.ns1 port 80: Connection refused"},
"",
fmt.Sprintf("http://static-server.%s", staticServerNamespace),
)
} else {
k8s.CheckStaticServerConnectionMultipleFailureMessages(t, staticClientOpts, StaticClientName, false, []string{"curl: (56) Recv failure: Connection reset by peer", "curl: (52) Empty reply from server"}, "", "http://localhost:1234")
}
Expand Down Expand Up @@ -285,6 +296,7 @@ func TestConnectInjectNamespaces_CleanupController(t *testing.T) {
helmValues := map[string]string{
"global.enableConsulNamespaces": "true",
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
// When mirroringK8S is set, this setting is ignored.
"connectInject.consulNamespaces.consulDestinationNamespace": c.destinationNamespace,
"connectInject.consulNamespaces.mirroringK8S": strconv.FormatBool(c.mirrorK8S),
Expand Down
13 changes: 9 additions & 4 deletions acceptance/tests/connect/connect_inject_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ func TestConnectInject_CleanupKilledPods(t *testing.T) {

helmValues := map[string]string{
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
"global.tls.enabled": strconv.FormatBool(c.secure),
"global.tls.enableAutoEncrypt": strconv.FormatBool(c.autoEncrypt),
"global.acls.manageSystemACLs": strconv.FormatBool(c.secure),
Expand Down Expand Up @@ -266,7 +267,8 @@ func TestConnectInject_RestartConsulClients(t *testing.T) {
ctx := suite.Environment().DefaultContext(t)

helmValues := map[string]string{
"connectInject.enabled": "true",
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
}

releaseName := helpers.RandomName()
Expand Down Expand Up @@ -301,8 +303,10 @@ func TestConnectInject_RestartConsulClients(t *testing.T) {
}
}

const multiport = "multiport"
const multiportAdmin = "multiport-admin"
const (
multiport = "multiport"
multiportAdmin = "multiport-admin"
)

// Test that Connect works for an application with multiple ports. The multiport application is a Pod listening on
// two ports. This tests inbound connections to each port of the multiport app, and outbound connections from the
Expand All @@ -329,7 +333,8 @@ func TestConnectInject_MultiportServices(t *testing.T) {
}

helmValues := map[string]string{
"connectInject.enabled": "true",
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),

"global.tls.enabled": strconv.FormatBool(c.secure),
"global.tls.enableAutoEncrypt": strconv.FormatBool(c.autoEncrypt),
Expand Down
3 changes: 3 additions & 0 deletions acceptance/tests/controller/controller_namespaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ const (
// and non-auto-encrypt secure installations, so testing just one is enough.
func TestControllerNamespaces(t *testing.T) {
cfg := suite.Config()
if cfg.EnableCNI {
t.Skipf("skipping because -enable-cni is set and controller is already tested with regular tproxy")
}
if !cfg.EnableEnterprise {
t.Skipf("skipping this test because -enable-enterprise is not set")
}
Expand Down
4 changes: 3 additions & 1 deletion acceptance/tests/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ const (

func TestController(t *testing.T) {
cfg := suite.Config()

if cfg.EnableCNI {
t.Skipf("skipping because -enable-cni is set and controller is already tested with regular tproxy")
}
cases := []struct {
secure bool
autoEncrypt bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ func TestIngressGatewaySingleNamespace(t *testing.T) {
// Install the Helm chart without the ingress gateway first
// so that we can create the namespace for it.
helmValues := map[string]string{
"connectInject.enabled": "true",
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
"connectInject.consulNamespaces.consulDestinationNamespace": testNamespace,

"global.enableConsulNamespaces": "true",
Expand Down Expand Up @@ -187,6 +188,7 @@ func TestIngressGatewayNamespaceMirroring(t *testing.T) {
// so that we can create the namespace for it.
helmValues := map[string]string{
"connectInject.enabled": "true",
"connectInject.cni.enabled": strconv.FormatBool(cfg.EnableCNI),
"connectInject.consulNamespaces.mirroringK8S": "true",

"global.enableConsulNamespaces": "true",
Expand Down
Loading