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.

Sign up for GitHub

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

Enable e2e tests using emulators for cloud providers [ aws, gcp, azure ] #743

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+1,749 −601
Open

Enable e2e tests using emulators for cloud providers [ aws, gcp, azure ] #743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd4a097
Enable e2e tests with cloud provider emulators for aws,gcp,azure
anveshreddy18 May 21, 2024
8b7bc47
Minor cleanup
anveshreddy18 May 22, 2024
5454c29
Remove kubeconfig and add it to .gitignore
anveshreddy18 May 22, 2024
3f37a55
Fix make check error
anveshreddy18 May 22, 2024
8d1c313
add docs, cleanup
anveshreddy18 May 29, 2024
b7580db
Add steps for setup,test,clean & fixed a failing test
anveshreddy18 Jun 6, 2024
c40c8b5
Setup makefile for installing required tools
anveshreddy18 Jun 10, 2024
a711dc5
Make git track hack/tools/bin dir
anveshreddy18 Jun 10, 2024
ec1496b
Update docs & delete the objects of gcp bucket in parallel
anveshreddy18 Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions .ci/integration_test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,11 @@ function create_etcd_data_directory() {
mkdir -p ${ETCD_DATA_DIR}
}

function get_aws_existing_region() {
function get_aws_existing_credentials() {
export REGION=`cat ${HOME}/.aws/config | grep -e "^.*region.*$" | sed "s/^.*region[ ]*=[ ]*//"`
export AWS_DEFAULT_REGION=${REGION}
export AWS_ACCESS_KEY_ID=`cat ${HOME}/.aws/credentials | grep -e "^.*aws_access_key_id.*$" | sed "s/^.*aws_access_key_id[ ]*=[ ]*//"`
export AWS_SECRET_ACCESS_KEY=`cat ${HOME}/.aws/credentials | grep -e "^.*aws_secret_access_key.*$" | sed "s/^.*aws_secret_access_key[ ]*=[ ]*//"`
Comment on lines +130 to +132
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. If the credentials can be fetched from the .aws folder, or from an applicationCredentials JSON file, then I would prefer that over exposing the env vars

}

#############################
Expand Down Expand Up @@ -155,6 +158,9 @@ EOF
}
EOF
export AWS_APPLICATION_CREDENTIALS_JSON="${credentials_file}"
export AWS_ACCESS_KEY_ID=$1
Copy link
Member

Choose a reason for hiding this comment

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

Why are these variables being exported as env variables when they weren't needed to be exposed before?
The aws cli picks up credentials from .aws that is created in this function anyway, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

I would like to avoid using environment variables to pass credential information to the etcd-backup-restore process (not just for the main codebase, but for the tests as well).

export AWS_SECRET_ACCESS_KEY=$2
export AWS_DEFAULT_REGION=$3
}

