-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add e2e tests for ProviderConfigs #1320
base: main
Are you sure you want to change the base?
Add e2e tests for ProviderConfigs #1320
Conversation
Signed-off-by: Erhan Cagirici <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only made it partway through before having to pause my review. So far my comments are mostly just questions or optional suggestions.
# It first builds and publishes the | ||
# provider-family-aws, provider-aws-ec2 and provider-aws-rds. | ||
# Then triggers the e2e provider config tests via `make`, | ||
# which resides in the `e2e/providerconfig-aws-e2e-test` directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a name that's longer than necessary to convey the information. How about just e2e/providerconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the renaming the folder. My reasoning was, since this directory is also an XP configuration package, I just named it the same with the configuration package name.
The reason for this long name for the configuration package is that we could have other testing configuration packages for other providers as well.
@@ -209,6 +209,38 @@ uptest: $(UPTEST) $(KUBECTL) $(KUTTL) | |||
@KUBECTL=$(KUBECTL) KUTTL=$(KUTTL) CROSSPLANE_NAMESPACE=$(CROSSPLANE_NAMESPACE) $(UPTEST) e2e "${UPTEST_EXAMPLE_LIST}" --data-source="${UPTEST_DATASOURCE_PATH}" --setup-script=cluster/test/setup.sh --default-conditions="Test" || $(FAIL) | |||
@$(OK) running automated tests | |||
|
|||
# This target triggers an e2e test for testing provider configs. | |||
# It first builds and publishes the | |||
# provider-family-aws, provider-aws-ec2 and provider-aws-rds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choosing two providers plus the family makes sense, but ec2 is by far the largest provider with 100 managed resource kinds. We might get better performance with the same confidence of catching regressions using two smaller providers that have a cross reference. How about kms and secretsmanager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of these tests, we only aim to test different provider configs. Any type of MR, therefore provider, works. We are not particularly interested in the External Resource itself, we just want to create MRs pointing to different providerconfigs in test.
I have no strong opinion on the resources/providers used for testing.
The reason behind EC2 VPC
and RDS ParameterGroup
is that they have
- relatively fast creation & readiness time,
- can be created standalone without any extra AWS resource
- does not incur extra costs during testing or if somehow they become orphan
The reason for having 2 providers is that, IRSA tests have injected identity in the pod, I just wanted to have an isolated environment for other tests in a separate provider pod that does not have any IRSA config involved.
AWS_FAMILY_PACKAGE_IMAGE="$(XPKG_REG_ORGS)/provider-family-aws:$(VERSION)" \ | ||
AWS_EC2_PACKAGE_IMAGE="$(XPKG_REG_ORGS)/provider-aws-ec2:$(VERSION)" \ | ||
AWS_RDS_PACKAGE_IMAGE="$(XPKG_REG_ORGS)/provider-aws-rds:$(VERSION)" \ | ||
TARGET_CROSSPLANE_VERSION="1.15.2" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised to see this, as I thought there was a difference in crossplane versions >= 1.15.0 related to how it identifies family providers that resulted in the providers we build locally being identified as being from different families, and breaking access to the ProviderConfig and ProviderConfigUsage resources.
Assuming your tests are working, it would be great to figure out what's different about them and the current make e2e
config that causes make e2e
to fail with crossplane versions >= 1.15.0.
@@ -0,0 +1,106 @@ | |||
# Project Setup | |||
PROJECT_NAME := providerconfig-aws-e2e-test | |||
PROJECT_REPO := github.com/upbound/provider-upjet-aws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL returns a 404. I thought it was either github.com/crossplane-contrib/provider-upjet-aws
or github.com/upbound/provider-aws
. But maybe we're using a different convention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been some time since this PR was initiated, so probably it switched orgs after I opened the PR. Will fix this
# ==================================================================================== | ||
# Setup XPKG | ||
XPKG_DIR = $(shell pwd)/package | ||
XPKG_IGNORE = .github/workflows/*.yaml,.github/workflows/*.yml,examples/*.yaml,.work/uptest-datasource.yaml,.cache/**,_output/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to entirely exclude .github/**
, .work/**
, examples/**
and examples-generated/**
?
|
||
e2e-lite: build controlplane.up local.xpkg.deploy.configuration.$(PROJECT_NAME) | ||
|
||
uptest-e2e: $(UPTEST) $(KUBECTL) $(KUTTL) $(YQ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names are getting confusing. We already have make uptest
and make e2e
in the root makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erhancagirici LGTM. Thank you very much for your great effort in this PR and adding the very critical e2e tests for different type of provider configs.
KIND_VERSION = v0.22.0 | ||
UP_VERSION = v0.28.0 | ||
UP_CHANNEL = stable | ||
UPTEST_VERSION = v0.11.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please upgrade to the latest one v0.13.1
. Also, now we are using the differente source for downloading the uptest artifacts. Please see the UPTEST_LOCAL
in the root Makefile.
``` | ||
|
||
### Via Github Actions from PR | ||
TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do you think address these TBDs on this PR or future?
uptest-e2e: $(UPTEST) $(KUBECTL) $(KUTTL) $(YQ) | ||
@$(INFO) dump e2e claim: | ||
@mkdir -p "_output" | ||
@$(YQ) '(.spec.parameters.targetClusterParameters.provider.familyPackage = env(AWS_FAMILY_PACKAGE_IMAGE)) | \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding YQ to the build submodule worked here too 🎉
Description of your changes
Introduces automatede2e tests for provider configs,
provider-aws supports several provider config scenarios such as IRSA, WebIdentity. To test these scenarios, one needs to prepare an testing environment manually, and testing auth methods like IRSA is only possible with an EKS cluster .
The change aims to provide an automated standard testing environment in EKS and conduct e2e tests for several predetermined scenarios. It also provides a base for extending the tests.
For the testing environment and the tests a new configuration package is introduced specifically targeting e2e testing. It introduces a new composite resource
xe2etestclusters.platformref.aws.upbound.io
, that provisions an IRSA-enabled EKS, deploys crossplane, providers, provider configs, and some built-in testing MRs.Currently, built-in scenarios include:
Starting with a local crossplane control plane (possibly in a local KinD cluster), when a
E2ETestCluster.platformref.aws.upbound.io
claim is created, all above will be provisioned and tested. If further tests need to be conducted with different provider configs, one can keep the claim and apply desired provider config and MR manifests to the remote EKS testing cluster, viaprovider-kubernetes
& produced k8s provider config .See readme at path
e2e/providerconfig-aws-e2e-test/README.md
further details on Structure & Usage.I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Tested manually and the newly introduced
providerconfig-e2e
make target.