update e2e helm install #10632

jenshu · 2025-02-14T20:52:25Z

Simplify the install API used by e2e tests by removing the SoloTestHelper, removing fields that are hardcoded (e.g. test assets directory, index filename) or that can be derived (e.g. local chart uri), and calling the helm cli for install/uninstall.

Also removed the build ignore directives from all the files used by the base e2e test installation, to ensure they compile.
Enabling feature tests will be done in follow-up PRs.

other parts of this PR were previously broken out:

lint fix update lint config #10633
profiles e2e profiles #10634
readme and glooctl cleanup e2e readme and glooctl cleanup #10636

Signed-off-by: Jenny Shu <[email protected]>

jenshu · 2025-02-17T17:03:16Z

pkg/utils/helmutils/client.go

-// InstallOpts is a set of typical options for a helm install which can be passed in
-// instead of requiring the caller to remember the helm cli flags. extraArgs should
-// always be accepted and respected when using InstallOpts.
-type InstallOpts struct {


moved to install.go

jenshu · 2025-02-17T17:04:18Z

pkg/utils/helmutils/client.go

-func (c *Client) AddGlooRepository(ctx context.Context, extraArgs ...string) error {
-	return c.AddRepository(ctx, ChartName, ChartRepositoryUrl, extraArgs...)
-}
-
-func (c *Client) AddPrGlooRepository(ctx context.Context, extraArgs ...string) error {
-	return c.AddRepository(ctx, ChartName, PrChartRepositoryUrl, extraArgs...)
-}


no longer needed, we now use an oci registry which doesn't require helm repo add

jenshu · 2025-02-17T17:07:26Z

test/kubernetes/testutils/install/context.go

-	TestAssetDir string
-
-	// Helm chart name
-	HelmChartName string
-
-	// Name of the helm index file name
-	HelmRepoIndexFileName string


these were always hardcoded; removed

jenshu · 2025-02-17T17:09:09Z

test/kubernetes/testutils/helper/install.go

 )

