check enablement from values pass through #20244

richardwxn · 2020-01-16T20:16:31Z

mirror of old PR: Check enablement from values part as well operator#727
fix default profile to add istio-egressgateway definition

istio-policy-bot · 2020-01-16T20:16:34Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

howardjohn · 2020-01-16T20:34:17Z

can you fix all the hacks we added to get around this if you didn't already (didn't look at the code yet)? in test/integration

…

On Thu, Jan 16, 2020, 12:22 PM Istio Automation ***@***.***> wrote: @richardwxn <https://github.com/richardwxn>: The following test *failed*, say /retest to rerun all failed tests: Test name Commit Details Rerun command gencheck_istio ffabd57 <ffabd57> link <https://prow.istio.io/view/gcs/istio-prow/pr-logs/pull/istio_istio/20244/gencheck_istio/4873> /test gencheck_istio Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#20244?email_source=notifications&email_token=AAEYGXKKV6BSXJSL34OI4STQ6C6ZVA5CNFSM4KH2ACIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJFNOSY#issuecomment-575330123>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXI3WLRIJZ3SAPJ7H7TQ6C6ZVANCNFSM4KH2ACIA> .

richardwxn · 2020-01-16T21:38:33Z

@howardjohn what hack did you refer to in test/integration

howardjohn · 2020-01-16T21:41:26Z

istio/tests/integration/telemetry/tracing/clienttracing/client_tracing_test.go

Line 89 in 7548bb2