function create_aws_secret() {
Expand Down Expand Up @@ -192,7 +198,7 @@ function setup-aws-infrastructure() {
if [[ "${USE_EXISTING_AWS_SECRET}" == "1" ]]; then
create_aws_secret
else
get_aws_existing_region
get_aws_existing_credentials
fi
create_s3_bucket
echo "AWS infrastructure setup completed."
Expand Down Expand Up @@ -256,7 +262,7 @@ function run_test_as_processes() {
setup_test_cluster

echo "Starting integration tests..."
cd test/e2e/integration
cd test/integration

set +e
ginkgo -r -mod=vendor
Expand Down Expand Up @@ -287,18 +293,18 @@ function run_test_on_cluster() {
setup-aws-infrastructure
fi

export ETCD_VERSION=${ETCD_VERSION:-"v3.4.13-bootstrap-1"}
export ETCD_VERSION=${ETCD_VERSION:-"v0.1.1"}
echo "Etcd version: ${ETCD_VERSION}"

export ETCDBR_VERSION=${ETCDBR_VERSION:-${ETCDBR_VER:-"v0.12.1"}}
export ETCDBR_VERSION=${ETCDBR_VERSION:-${ETCDBR_VER:-"v0.29.0-dev"}}
Comment on lines +296 to +299
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please update the etcd-wrapper and etcd-backup-restore versions to the latest versions?

echo "Etcd-backup-restore version: ${ETCDBR_VERSION}"

echo "Starting integration tests on k8s cluster."

set +e

if [ -r "$INTEGRATION_TEST_KUBECONFIG" ]; then
KUBECONFIG=$INTEGRATION_TEST_KUBECONFIG STORAGE_CONTAINER=$TEST_ID ginkgo -v -timeout=15m -mod=vendor test/e2e/integrationcluster
KUBECONFIG=$INTEGRATION_TEST_KUBECONFIG STORAGE_CONTAINER=$TEST_ID PROVIDER=aws ginkgo -v -timeout=15m -mod=vendor test/integrationcluster
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we defaulting to aws here, given that the PR adds support for all the 3 major providers? Shouldn't the providers option be taking from the args passed to the integration_test script?

TEST_RESULT=$?
echo "Successfully completed all tests."
else
Expand Down
6 changes: 5 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
.glide/

# Build Binaries
bin
/bin
/hack/tools/bin
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to add the / in front of bin and hack?
It would be cleaner if these lines are consistent with the rest of the file where / is not added to the beginning.

test/output
test/e2e_test_data
default.bkp*
Expand All @@ -28,6 +29,9 @@ compctedsnap.bkp*
.vscode
.idea/

# kubeconfig
hack/e2e-test/infrastructure/kind/kubeconfig

# developers workspace
tmp
dev
Expand Down
45 changes: 44 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,22 @@
# SPDX-License-Identifier: Apache-2.0

VERSION ?= $(shell cat VERSION)
REPO_ROOT := $(shell dirname "$(realpath $(lastword $(MAKEFILE_LIST)))")
REGISTRY ?= europe-docker.pkg.dev/gardener-project/snapshots
IMAGE_REPOSITORY := $(REGISTRY)/gardener/etcdbrctl
IMAGE_TAG := $(VERSION)
BUILD_DIR := build
BIN_DIR := bin
COVERPROFILE := test/output/coverprofile.out
KUBECONFIG_PATH :=$(REPO_ROOT)/hack/e2e-test/infrastructure/kind/kubeconfig

IMG ?= ${IMAGE_REPOSITORY}:${IMAGE_TAG}

.DEFAULT_GOAL := build-local

TOOLS_DIR := hack/tools
include $(REPO_ROOT)/hack/tools.mk
Comment on lines +19 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Etcdbr master branch now includes

etcd-backup-restore/Makefile

Line 17 in 867f84d

include hack/tools.mk

Can you please replace that with your changes - basically rebase on latest master and take care of the merge conflicts.


.PHONY: revendor
revendor:
@env GO111MODULE=on go mod tidy -v
Expand All @@ -24,6 +29,8 @@ update-dependencies:
@env GO111MODULE=on go get -u
@make revendor

kind-up kind-down ci-e2e-kind deploy-localstack deploy-fakegcs deploy-azurite test-e2e: export KUBECONFIG = $(KUBECONFIG_PATH)

.PHONY: build
build:
@.ci/build
Expand Down Expand Up @@ -65,10 +72,46 @@ integration-test:
@.ci/integration_test

.PHONY: integration-test-cluster
integration-test-cluster:
integration-test-cluster: $(KIND) $(HELM) $(GINKGO) $(KUBECTL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't we want to rename integration-test-cluster to e2e instead? That also means renaming the test/integrationcluster dir to test/e2e, correct?

@.ci/integration_test cluster

.PHONY: show-coverage
show-coverage:
@if [ ! -f $(COVERPROFILE) ]; then echo "$(COVERPROFILE) is not yet built. Please run 'COVER=true make test'"; false; fi
@go tool cover -html $(COVERPROFILE)

.PHONY: test-e2e
test-e2e: $(KIND) $(HELM) $(GINKGO) $(KUBECTL)
@"$(REPO_ROOT)/hack/e2e-test/run-e2e-test.sh" $(PROVIDERS)

.PHONY: kind-up
kind-up: $(KIND)
./hack/kind-up.sh

.PHONY: kind-down
kind-down: $(KIND)
kind delete cluster --name etcdbr-e2e

.PHONY: deploy-localstack
deploy-localstack: $(KUBECTL)
./hack/deploy-localstack.sh

.PHONY: deploy-fakegcs
deploy-fakegcs: $(KUBECTL)
./hack/deploy-fakegcs.sh

.PHONY: deploy-azurite
deploy-azurite: $(KUBECTL)
./hack/deploy-azurite.sh

.PHONY: ci-e2e-kind
ci-e2e-kind:
./hack/ci-e2e-kind.sh $(PROVIDERS)

.PHONY: pr-test-e2e
pr-test-e2e:
./hack/ci-e2e-kind.sh aws

.PHONY: merge-test-e2e
merge-test-e2e:
./hack/ci-e2e-kind.sh aws,gcp,azure
Comment on lines +111 to +117
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice touch, running just one provider on PRs and against all providers on PR merge (head-update). But is it possible to simply provider arguments to make ci-e2e-kind itself, since the Make targets should not really be aware of how the tests are triggered/run in the pipeline, correct?

And how is this implemented in the pipeline? Will you also introduce prow jobs that run these specific make targets upon specific triggers, like pr-build and head-update (I'm not aware if we can run a prow job on head-update and somehow get notified of the test run results in a meaningful way). Is there possibly a way to specify to prow which providers to run against, using github PR labels maybe? For instance, adding label test/e2e/aws should run a prow job for ./hack/ci-e2e-kind.sh aws and test/e2e/all should run prow job for ./hack/ci-e2e-kind.sh aws,gcp,azure.

The advantage with this approach is that we can choose which labels to set on which PRs, for instance a PR that touches GCS snapstore code can use label test/e2e/gcp so that GCP-based e2e tests are run on every PR update, and finally when the PR is about to be merged, test/e2e/all is set to run a final prow job against all providers before PR merge is allowed. WDYT?

6 changes: 6 additions & 0 deletions chart/etcd-backup-restore/templates/etcd-backup-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ data:
region: {{ .Values.backup.s3.region | b64enc }}
secretAccessKey: {{ .Values.backup.s3.secretAccessKey | b64enc }}
accessKeyID: {{ .Values.backup.s3.accessKeyID | b64enc }}
{{- if .Values.backup.s3.endpoint }}
endpoint: {{ .Values.backup.s3.endpoint | b64enc }}
{{- end }}
{{- if .Values.backup.s3.s3ForcePathStyle }}
s3ForcePathStyle: {{ .Values.backup.s3.s3ForcePathStyle | b64enc }}
{{- end }}
{{- else if eq .Values.backup.storageProvider "ABS" }}
storageAccount: {{ .Values.backup.abs.storageAccount | b64enc }}
storageKey : {{ .Values.backup.abs.storageKey | b64enc }}
Expand Down
8 changes: 8 additions & 0 deletions chart/etcd-backup-restore/templates/etcd-client-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,11 @@ spec:
protocol: TCP
port: {{ .Values.servicePorts.client }}
targetPort: {{ .Values.servicePorts.client }}
- name: server
Copy link
Collaborator

Choose a reason for hiding this comment

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

Review comments provided in #813 (review). Please rebase this PR on master branch once #813 is merged.

protocol: TCP
port: {{ .Values.servicePorts.server }}
targetPort: {{ .Values.servicePorts.server }}
- name: backuprestore
protocol: TCP
port: {{ .Values.servicePorts.backupRestore }}
targetPort: {{ .Values.servicePorts.backupRestore }}
18 changes: 16 additions & 2 deletions chart/etcd-backup-restore/templates/etcd-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ data:
# Number of committed transactions to trigger a snapshot to disk.
snapshot-count: 75000

enable-v2: false

# Raise alarms when backend size exceeds the given quota. 0 means use the
# default quota.
{{- if .Values.backup.etcdQuotaBytes }}
Expand All @@ -31,12 +33,24 @@ data:
# List of comma separated URLs to listen on for client traffic.
listen-client-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.client }}

# List of comma separated URLs to listen on for peer traffic.
listen-peer-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.server }}

# List of this member's client URLs to advertise to the public.
# The URLs needed to be a comma-separated list.
advertise-client-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.client }}
advertise-client-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}@{{ .Release.Name }}-etcd-peer@{{ .Release.Namespace }}@{{ .Values.servicePorts.client }}
# advertise-client-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.client }}