-// Default test configuration
-var defaults = TestConfig{


glooctl stuff, not needed

jenshu · 2025-02-17T17:10:20Z

test/kubernetes/testutils/helper/install.go

-// Function to provide/override test configuration. Default values will be passed in.
-type TestConfigFunc func(defaults TestConfig) TestConfig
-
-type TestConfig struct {


removed TestConfig and SoloTestHelper in favor of just passing in the options directly via the helm client InstallOptions in pkg/utils/helmutils/install.go

jenshu · 2025-02-17T17:11:06Z

test/kubernetes/testutils/helper/install.go

-// there's an issue pulling the certgen docker image.
-// Without this timeout, it would just hang indefinitely.
-func glooctlInstallWithTimeout(rootDir string, opts *Options, timeout time.Duration) error {
-	return runWithTimeout(rootDir, opts, timeout, "install")


i think it's unlikely we'll run into timeout issues since we don't have jobs running as part of the helm install now, but we can always add this back later if needed

jenshu · 2025-02-17T17:13:43Z

test/kubernetes/testutils/install/context.go

-	ReleasedVersion string
-
-	// The version of the Helm chart. Calculated from either the chart or the released version. It will not have a leading 'v'
-	ChartVersion string
-
-	// The path to the local helm chart used for testing. Based on the TestAssertDir and relative to RootDir.
-	ChartUri string


instead of storing them on the Context (where they were never used), we just pass these in via the helm client InstallOptions

jenshu · 2025-02-17T17:14:48Z

test/kubernetes/e2e/tests/k8s_gw_tests.go

-	kubeGatewaySuiteRunner.Register("DirectResponse", directresponse.NewTestingSuite)
-	kubeGatewaySuiteRunner.Register("CRDCategories", crd_categories.NewTestingSuite)
-	kubeGatewaySuiteRunner.Register("Metrics", metrics.NewTestingSuite)
+	// kubeGatewaySuiteRunner.Register("Deployer", deployer.NewTestingSuite)


commented out the feature tests just to get the main SuiteRunner compiling.
the feature tests will be re-enabled individually

jenshu · 2025-02-17T17:16:36Z

test/kubernetes/e2e/test.go

-			helper.WithExtraArgs("--values", i.Metadata.ProfileValuesManifestFile),
-			helper.WithExtraArgs("--values", i.Metadata.ValuesManifestFile),
-		)
+func (i *TestInstallation) InstallKgatewayFromLocalChart(ctx context.Context) {


this replaces the previous TestHelper stuff. it simply gets the local chart path and calls the helm cli install func

nit: s/InstallKgatewayFromLocalChart/InstallFromLocalChart?

hmm I kind of like having the 'kgateway' in the name because there could be other things we install/uninstall during the tests (e.g. istio)

I think the chart name is implicit based on the "FromLocalChart" naming as we don't have multiple local charts (well I guess maybe the deployer chart counts here too).

We could also have an Install method that acts as a driver / hook for other suites to DI the dependencies that need to be installed (e.g. helm charts, binaries like istioctl, etc.). In this case, this method would take a variadic parameter and delegate to sub-install functions that match the expected function signature s.t. call sites can configure which hooks are needed.

timflannagan · 2025-02-17T17:33:49Z

Supports #10480.

timflannagan

Had a couple of questions, comments, etc. but nothing that isn't immediately blocking as long as we're tracking everything in the #10480 epic properly.

Just want to wrap my head around the order of operations here. You added some context that this PR is foundational and that enabling individual feature suites will be done as a follow-up. Are we planning on enabling this suite in our GHA workflows then?

timflannagan · 2025-02-17T17:24:16Z

go.mod

@@ -18,6 +18,7 @@ require (
 	github.com/golang/mock v1.6.0
 	github.com/google/go-cmp v0.6.0
 	github.com/grpc-ecosystem/go-grpc-middleware v1.4.0
+	github.com/hashicorp/go-multierror v1.1.1


We should rip this dependency out in favor of other methods for aggregating errors together (e.g. utilerrors.NewAggregate or the stlib errors.Join). This can be done in a follow-up since we're removing the build ignore flag and these are issues with the existing code.

filed #10643

timflannagan · 2025-02-17T17:25:23Z

pkg/utils/helmutils/install.go

Should we move this to internal? Any reason we'd need this to live in pkg/ right now?

Are we using this helmutils package anywhere outside of the test/ directory? I'm wondering whether we should move it there instead.

it was previously outside the test pkg because our CLI used this pkg. now that we don't have a CLI we could move it back into a test pkg but not sure what the plans are for adding back a CLI or if it will live in this repo

yep sgtm. let's get the main structure in place first and then revisit -- the priority is getting feedback on our extension APIs for PRs.

timflannagan · 2025-02-17T17:29:03Z

test/kubernetes/e2e/test.go

-			helper.WithExtraArgs("--values", i.Metadata.ProfileValuesManifestFile),
-			helper.WithExtraArgs("--values", i.Metadata.ValuesManifestFile),
-		)
+func (i *TestInstallation) InstallKgatewayFromLocalChart(ctx context.Context) {


nit: s/InstallKgatewayFromLocalChart/InstallFromLocalChart?

timflannagan · 2025-02-17T17:31:02Z

test/kubernetes/e2e/tests/base/base_suite.go

@@ -36,7 +35,7 @@ type SimpleTestCase struct {
 	// Resources expected to be created by manifest
 	Resources []client.Object
 	// values file passed during an upgrade
-	UpgradeValues string
+	// UpgradeValues string


nit: Add TODO comment like we did with the other uncommented function/code blocks?

mind if i handle that in the next PR so this one doesn't have to re-run CI again?

timflannagan · 2025-02-17T17:32:30Z

test/kubernetes/testutils/helper/util.go

-// Deprecated; if this is needed create a resource yaml for it.
-func GetHttpEchoImage() string {
-	httpEchoImage := "hashicorp/http-echo"
-	if runtime.GOARCH == "arm64" {
-		httpEchoImage = "gcr.io/solo-test-236622/http-echo:0.2.4"
-	}
-	return httpEchoImage
-}


Just curious -- was this dead code from before, or dead code as a result of ripping out some code with these changes?

looks like it was dead code even in the gloo repo

timflannagan · 2025-02-17T17:35:03Z

test/kubernetes/e2e/test.go


-func (i *TestInstallation) UninstallGlooGateway(ctx context.Context, uninstallFn func(ctx context.Context) error) {
+func (i *TestInstallation) UninstallKgateway(ctx context.Context) {


nit: Same comment as above. I think we can improve this name in addition to how the general install, upgrade, uninstall flow methods are implemented, but that can be done organically over time & a problem with the existing code.

see my response from above

jenshu added 19 commits February 13, 2025 22:20

cleanup

c1c37dc

remove old refs

f98ffbd

Signed-off-by: Jenny Shu <[email protected]>

Merge branch 'main' of github.com:jenshu/kgateway into compile-e2e-1

2b38e2b

Signed-off-by: Jenny Shu <[email protected]>

Merge branch 'main' of github.com:jenshu/kgateway into compile-e2e-1

96a3c14

Signed-off-by: Jenny Shu <[email protected]>

clean up helm client

3f00155

Signed-off-by: Jenny Shu <[email protected]>

Merge branch 'main' of github.com:jenshu/kgateway into compile-e2e-1

f0f892a

Signed-off-by: Jenny Shu <[email protected]>

delete file

c08189a

Signed-off-by: Jenny Shu <[email protected]>

uninstall

3a57562

Signed-off-by: Jenny Shu <[email protected]>

fix comments

717a1f1

Signed-off-by: Jenny Shu <[email protected]>

remove profiles

765c83a

Signed-off-by: Jenny Shu <[email protected]>

move file

691f131

Signed-off-by: Jenny Shu <[email protected]>

build

8fc1412

Signed-off-by: Jenny Shu <[email protected]>

replace profiles with common

34600ed

Signed-off-by: Jenny Shu <[email protected]>

remove unneeded context fields

fdacf9b

Signed-off-by: Jenny Shu <[email protected]>

renames

078ae1e

Signed-off-by: Jenny Shu <[email protected]>

remove gloocli

b41324a

Signed-off-by: Jenny Shu <[email protected]>

update common recs

a378d85

Signed-off-by: Jenny Shu <[email protected]>

remove gloo func

90401a3

Signed-off-by: Jenny Shu <[email protected]>

remove testhelper stuff, use new helm install

08aadbd

Signed-off-by: Jenny Shu <[email protected]>

jenshu marked this pull request as ready for review February 17, 2025 01:10

jenshu added 2 commits February 16, 2025 20:14

generate

835a617

Signed-off-by: Jenny Shu <[email protected]>

fix deprecated lint fields

076ff89

Signed-off-by: Jenny Shu <[email protected]>

jenshu mentioned this pull request Feb 17, 2025

e2e testing: do not rely on glooctl #10522

Closed

3 tasks

timflannagan mentioned this pull request Feb 17, 2025

e2e readme and glooctl cleanup #10636

Merged

Merge branch 'main' of github.com:jenshu/kgateway into compile-e2e-1

03226dd

Signed-off-by: Jenny Shu <[email protected]>

jenshu changed the title ~~[WIP] update e2e helm install~~ update e2e helm install Feb 17, 2025

jenshu added 3 commits February 17, 2025 11:39

rename kgateway context to install context

e15f0d8

Signed-off-by: Jenny Shu <[email protected]>

rename kgatewayContext to installContext

fab1bdb

Signed-off-by: Jenny Shu <[email protected]>

update comment

84282a2

Signed-off-by: Jenny Shu <[email protected]>

jenshu commented Feb 17, 2025

View reviewed changes

timflannagan approved these changes Feb 17, 2025

View reviewed changes

jenshu mentioned this pull request Feb 17, 2025

remove dependency on go-multierror #10643

Open

jenshu added this pull request to the merge queue Feb 17, 2025

Merged via the queue into kgateway-dev:main with commit 3ef5a55 Feb 17, 2025
9 checks passed

jenshu deleted the compile-e2e-1 branch February 17, 2025 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update e2e helm install #10632

update e2e helm install #10632

jenshu commented Feb 14, 2025 •

edited

Loading

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

timflannagan commented Feb 17, 2025

timflannagan left a comment

timflannagan Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

timflannagan Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

timflannagan Feb 17, 2025

timflannagan Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

jenshu Feb 17, 2025

timflannagan Feb 17, 2025

jenshu Feb 17, 2025


		func (i *TestInstallation) UninstallGlooGateway(ctx context.Context, uninstallFn func(ctx context.Context) error) {
		func (i *TestInstallation) UninstallKgateway(ctx context.Context) {

update e2e helm install #10632

update e2e helm install #10632

Conversation

jenshu commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan commented Feb 17, 2025

timflannagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshu commented Feb 14, 2025 •

edited

Loading