cfg.ControlPlaneValues = `

and others that reference that issue (may just be 1 other?)

howardjohn · 2020-01-18T23:54:19Z

operator/data/profiles/default.yaml

-
+    - name: istio-egressgateway
+      enabled: false
+      k8s:


-1 to this. Why do we need to copy everything over? its already defined in the templates. Its so hard to figure out what I am going to get when I install with istioctl now because there are so many different configs

I would update this part to keep only the settings we expose as params in the template

actually only the scaleTargetRef is hardcoded in the template, however we can not drop this part from the profile because of existing issue: #20143

howardjohn · 2020-01-18T23:55:40Z

operator/pkg/name/name.go

+		IngressComponentName:         "gateways.istio-ingressgateway",
+		EgressComponentName:          "gateways.istio-egressgateway",
+		CNIComponentName:             "cni",
+	}


We need prometheus, grafana, etc as well.

Any update on this?

I'm adding those in my PR: #20352

howardjohn · 2020-01-18T23:58:37Z

operator/pkg/name/name.go

@@ -101,6 +117,18 @@ func (cn ComponentName) IsAddon() bool {
 // IsComponentEnabledInSpec reports whether the given component is enabled in the given spec.
 // IsComponentEnabledInSpec assumes that controlPlaneSpec has been validated.
 func IsComponentEnabledInSpec(componentName ComponentName, controlPlaneSpec *v1alpha1.IstioOperatorSpec) (bool, error) {
+	// for addon components, enablement is defined in values part.


This seems to try to turn off addons but something must be wrong. --set values.prometheus.enabled=false does not do anything

this PR can only address the enablement issue of istio components now, for addons, I added a TODO to address that after #20316 is resolved

richardwxn · 2020-01-23T23:15:47Z

/test integ-security-k8s-tests_istio

richardwxn · 2020-01-25T03:06:20Z

It should work now for both Istio components and addons. Please check again: @ostromart @howardjohn

howardjohn · 2020-01-25T03:38:01Z

operator/pkg/name/name.go

+		CoreDNSComponentName,
+	}
+	allComponentNamesMap         = make(map[ComponentName]bool)
+	deprecatedComponentNamesMap  = make(map[ComponentName]bool)


nit: using a struct{} instead of a bool is slightly nicer for a Set because that's is only 2 states (present or not present) instead of 3 (not present, true, false) - what does it mean to be false? It's also (trivially) more perfomant

you mean using an empty struct to replace bool? that's actually better in terms of memory(trivial) but here I just try to align with other implementation for set in this package.

richardwxn · 2020-01-25T07:28:40Z

/test integ-pilot-k8s-tests_istio

richardwxn · 2020-01-25T20:19:07Z

/test integ-security-k8s-tests_istio

richardwxn · 2020-01-25T20:19:20Z

/test pilot-multicluster-e2e_istio

richardwxn · 2020-01-25T22:57:39Z

/test integ-security-k8s-tests_istio

richardwxn · 2020-01-26T00:19:02Z

/retest

richardwxn · 2020-01-26T02:08:06Z

/retest

richardwxn · 2020-01-26T03:53:22Z

/test integ-security-k8s-tests_istio

howardjohn · 2020-01-26T03:59:08Z

a test failing 6 times in a row suggests an issue with the pr, our tests don't flake often anymore

howardjohn · 2020-01-27T01:25:11Z

@richardwxn another solution which may make more sense is to disable egress in ./install/kubernetes/helm/istio/test-values/values-integ.yaml. Then instead of skipping the test it will pass, and the follow up here is to re-enable egress just for that test once we have egress working (really we don't need to install it for every single test since we never use it).

richardwxn · 2020-01-27T01:51:20Z

@howardjohn Can we just add override in this failing test to disable egress? I am not sure whether changing that in the shared values-integ.yaml would impact other tests.

howardjohn · 2020-01-27T01:52:54Z

it will impact all tests. But currently, NO tests use egress. With your changes, All tests will use egress - which we do not want.

…

On Sun, Jan 26, 2020, 5:51 PM Xinnan Wen ***@***.***> wrote: @howardjohn <https://github.com/howardjohn> Can we just add override in this failing test to disable egress? I am not sure whether changing that in the shared values-integ.yaml would impact other tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20244?email_source=notifications&email_token=AAEYGXKG4SZV3RDZBGD3ISTQ7Y4ZTA5CNFSM4KH2ACIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ6D7MA#issuecomment-578568112>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXO2GYTKB76BOIAC7CDQ7Y4ZTANCNFSM4KH2ACIA> .

richardwxn · 2020-01-27T01:59:24Z

@howardjohn ok, if no test is using egress, then the default should be turning that off?

howardjohn · 2020-01-27T02:01:00Z

yeah I think so That is what we are doing today (as a bug, really, but it makes sense to keep it)

…

On Sun, Jan 26, 2020, 5:59 PM Xinnan Wen ***@***.***> wrote: @howardjohn <https://github.com/howardjohn> ok, if no test is using egress, then the default should be turning that off? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20244?email_source=notifications&email_token=AAEYGXIYQR7SGICDM3OARSDQ7Y5X3A5CNFSM4KH2ACIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ6EGTQ#issuecomment-578569038>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXJUDTUBBPWO5YZLGIDQ7Y5X3ANCNFSM4KH2ACIA> .

ostromart · 2020-01-27T16:17:44Z

operator/pkg/component/controlplane/control_plane.go

 	for idx, c := range installSpec.Components.IngressGateways {
+		if c.Name == istioIngressGatewayName {


This needs to be removed per our discussion.

ostromart · 2020-01-27T16:17:56Z

operator/pkg/component/controlplane/control_plane.go

@@ -57,6 +73,16 @@ func NewIstioOperator(installSpec *v1alpha1.IstioOperatorSpec, translator *trans
 		out.components = append(out.components, component.NewIngressComponent(c.Name, idx, &o))
 	}
 	for idx, c := range installSpec.Components.EgressGateways {
+		if c.Name == istioEgressGatewayName {


ostromart · 2020-01-27T16:18:26Z

operator/pkg/name/name.go

+	KialiComponentName      ComponentName = "Kiali"
+	GrafanaComponentName    ComponentName = "Grafana"
+	TracingComponentName    ComponentName = "Tracing"
+	CoreDNSComponentName    ComponentName = "Istiocoredns"


Remove these per discussion.

Will we be able to enable them in some other way?

howardjohn · 2020-01-27T21:28:48Z

/retest

cluster failed creation

istio-testing · 2020-01-27T23:55:52Z

In response to a cherrypick label: new pull request created: #20580

* check enablement from values pass through as well * check enablement from values pass through as well * fix bug * fix gen * fix tpath for merging * resolve conflict * turn off egress by default for integ test

…tput (#20651) * check enablement from values pass through (#20244) * check enablement from values pass through as well * check enablement from values pass through as well * fix bug * fix gen * fix tpath for merging * resolve conflict * turn off egress by default for integ test * move naming logics to translate (#20618) * move naming logics to translate * fix gen * update file name * rename file * add select and ignore to manifest generate test output (#20622) * add select and ignore to manifest generate output * fix the output order * fix lint * resolve conflict * fix lint * fix lint * resolve conflict * resolve conflict

richardwxn requested a review from a team as a code owner January 16, 2020 20:16

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 16, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 16, 2020

howardjohn reviewed Jan 18, 2020

View reviewed changes

This was referenced Jan 20, 2020

AddonComponent issues of new operator API #20316

Closed

Gateway component not enabled by .values #19799

Closed

richardwxn force-pushed the check-enablement-values branch from ffabd57 to 3aa0950 Compare January 23, 2020 01:25

howardjohn added this to the 1.6 milestone Jan 23, 2020

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 25, 2020

richardwxn force-pushed the check-enablement-values branch from 11ec05b to 7c6cea0 Compare January 25, 2020 01:13

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 25, 2020

howardjohn approved these changes Jan 25, 2020

View reviewed changes

richardwxn force-pushed the check-enablement-values branch from 7c6cea0 to d6228f3 Compare January 25, 2020 19:45

richardwxn added the cherrypick/release-1.5 label Jan 25, 2020

dgn approved these changes Jan 26, 2020

View reviewed changes

richardwxn mentioned this pull request Jan 26, 2020

Update manifest-migrate based on IOP change #20542

Merged

richardwxn force-pushed the check-enablement-values branch from ca3d622 to b589eae Compare January 27, 2020 06:04

ostromart reviewed Jan 27, 2020

View reviewed changes

richardwxn added 7 commits January 27, 2020 11:54

check enablement from values pass through as well

a729cc3

check enablement from values pass through as well

02bae7b

fix bug

8701e81

fix gen

fffed09

fix tpath for merging

d767b14

resolve conflict

213b6da

turn off egress by default for integ test

40cf7fa

richardwxn force-pushed the check-enablement-values branch 2 times, most recently from ca3d622 to 40cf7fa Compare January 27, 2020 20:16

howardjohn approved these changes Jan 27, 2020

View reviewed changes

istio-testing merged commit 29be605 into istio:master Jan 27, 2020

istio-testing mentioned this pull request Jan 27, 2020

[release-1.5] check enablement from values pass through #20580

Closed

clarketm mentioned this pull request Jan 28, 2020

Cherrypicker missing many (all?) cherrypicks from label istio/test-infra#2309

Open

richardwxn mentioned this pull request Jan 29, 2020

[release-1.5] operator enablement issues fix and prune golden test output #20651

Merged

		for idx, c := range installSpec.Components.IngressGateways {
		if c.Name == istioIngressGatewayName {

check enablement from values pass through #20244

check enablement from values pass through #20244

Conversation

richardwxn commented Jan 16, 2020

istio-policy-bot commented Jan 16, 2020

howardjohn commented Jan 16, 2020 via email

richardwxn commented Jan 16, 2020

howardjohn commented Jan 16, 2020

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

richardwxn commented Jan 23, 2020

richardwxn commented Jan 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardwxn commented Jan 25, 2020

richardwxn commented Jan 25, 2020

richardwxn commented Jan 25, 2020

richardwxn commented Jan 25, 2020

richardwxn commented Jan 26, 2020

richardwxn commented Jan 26, 2020

richardwxn commented Jan 26, 2020

howardjohn commented Jan 26, 2020

howardjohn commented Jan 27, 2020

richardwxn commented Jan 27, 2020

howardjohn commented Jan 27, 2020 via email

richardwxn commented Jan 27, 2020

howardjohn commented Jan 27, 2020 via email

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

howardjohn commented Jan 27, 2020

istio-testing commented Jan 27, 2020