Remove Istio trust domain #9713

npolshakova · 2024-07-01T13:56:08Z

Description

Removes trust domain arg for Edge and K8s gateway.
Removes gloo edge gateway from e2e tests when k8s controller is enabled

Fill out any of the following sections that are relevant and remove the others

API changes

None

Code changes

Removes --trust-domain arg from Istio container

CI changes

Adds new Istio integration test e2e scenarios:

Gloo Edge Gateway w/t Istio installed with revisions
Gloo k8s Gateway w/t Istio installed with revisions

These tests were added to cluster-two since that cluster has a total runtime of 16m: https://github.com/solo-io/gloo/actions/runs/9787855207/job/27025026149

Docs changes

None

Context

We don't currently have a way to change the trust domain in the Edge Gateway or Gloo k8s Gateway.

This is not an issue because the istio-proxy is not running Envoy (the env DISABLE_ENVOY=true is set), only the agent for the CSR request. Leaving the --trust-domain flag can cause confusion since it's not configurable and does not need to be set for the agent to do the CSR.

See slack discussion: https://solo-io-corp.slack.com/archives/C07077NS0NP/p1719594402696959

Interesting decisions

None

Testing steps

Manually verification:

New e2e tests also confirm revisions work as expected.
Manually confirmed removing --trust-domain does not break Istio integration

Notes for reviewers

None

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves https://github.com/solo-io/solo-projects/issues/6472

solo-changelog-bot · 2024-07-01T14:06:59Z

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/6472

github-actions · 2024-07-01T15:24:32Z

Visit the preview URL for this PR (updated for commit 0ff8863):

https://gloo-edge--pr9713-npolshak-add-trust-d-bv7z2z6u.web.app

