Skip to content

Commit 395512a

Browse files
authored
Merge pull request crossplane#6182 from jbw976/e2e-fixes
E2E improvements for claim server side apply
2 parents cd64e64 + 93f96e6 commit 395512a

File tree

5 files changed

+187
-16
lines changed

5 files changed

+187
-16
lines changed

go.mod

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ require (
4141
k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
4242
sigs.k8s.io/controller-runtime v0.19.0
4343
sigs.k8s.io/controller-tools v0.16.5
44-
sigs.k8s.io/e2e-framework v0.4.0
44+
sigs.k8s.io/e2e-framework v0.5.0
4545
sigs.k8s.io/kind v0.20.0
4646
sigs.k8s.io/yaml v1.4.0
4747
)
@@ -234,7 +234,7 @@ require (
234234
github.com/spf13/cobra v1.8.1 // indirect
235235
github.com/spf13/pflag v1.0.5 // indirect
236236
github.com/vbatts/tar-split v0.11.5 // indirect
237-
github.com/vladimirvivien/gexe v0.2.0 // indirect
237+
github.com/vladimirvivien/gexe v0.3.0 // indirect
238238
go.opentelemetry.io/otel v1.28.0 // indirect
239239
go.opentelemetry.io/otel/metric v1.28.0 // indirect
240240
go.opentelemetry.io/otel/trace v1.28.0 // indirect

go.sum

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -709,8 +709,8 @@ github.com/upbound/up-sdk-go v0.1.1-0.20240122203953-2d00664aab8e h1:aNzUuv4ZKH2
709709
github.com/upbound/up-sdk-go v0.1.1-0.20240122203953-2d00664aab8e/go.mod h1:IDIbYDb9fbedtxCc2CrdGcVRol6la7z2gkKh0VYWVGk=
710710
github.com/vbatts/tar-split v0.11.5 h1:3bHCTIheBm1qFTcgh9oPu+nNBtX+XJIupG/vacinCts=
711711
github.com/vbatts/tar-split v0.11.5/go.mod h1:yZbwRsSeGjusneWgA781EKej9HF8vme8okylkAeNKLk=
712-
github.com/vladimirvivien/gexe v0.2.0 h1:nbdAQ6vbZ+ZNsolCgSVb9Fno60kzSuvtzVh6Ytqi/xY=
713-
github.com/vladimirvivien/gexe v0.2.0/go.mod h1:LHQL00w/7gDUKIak24n801ABp8C+ni6eBht9vGVst8w=
712+
github.com/vladimirvivien/gexe v0.3.0 h1:4xwiOwGrDob5OMR6E92B9olDXYDglXdHhzR1ggYtWJM=
713+
github.com/vladimirvivien/gexe v0.3.0/go.mod h1:fp7cy60ON1xjhtEI/+bfSEIXX35qgmI+iRYlGOqbBFM=
714714
github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
715715
github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=
716716
github.com/xanzy/go-gitlab v0.103.0 h1:J9pTQoq0GsEFqzd6srCM1QfdfKAxSNz6mT6ntrpNF2w=
@@ -994,8 +994,8 @@ sigs.k8s.io/controller-runtime v0.19.0 h1:nWVM7aq+Il2ABxwiCizrVDSlmDcshi9llbaFbC
994994
sigs.k8s.io/controller-runtime v0.19.0/go.mod h1:iRmWllt8IlaLjvTTDLhRBXIEtkCK6hwVBJJsYS9Ajf4=
995995
sigs.k8s.io/controller-tools v0.16.5 h1:5k9FNRqziBPwqr17AMEPPV/En39ZBplLAdOwwQHruP4=
996996
sigs.k8s.io/controller-tools v0.16.5/go.mod h1:8vztuRVzs8IuuJqKqbXCSlXcw+lkAv/M2sTpg55qjMY=
997-
sigs.k8s.io/e2e-framework v0.4.0 h1:4yYmFDNNoTnazqmZJXQ6dlQF1vrnDbutmxlyvBpC5rY=
998-
sigs.k8s.io/e2e-framework v0.4.0/go.mod h1:JilFQPF1OL1728ABhMlf9huse7h+uBJDXl9YeTs49A8=
997+
sigs.k8s.io/e2e-framework v0.5.0 h1:YLhk8R7EHuTFQAe6Fxy5eBzn5Vb+yamR5u8MH1Rq3cE=
998+
sigs.k8s.io/e2e-framework v0.5.0/go.mod h1:jJSH8u2RNmruekUZgHAtmRjb5Wj67GErli9UjLSY7Zc=
999999
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=
10001000
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0=
10011001
sigs.k8s.io/kind v0.20.0 h1:f0sc3v9mQbGnjBUaqSFST1dwIuiikKVGgoTwpoP33a8=

test/e2e/README.md

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ Composition, etc. They should not take dependencies on 'real' Crossplane
1313
providers or external systems. Instead they use 'fake' providers like
1414
`provider-nop` and `provider-dummy`.
1515

16-
All Crossplane features must be exercised by these tests, as well as unit tests.
16+
All Crossplane features must be exercised by these tests, as well as unit tests.
1717

1818
## Running Tests
1919

@@ -38,7 +38,7 @@ earthly -P +e2e --FLAGS="-labels area=apiextensions"
3838
earthly -P +e2e --FLAGS="-feature=ConfigurationWithDependency"
3939

4040
# Stop immediately on first test failure, and leave the kind cluster to debug.
41-
earthly -i -P +e2e --FLAGS="-test.failfast -destroy-kind-cluster=false"
41+
earthly -i -P +e2e --FLAGS="-test.failfast -fail-fast -destroy-kind-cluster=false"
4242

4343
# Run a specific test suite.
4444
earthly -P +e2e --FLAGS="-test.v -test-suite=composition-webhook-schema-validation"
@@ -89,6 +89,44 @@ $ apk add kubectl
8989
# Now you can use kubectl to access the cluster
9090
```
9191

92+
### Test Logging
93+
94+
The E2E tests are run using [`gotestsum`] that helps output the test results in
95+
a format that integrates easily with CI systems. The downside though is that
96+
test logging statements, like `t.Logf("foo")`, are not streamed in real time and
97+
they are only included in the output if there is a failure.
98+
99+
To change this behavior when running the E2E tests locally, you can update the
100+
`Earthfile` to call `gotestsum` with a format like `--format standard-verbose`
101+
instead, for example:
102+
103+
```Dockerfile
104+
RUN gotestsum --no-color=false --format standard-verbose ...
105+
```
106+
107+
### Running Tests Directly
108+
109+
The E2E tests are typically run via `earthly` for convenience and consistency.
110+
However, this introduces additional layers that can make it challenging to debug
111+
issues (see [Accessing the Test Cluster](#accessing-the-test-cluster) above). An
112+
alternative is to run the tests more directly by basically manually running a
113+
couple critical commands that `earthly` would normally run for you inside of its
114+
build container.
115+
116+
First compile the E2E test binary:
117+
```shell
118+
go test -c -o e2e ./test/e2e
119+
```
120+
121+
Then run the `e2e` test binary, passing your specific flags and specific tests
122+
you want to execute if desired:
123+
```shell
124+
./e2e -v=4 -test.v -test.failfast -fail-fast -destroy-kind-cluster=false -test.run ^TestPropagateFieldsRemovalToXRAfterUpgrade
125+
```
126+
127+
Note the command above doesn't clean up the `kind` cluster when finished, so
128+
you'll need to manually delete it when you're done.
129+
92130
## Test Parallelism
93131

94132
`earthly -P +e2e` runs all defined E2E tests serially. Tests do not run in
@@ -98,7 +136,7 @@ etc. It's easier and less error-prone to write tests when you don't have to
98136
worry about one test potentially conflicting with another - for example by
99137
installing the same provider another test would install.
100138

101-
The [CI GitHub workflow] uses a matrix strategy to run multiple jobs in parallel,
139+
The [CI GitHub workflow] uses a matrix strategy to run multiple jobs in parallel,
102140
each running a test suite, see the dedicated section for more details.
103141

104142
We are currently splitting the tests to be able to run all basic tests against
@@ -112,7 +150,7 @@ the area to the matrix or spinning multiple kind clusters for each job.
112150
## Test Suite
113151

114152
In order to be able to run specific subsets of tests, we introduced the concept
115-
of test suites. To run a specific test suite use the `-test-suite` flag.
153+
of test suites. To run a specific test suite use the `-test-suite` flag.
116154

117155
A test suite is currently defined by:
118156
- A key to be used as value of the `-test-suite` flag.
@@ -292,3 +330,4 @@ Refer to the [E2E one-pager] for more context.
292330
[`testing`]: https://pkg.go.dev/testing
293331
[`e2e-framework`]: https://pkg.go.dev/sigs.k8s.io/e2e-framework
294332
[E2e one-pager]: ../../design/one-pager-e2e-tests.md
333+
[`gotestsum`]: https://github.com/gotestyourself/gotestsum

test/e2e/apiextensions_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,9 @@ func TestPropagateFieldsRemovalToXRAfterUpgrade(t *testing.T) {
304304
// explicitly disable them first before we create anything.
305305
WithSetup("DisableSSAClaims", funcs.AllOf(
306306
funcs.AsFeaturesFunc(environment.HelmUpgradeCrossplaneToSuite(config.TestSuiteDefault)), // Disable our feature flag.
307+
funcs.ArgUnsetWithin(1*time.Minute, "--enable-ssa-claims", namespace, "crossplane"),
307308
funcs.ReadyToTestWithin(1*time.Minute, namespace),
309+
funcs.DeploymentPodIsRunningMustNotChangeWithin(10*time.Second, namespace, "crossplane"),
308310
)).
309311
WithSetup("PrerequisitesAreCreated", funcs.AllOf(
310312
funcs.ApplyResources(FieldManager, manifests, "setup/*.yaml"),
@@ -323,7 +325,9 @@ func TestPropagateFieldsRemovalToXRAfterUpgrade(t *testing.T) {
323325
// would end up sharing ownership with the old CSA field manager.
324326
Assess("EnableSSAClaims", funcs.AllOf(
325327
funcs.AsFeaturesFunc(environment.HelmUpgradeCrossplaneToSuite(SuiteSSAClaims)),
328+
funcs.ArgSetWithin(1*time.Minute, "--enable-ssa-claims", namespace, "crossplane"),
326329
funcs.ReadyToTestWithin(1*time.Minute, namespace),
330+
funcs.DeploymentPodIsRunningMustNotChangeWithin(10*time.Second, namespace, "crossplane"),
327331
)).
328332
Assess("UpdateClaim", funcs.AllOf(
329333
funcs.ApplyClaim(FieldManager, manifests, "claim-update.yaml"),

test/e2e/funcs/feature.go

Lines changed: 134 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,102 @@ func DeploymentBecomesAvailableWithin(d time.Duration, namespace, name string) f
133133
}
134134
}
135135

136+
// DeploymentPodIsRunningMustNotChangeWithin fails a test if the supplied Deployment does
137+
// not have a running Pod that stays running for the supplied duration.
138+
func DeploymentPodIsRunningMustNotChangeWithin(d time.Duration, namespace, name string) features.Func {
139+
return func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
140+
t.Helper()
141+
142+
dp := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}}
143+
t.Logf("Ensuring deployment %s/%s has running pod that does not change within %s", dp.GetNamespace(), dp.GetName(), d.String())
144+
start := time.Now()
145+
146+
pod, err := podForDeployment(ctx, t, c, dp)
147+
if err != nil {
148+
t.Errorf("Failed to get pod for deployment %s/%s: %s", dp.GetNamespace(), dp.GetName(), err)
149+
return ctx
150+
}
151+
152+
// first wait for pod to be running
153+
if err := wait.For(conditions.New(c.Client().Resources()).PodConditionMatch(pod, corev1.PodReady, corev1.ConditionTrue), wait.WithTimeout(d), wait.WithInterval(DefaultPollInterval)); err != nil {
154+
t.Errorf("Deployment %s/%s never got a running pod after %s", dp.GetNamespace(), dp.GetName(), since(start))
155+
return ctx
156+
}
157+
158+
// now wait to make sure the pod stays running (does not change)
159+
start = time.Now()
160+
if err := wait.For(conditions.New(c.Client().Resources()).PodConditionMatch(pod, corev1.PodReady, corev1.ConditionFalse), wait.WithTimeout(d), wait.WithInterval(DefaultPollInterval)); err != nil {
161+
if deadlineExceed(err) {
162+
t.Logf("Deployment %s/%s had running pod that did not change after %s", dp.GetNamespace(), dp.GetName(), since(start))
163+
return ctx
164+
}
165+
166+
t.Errorf("Error while observing pod for deployment %s/%s", dp.GetNamespace(), dp.GetName())
167+
return ctx
168+
}
169+
t.Errorf("Deployment %s/%s had pod that changed within %s, but it should not have", dp.GetNamespace(), dp.GetName(), d.String())
170+
return ctx
171+
}
172+
}
173+
174+
// ArgSetWithin fails a test if the supplied Deployment does not have a Pod with
175+
// the given argument set within the supplied duration.
176+
func ArgSetWithin(d time.Duration, arg, namespace, name string) features.Func {
177+
return checkArgSetWithin(d, arg, true, namespace, name)
178+
}
179+
180+
// ArgUnsetWithin fails a test if the supplied Deployment does not have a Pod with
181+
// the given argument unset within the supplied duration.
182+
func ArgUnsetWithin(d time.Duration, arg, namespace, name string) features.Func {
183+
return checkArgSetWithin(d, arg, false, namespace, name)
184+
}
185+
186+
// checkArgSetWithin implements a check for the supplied Deployment having a Pod
187+
// with the given argument being either set or unset within the supplied
188+
// duration.
189+
func checkArgSetWithin(d time.Duration, arg string, wantSet bool, namespace, name string) features.Func {
190+
return func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
191+
t.Helper()
192+
193+
dp := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}}
194+
t.Logf("Waiting %s for pod in deployment %s/%s to have arg %s set=%t...", d, dp.GetNamespace(), dp.GetName(), arg, wantSet)
195+
start := time.Now()
196+
197+
if err := wait.For(func(ctx context.Context) (done bool, err error) {
198+
pod, err := podForDeployment(ctx, t, c, dp)
199+
if err != nil {
200+
t.Logf("failed to get pod for deployment %s/%s: %s", dp.GetNamespace(), dp.GetName(), err)
201+
return false, nil
202+
}
203+
204+
found := false
205+
c := pod.Spec.Containers[0]
206+
for _, a := range c.Args {
207+
if a == arg {
208+
found = true
209+
}
210+
}
211+
212+
switch {
213+
case wantSet && !found:
214+
t.Logf("did not find arg %s within %s", arg, c.Args)
215+
return false, nil
216+
case !wantSet && found:
217+
t.Logf("unexpectedly found arg %s within %s", arg, c.Args)
218+
return false, nil
219+
default:
220+
return true, nil
221+
}
222+
}, wait.WithTimeout(d), wait.WithInterval(DefaultPollInterval)); err != nil {
223+
t.Fatal(err)
224+
return ctx
225+
}
226+
227+
t.Logf("Deployment %s/%s has pod with arg %s set=%t after %s", dp.GetNamespace(), dp.GetName(), arg, wantSet, since(start))
228+
return ctx
229+
}
230+
}
231+
136232
// ResourcesCreatedWithin fails a test if the supplied resources are not found
137233
// to exist within the supplied duration.
138234
func ResourcesCreatedWithin(d time.Duration, dir, pattern string, options ...decoder.DecodeOption) features.Func {
@@ -597,9 +693,10 @@ func ClaimUnderTestMustNotChangeWithin(d time.Duration) features.Func {
597693
if err := wait.For(conditions.New(c.Client().Resources()).ResourcesMatch(list, m), wait.WithTimeout(d)); err != nil {
598694
if deadlineExceed(err) {
599695
t.Logf("Claim %s did not change within %s", identifier(cm), d.String())
600-
} else {
601-
t.Errorf("Error while observing claim %s: %v", identifier(cm), err)
696+
return ctx
602697
}
698+
699+
t.Errorf("Error while observing claim %s: %v", identifier(cm), err)
603700
return ctx
604701
}
605702
t.Errorf("Claim %s changed within %s, but it should not have", identifier(cm), d.String())
@@ -644,9 +741,10 @@ func CompositeUnderTestMustNotChangeWithin(d time.Duration) features.Func {
644741
if err := wait.For(conditions.New(c.Client().Resources()).ResourcesMatch(list, m), wait.WithTimeout(d)); err != nil {
645742
if deadlineExceed(err) {
646743
t.Logf("Composite resource %s did not change within %s", identifier(cp), d.String())
647-
} else {
648-
t.Errorf("Error while observing composite resource %s: %v", identifier(cp), err)
744+
return ctx
649745
}
746+
747+
t.Errorf("Error while observing composite resource %s: %v", identifier(cp), err)
650748
return ctx
651749
}
652750
t.Errorf("Composite resource %s changed within %s, but it should not have", identifier(cp), d.String())
@@ -727,7 +825,7 @@ func CompositeResourceHasFieldValueWithin(d time.Duration, dir, claimFile, path
727825
return got != ""
728826
}
729827

730-
if err := wait.For(conditions.New(c.Client().Resources()).ResourceMatch(cm, hasResourceRef), wait.WithTimeout(d), wait.WithInterval(DefaultPollInterval)); err != nil {
828+
if err := wait.For(conditions.New(c.Client().Resources()).ResourceMatch(cm, hasResourceRef), wait.WithTimeout(d), wait.WithInterval(DefaultPollInterval), wait.WithImmediate()); err != nil {
731829
t.Errorf("Claim %q does not have a resourceRef to an XR: %v", cm.GetName(), err)
732830
return ctx
733831
}
@@ -1026,7 +1124,7 @@ func DeletionBlockedByUsageWebhook(dir, pattern string, options ...decoder.Decod
10261124
return ctx
10271125
}
10281126

1029-
if !strings.HasPrefix(err.Error(), "admission webhook \"nousages.apiextensions.crossplane.io\" denied the request") {
1127+
if !strings.Contains(err.Error(), "admission webhook \"nousages.apiextensions.crossplane.io\" denied the request") {
10301128
t.Fatalf("expected the usage webhook to deny the request but it failed with err: %s", err.Error())
10311129
return ctx
10321130
}
@@ -1168,3 +1266,33 @@ func since(t time.Time) string {
11681266
func deadlineExceed(err error) bool {
11691267
return errors.Is(err, context.DeadlineExceeded) || strings.Contains(err.Error(), "would exceed context deadline")
11701268
}
1269+
1270+
// podForDeployment returns the pod for a given Deployment. If the number of
1271+
// pods found is not exactly one, or that one pod does not have exactly one
1272+
// container, then this function returns an error.
1273+
func podForDeployment(ctx context.Context, t *testing.T, c *envconf.Config, dp *appsv1.Deployment) (*corev1.Pod, error) {
1274+
t.Helper()
1275+
if err := c.Client().Resources().Get(ctx, dp.GetName(), dp.GetNamespace(), dp); err != nil {
1276+
t.Logf("failed to get deployment %s/%s: %s", dp.GetNamespace(), dp.GetName(), err)
1277+
return nil, err
1278+
}
1279+
1280+
// use the deployment's selector to list all pods belonging to the deployment
1281+
selector := metav1.FormatLabelSelector(dp.Spec.Selector)
1282+
pods := &corev1.PodList{}
1283+
if err := c.Client().Resources().List(ctx, pods, resources.WithLabelSelector(selector)); err != nil {
1284+
t.Logf("failed to list pods for deployment %s/%s: %s", dp.GetNamespace(), dp.GetName(), err)
1285+
return nil, err
1286+
}
1287+
1288+
if len(pods.Items) != 1 {
1289+
return nil, errors.Errorf("expected 1 pod, found %d", len(pods.Items))
1290+
}
1291+
1292+
pod := pods.Items[0]
1293+
if len(pod.Spec.Containers) != 1 {
1294+
return nil, errors.Errorf("expected 1 container, found %d", len(pod.Spec.Containers))
1295+
}
1296+
1297+
return &pod, nil
1298+
}

0 commit comments

Comments
 (0)