# List of this member's peer URLs to advertise to the public.
# The URLs needed to be a comma-separated list.
initial-advertise-peer-urls: {{ if .Values.etcdTLS }}https{{ else }}http{{ end }}@{{ .Release.Name }}-etcd-peer@{{ .Release.Namespace }}@{{ .Values.servicePorts.server }}

# List of server endpoints with which this cluster should be started
initial-cluster: {{ .Release.Name }}-etcd-0={{ if .Values.etcdTLS }}https{{ else }}http{{ end }}://{{ .Release.Name }}-etcd-0.{{ .Release.Name }}-etcd-peer.{{ .Release.Namespace }}.svc:{{ .Values.servicePorts.server }}

# Initial cluster token for the etcd cluster during bootstrap.
initial-cluster-token: 'new'
initial-cluster-token: 'etcd-cluster'

# Initial cluster state ('new' or 'existing').
initial-cluster-state: 'new'
Expand Down
26 changes: 26 additions & 0 deletions chart/etcd-backup-restore/templates/etcd-peer-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
apiVersion: v1
kind: Service
metadata:
name: {{ .Release.Name }}-etcd-peer
namespace: {{ .Release.Namespace }}
labels:
app.kubernetes.io/name: etcd
app.kubernetes.io/managed-by: {{ .Release.Service }}
app.kubernetes.io/instance: {{ .Release.Name }}
spec:
publishNotReadyAddresses: true
type: ClusterIP
clusterIP: None
clusterIPs:
- None
internalTrafficPolicy: Cluster
imFamilyPolicy: SingleStack
sessionAffinity: None
selector:
app.kubernetes.io/name: etcd
app.kubernetes.io/instance: {{ .Release.Name }}
ports:
- name: peer
protocol: TCP
port: {{ .Values.servicePorts.server }}
targetPort: {{ .Values.servicePorts.server }}
Loading