_{(expires Wed, 17 Jul 2024 18:12:12 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 77c2b86e287749579b7ff9cadb81e099042ef677}

install/helm/gloo/templates/7-gateway-proxy-deployment.yaml

.github/workflows/pr-kubernetes-tests.yaml

changelog/v1.18.0-beta5/add-istio-trust-domain.yaml

install/helm/gloo/generate/values.go

test/kubernetes/e2e/test.go

projects/gateway2/api/v1alpha1/gateway_parameters.proto

projects/gateway2/deployer/merge.go

projects/gloo/cli/pkg/cmd/istio/util.go

test/kubernetes/e2e/tests/manifests/istio-custom-trust-domain-helm.yaml

test/kubernetes/e2e/tests/revision_istio_edge_gw_test.go

test/kubernetes/e2e/tests/revision_istio_k8s_gw_test.go

test/kubernetes/testutils/cluster/istio.go

jenshu · 2024-07-11T14:37:44Z

test/kubernetes/e2e/test.go

@@ -141,6 +141,10 @@ func (i *TestInstallation) InstallMinimalIstio(ctx context.Context) error {
 	return cluster.InstallMinimalIstio(ctx, i.IstioctlBinary, i.ClusterContext.KubeContext)
 }

+func (i *TestInstallation) InstallRevisionedIstio(ctx context.Context) error {
+	return cluster.InstallRevisionedIstio(ctx, i.IstioctlBinary, i.ClusterContext.KubeContext, "1-22-1", "minimal")


what is the meaning of 1-22-1? is this an arbitrary string that a user can set?

Yep- actually lemme make it configurable at the InstallRevisionedIstio() level!

jenshu · 2024-07-11T14:39:14Z

changelog/v1.18.0-beta7/add-istio-trust-domain.yaml

@@ -0,0 +1,7 @@
+changelog:


super-nit: the name of this file should be remove-istio-trust-domain.yaml

Ah!! Not sure how to change this and make the bot happy- I think it's fine since it's not user facing? 😅

jenshu · 2024-07-11T14:47:35Z

test/kubernetes/e2e/tests/automtls_istio_test.go

@@ -29,7 +29,8 @@ func TestK8sGatewayIstioAutoMtls(t *testing.T) {
 	err := testInstallation.AddIstioctl(ctx)
 	if err != nil {
 		log.Printf("failed to install: %v\n", err)
-		t.Fail()
+		// immediately stop if Istio installation fails
+		t.Error()


according to the docs Error is equivalent to Log followed by Fail.
can the above log message be moved into the t.Error/Errorf?
(same question for all the other instances of t.Error)

I think we can combine! This was because of the change from Fail() based on Nathan's suggestion

jenshu · 2024-07-11T14:48:29Z

test/kubernetes/e2e/tests/manifests/istio-revision-k8s-gateway.yaml

@@ -0,0 +1,51 @@
+global:


nit: add -helm suffix to filename

jenshu · 2024-07-11T14:50:03Z

test/kubernetes/e2e/tests/revision_istio_edge_gw_test.go

+)
+
+// TestRevisionIstioRegression is the function which executes a series of tests against a given installation where
+// the k8s Gateway controller is disabled and the deprecated Istio integration values are used to check for regressions


do we have somewhere tracking the removal of these tests once we stop supporting the deprecated values?

The removal of which tests? The old Istio values? Those should probably be deleted in 1.18 once the values are removed from the helm chart completely.

These are net new revision tests that will be kept around for a while- I think this comment should be updated though?

jenshu · 2024-07-11T14:53:30Z

test/kubernetes/e2e/tests/revision_istio_edge_gw_tests.go

+func RevisionIstioEdgeGatewaySuiteRunner() e2e.SuiteRunner {
+	revisionIstioSuiteRunner := e2e.NewSuiteRunner(false)
+
+	revisionIstioSuiteRunner.Register("IstioIntegration", istio.NewGlooTestingSuite)


i noticed that many other suite runners have registered the name "IstioIntegration" as well - just to confirm, is that ok as long as the name is unique within the suite runner?

Yep, it's running the same feature tests, but with a different installation. So as long as the parent is differently named it should be fine. (And probably easier to tell which tests are which)

test/kubernetes/testutils/cluster/istio.go

* add trust domain * changelog * tests * fix test name * add to workflow * Adding changelog file to new location * Deleting changelog file from old location * rebalance tests, fix helm * update workflow with numbers for loadbalancing between e2e test clusters * pr feedback * update gateway proxies to be disabled for k8s gateway e2e tests * cleanup istio install, remove old trust domain tests * fix merge * minimal ci change * fix test name * fix glooctl test helm chart * fix istio edge gw manifest * fix configmap template * Adding changelog file to new location * Deleting changelog file from old location * missing gatewayProxy * t.Error * Adding changelog file to new location * Deleting changelog file from old location --------- Co-authored-by: changelog-bot <changelog-bot> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Remove Istio trust domain (#9713) * add trust domain * changelog * tests * fix test name * add to workflow * Adding changelog file to new location * Deleting changelog file from old location * rebalance tests, fix helm * update workflow with numbers for loadbalancing between e2e test clusters * pr feedback * update gateway proxies to be disabled for k8s gateway e2e tests * cleanup istio install, remove old trust domain tests * fix merge * minimal ci change * fix test name * fix glooctl test helm chart * fix istio edge gw manifest * fix configmap template * Adding changelog file to new location * Deleting changelog file from old location * missing gatewayProxy * t.Error * Adding changelog file to new location * Deleting changelog file from old location --------- Co-authored-by: changelog-bot <changelog-bot> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * move changelog --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Jacob Bohanon <jacob.bohanon@solo.io>

add trust domain

8611d0f

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Jul 1, 2024

changelog

83312da

npolshakova requested review from lgadban and sam-heilbron July 1, 2024 14:10

tests

bd0f413

npolshakova marked this pull request as ready for review July 1, 2024 14:50

npolshakova requested a review from a team as a code owner July 1, 2024 14:50

npolshakova added 2 commits July 1, 2024 11:00

fix test name

434a967

add to workflow

e97da74

changelog-bot and others added 3 commits July 1, 2024 15:52

Adding changelog file to new location

0301dac

Deleting changelog file from old location

db3f43f

rebalance tests, fix helm

d1f07e2

npolshakova requested a review from jbohanon July 1, 2024 17:06

update workflow with numbers for loadbalancing between e2e test clusters

0a249bb

jenshu reviewed Jul 1, 2024

View reviewed changes

install/helm/gloo/templates/7-gateway-proxy-deployment.yaml Outdated Show resolved Hide resolved

sam-heilbron reviewed Jul 1, 2024

View reviewed changes

Merge refs/heads/main into npolshak/add-trust-domain

ee1c9ac

npolshakova changed the title ~~Add Istio trust domain~~ Remove Istio trust domain Jul 3, 2024

npolshakova marked this pull request as draft July 3, 2024 13:20

npolshakova added 3 commits July 3, 2024 15:49

pr feedback

d7420f3

update gateway proxies to be disabled for k8s gateway e2e tests

e3a1c1e

cleanup istio install, remove old trust domain tests

6ac4e4c

npolshakova marked this pull request as ready for review July 3, 2024 20:37

Merge branch 'main' into npolshak/add-trust-domain

0797b82

npolshakova requested review from jenshu and sam-heilbron July 3, 2024 20:37

Merge refs/heads/main into npolshak/add-trust-domain

dae8f6d

npolshakova and others added 8 commits July 5, 2024 10:27

minimal ci change

5849f10

fix test name

35c9c56

fix glooctl test helm chart

38e49df

fix istio edge gw manifest

2dc73c1

fix configmap template

937e616

Adding changelog file to new location

7c12888

Deleting changelog file from old location

4208f60

missing gatewayProxy

394f72c

nfuden previously approved these changes Jul 8, 2024

View reviewed changes

test/kubernetes/e2e/tests/revision_istio_edge_gw_test.go Outdated Show resolved Hide resolved

test/kubernetes/e2e/tests/revision_istio_k8s_gw_test.go Show resolved Hide resolved

t.Error

0737893

npolshakova dismissed nfuden’s stale review via 0737893 July 8, 2024 19:15

soloio-bulldozer bot and others added 4 commits July 9, 2024 15:10

Merge refs/heads/main into npolshak/add-trust-domain

607018f

Merge refs/heads/main into npolshak/add-trust-domain

ec8c57a

Adding changelog file to new location

600fc8f

Deleting changelog file from old location

6038c0f

sam-heilbron approved these changes Jul 9, 2024

View reviewed changes

Merge refs/heads/main into npolshak/add-trust-domain

bf44f0b

npolshakova enabled auto-merge (squash) July 10, 2024 17:57

Merge branch 'main' into npolshak/add-trust-domain

0ff8863

npolshakova requested review from sam-heilbron and nfuden July 10, 2024 17:57

jbohanon reviewed Jul 11, 2024

View reviewed changes

test/kubernetes/testutils/cluster/istio.go Show resolved Hide resolved

jbohanon reviewed Jul 11, 2024

View reviewed changes

test/kubernetes/testutils/cluster/istio.go Show resolved Hide resolved

jbohanon approved these changes Jul 11, 2024

View reviewed changes

npolshakova merged commit d67089f into main Jul 11, 2024
20 checks passed

npolshakova deleted the npolshak/add-trust-domain branch July 11, 2024 14:41

jenshu reviewed Jul 11, 2024

View reviewed changes

npolshakova mentioned this pull request Jul 11, 2024

Backport 1.17: Remove Istio trust domain #9749

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Istio trust domain #9713

Remove Istio trust domain #9713

npolshakova commented Jul 1, 2024 •

edited

Loading

solo-changelog-bot bot commented Jul 1, 2024

github-actions bot commented Jul 1, 2024 •

edited

Loading

jenshu Jul 11, 2024

npolshakova Jul 11, 2024

jenshu Jul 11, 2024

npolshakova Jul 11, 2024

jenshu Jul 11, 2024

npolshakova Jul 11, 2024

jenshu Jul 11, 2024

jenshu Jul 11, 2024

npolshakova Jul 11, 2024

jenshu Jul 11, 2024

npolshakova Jul 11, 2024

Remove Istio trust domain #9713

Remove Istio trust domain #9713

Conversation

npolshakova commented Jul 1, 2024 • edited Loading

Description

API changes

Code changes

CI changes

Docs changes

Context

Interesting decisions

Testing steps

Notes for reviewers

Checklist:

solo-changelog-bot bot commented Jul 1, 2024

github-actions bot commented Jul 1, 2024 • 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

npolshakova commented Jul 1, 2024 •

edited

Loading

github-actions bot commented Jul 1, 2024 •

edited

Loading