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

Run integration tests with Enterprise version #1900

Merged
merged 14 commits into from
Oct 11, 2021
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Oct 5, 2021

What this PR does / why we need it:
Revamped version of #1758 reduced down to only test using KTF's Enterprise features and to pass workspace/RBAC configuration in when starting the test controller. It does not attempt to upgrade decK or fix the race conditions we currently encounter with it.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1279

Special notes for your reviewer:

  • Commit history is a bit messy between starting from an older PR and changes that touched the same areas of code and/or KTF. Will squash; they're left in for now because rebasing to try and clear them up before the PR doesn't play nice with the changes made to fix merge conflicts.
  • Earlier iterations selected the image from the test command. I think that's probably still a useful thing to have, but it's not strictly required, and we can rely on the default Enterprise image from KTF, hence the slightly odd commit (2219055) that removes image version stuff. In the interest of avoiding future drift from main and associated merge conflict fun, deferring that as something to revisit whenever/if we have an additional issue to add matrix testing of many versions.
  • The difference in secret/envvar names in 4aae9a9 is a bit odd, but I don't have repo permissions to rename the secret. If you do have permissions, feel free to revise that so that everything is KONG_LICENSE_DATA.

PR Readiness Checklist:
Changelog n/a, testing only.

Travis Raines and others added 7 commits September 28, 2021 14:21
Co-authored-by: Jimin <jimin.hu@konghq.com>
Use go-kong ExistsByName() API for retrieving workspaces. This allows
KIC to confirm a workspace's presence with a user that has access to
that workspace only. Previously, KIC could only retrieve workspaces
with an RBAC user that had access to the default workspace.

Co-authored-by: Jimin <jimin.hu@konghq.com>
This reverts commit 1029002.
Remove the image environment variable from the Enterprise tests. This
was a holdover from earlier development. The image and tag is now set in
KTF via WithProxyEnterpriseEnabled().
@rainest rainest requested a review from a team as a code owner October 5, 2021 23:53
@rainest rainest temporarily deployed to Configure ci October 5, 2021 23:54 Inactive
@rainest rainest temporarily deployed to Configure ci October 5, 2021 23:54 Inactive
@rainest rainest enabled auto-merge (squash) October 5, 2021 23:55
Set KONG_LICENSE_DATA from the KONG_ENTERPRISE_LICENSE Github secret.

KONG_LICENSE_DATA is the actual variable Kong reads, and what should be
set in the test environment.

KONG_ENTERPRISE_LICENSE is what we happened to name the repo secret that
populates it.
@rainest rainest temporarily deployed to Configure ci October 6, 2021 00:15 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 00:15 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 00:15 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 00:16 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 00:25 Inactive
shaneutt
shaneutt previously approved these changes Oct 6, 2021
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

No blockers, but several comments that I would like to see resolved.

.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Show resolved Hide resolved
.github/workflows/test.yaml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/integration/suite_test.go Outdated Show resolved Hide resolved
test/integration/suite_test.go Show resolved Hide resolved
test/integration/suite_test.go Outdated Show resolved Hide resolved
test/integration/suite_test.go Outdated Show resolved Hide resolved
test/integration/utils_test.go Show resolved Hide resolved
test/integration/utils_test.go Outdated Show resolved Hide resolved
@rainest rainest temporarily deployed to Configure ci October 6, 2021 16:44 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 16:44 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 16:49 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 16:49 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 16:56 Inactive
@rainest rainest temporarily deployed to Configure ci October 6, 2021 17:06 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 16:18 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 16:30 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 16:41 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 16:52 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 17:31 Inactive
@rainest rainest temporarily deployed to Configure ci October 11, 2021 17:40 Inactive
@rainest rainest requested a review from shaneutt October 11, 2021 18:06
@rainest
Copy link
Contributor Author

rainest commented Oct 11, 2021

E2E failure looks to be the same unrelated issue mentioned in #1914 (comment)

Re-requesting review as we should have actually included the change to workspace lookup in 2.0 GA, and will want to release a hotfix with that quickly.

I don't care about the secret/actual environment variable name matching much: again, I can't fix it completely, so if you do want to make it match throughout, please feel free to update the repo config and push a commit to change it.

Co-authored-by: Shane Utt <shaneutt@linux.com>
@rainest rainest temporarily deployed to Configure ci October 11, 2021 19:11 Inactive
Co-authored-by: Shane Utt <shaneutt@linux.com>
@rainest rainest temporarily deployed to Configure ci October 11, 2021 19:13 Inactive
Simplify test RBAC environment by just always using a static password.
The test doesn't need actual security, and the main qualities its
password needs is (a) to exist and allow RBAC testing at all and (b) to
be obvious to the tester during manual runs, in case test failures
require manual inspection of the admin API to clear.
@rainest rainest force-pushed the test/enterprise-integration branch from e7306db to 7852b40 Compare October 11, 2021 19:38
@rainest rainest temporarily deployed to Configure ci October 11, 2021 19:38 Inactive
@rainest rainest merged commit 9594670 into main Oct 11, 2021
@rainest rainest deleted the test/enterprise-integration branch October 11, 2021 19:47
@rainest rainest temporarily deployed to Configure ci October 11, 2021 19:47 Inactive
@rainest
Copy link
Contributor Author

rainest commented Oct 11, 2021

Integration failure was due to a leftover empty string placeholder variable in the args. That's now:

extraControllerArgs = append(extraControllerArgs, fmt.Sprintf("--kong-admin-token=%s", kongTestPassword))

I still cannot see why that broke on the healthz test, since that's just a raw httpc call, not something that uses the controller/admin API interaction. The broken parameter did break everything else though--running it locally ran into a fatal panic when the controller failed to retrieve Kong configuration--so maybe there was just something bizarre with the CI environment output?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KIC 2.0: Run all CI integration tests against Kong EE as well
2 participants