From 9ca57b237ea0547b5ed23fdcb767a91aea8503dd Mon Sep 17 00:00:00 2001 From: Xinnan Wen Date: Mon, 30 Dec 2019 08:16:48 -0800 Subject: [PATCH 1/3] Check enablement from values part as well --- pkg/name/name.go | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/pkg/name/name.go b/pkg/name/name.go index 335a577d4..b20d97e5d 100644 --- a/pkg/name/name.go +++ b/pkg/name/name.go @@ -56,6 +56,8 @@ const ( // Operator components IstioOperatorComponentName ComponentName = "IstioOperator" IstioOperatorCustomResourceName ComponentName = "IstioOperatorCustomResource" + + ) var ( @@ -72,6 +74,21 @@ var ( CNIComponentName, } allComponentNamesMap = make(map[ComponentName]bool) + // TODO: merge this with the componentMaps defined in translateConfig + // ComponentNameToHelmComponentPath defines mapping from component name to helm component root path. + ComponentNameToHelmComponentPath = map[ComponentName]string { + PilotComponentName: "pilot", + GalleyComponentName: "galley", + SidecarInjectorComponentName: "sidecarInjectorWebhook", + PolicyComponentName: "mixer.policy", + TelemetryComponentName: "mixer.telemetry", + CitadelComponentName: "security", + CertManagerComponentName: "certmanager", + NodeAgentComponentName: "nodeagent", + IngressComponentName: "gateways.istio-ingressgateway", + EgressComponentName: "gateways.istio-egressgateway", + CNIComponentName: "cni", + } ) func init() { @@ -100,7 +117,14 @@ func (cn ComponentName) IsAddon() bool { // IsComponentEnabledInSpec reports whether the given component is enabled in the given spec. // IsComponentEnabledInSpec assumes that controlPlaneSpec has been validated. +// TODO: remove extra validations when comfort level is high enough. func IsComponentEnabledInSpec(componentName ComponentName, controlPlaneSpec *v1alpha1.IstioOperatorSpec) (bool, error) { + // if component enablement path is defined and found in Values part, return it first, otherwise check from ISCP. + valuePath := ComponentNameToHelmComponentPath[componentName] + enabled, pathExist, err := IsComponentEnabledFromValue(valuePath, controlPlaneSpec.Values) + if err == nil && pathExist { + return enabled, nil + } if componentName == IngressComponentName { return len(controlPlaneSpec.Components.IngressGateways) != 0, nil } @@ -128,25 +152,28 @@ func IsComponentEnabledInSpec(componentName ComponentName, controlPlaneSpec *v1a // IsComponentEnabledFromValue get whether component is enabled in helm value.yaml tree. // valuePath points to component path in the values tree. -func IsComponentEnabledFromValue(valuePath string, valueSpec map[string]interface{}) (bool, error) { +func IsComponentEnabledFromValue(valuePath string, valueSpec map[string]interface{}) (enabled bool, pathExist bool, err error) { enabledPath := valuePath + ".enabled" enableNodeI, found, err := tpath.GetFromTreePath(valueSpec, util.ToYAMLPath(enabledPath)) if err != nil { - return false, fmt.Errorf("error finding component enablement path: %s in helm value.yaml tree", enabledPath) + return false, false, fmt.Errorf("error finding component enablement path: %s in helm value.yaml tree", enabledPath) } if !found { // Some components do not specify enablement should be treated as enabled if the root node in the component subtree exists. _, found, err := tpath.GetFromTreePath(valueSpec, util.ToYAMLPath(valuePath)) - if found && err == nil { - return true, nil + if err != nil { + return false, false, err } - return false, nil + if found { + return true, true, nil + } + return false, false, nil } enableNode, ok := enableNodeI.(bool) if !ok { - return false, fmt.Errorf("node at valuePath %s has bad type %T, expect bool", enabledPath, enableNodeI) + return false, true, fmt.Errorf("node at valuePath %s has bad type %T, expect bool", enabledPath, enableNodeI) } - return enableNode, nil + return enableNode, true, nil } // NamespaceFromValue gets the namespace value in helm value.yaml tree. From 76a187205a675b719b55a19313cc64a1d11bde67 Mon Sep 17 00:00:00 2001 From: Xinnan Wen Date: Tue, 31 Dec 2019 05:53:21 -0800 Subject: [PATCH 2/3] fix test --- pkg/name/name.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/name/name.go b/pkg/name/name.go index b20d97e5d..5214d70fd 100644 --- a/pkg/name/name.go +++ b/pkg/name/name.go @@ -56,8 +56,6 @@ const ( // Operator components IstioOperatorComponentName ComponentName = "IstioOperator" IstioOperatorCustomResourceName ComponentName = "IstioOperatorCustomResource" - - ) var ( @@ -74,9 +72,10 @@ var ( CNIComponentName, } allComponentNamesMap = make(map[ComponentName]bool) - // TODO: merge this with the componentMaps defined in translateConfig + // ComponentNameToHelmComponentPath defines mapping from component name to helm component root path. - ComponentNameToHelmComponentPath = map[ComponentName]string { + // TODO: merge this with the componentMaps defined in translateConfig + ComponentNameToHelmComponentPath = map[ComponentName]string{ PilotComponentName: "pilot", GalleyComponentName: "galley", SidecarInjectorComponentName: "sidecarInjectorWebhook", @@ -119,9 +118,15 @@ func (cn ComponentName) IsAddon() bool { // IsComponentEnabledInSpec assumes that controlPlaneSpec has been validated. // TODO: remove extra validations when comfort level is high enough. func IsComponentEnabledInSpec(componentName ComponentName, controlPlaneSpec *v1alpha1.IstioOperatorSpec) (bool, error) { - // if component enablement path is defined and found in Values part, return it first, otherwise check from ISCP. + // for addon components, enablement is defined in values part. + if componentName == AddonComponentName { + enabled, _, err := IsComponentEnabledFromValue(string(componentName), controlPlaneSpec.Values) + return enabled, err + } + // for Istio components, check whether override path exist in values part first then ISCP. valuePath := ComponentNameToHelmComponentPath[componentName] enabled, pathExist, err := IsComponentEnabledFromValue(valuePath, controlPlaneSpec.Values) + // only return value when path exists if err == nil && pathExist { return enabled, nil } @@ -165,7 +170,7 @@ func IsComponentEnabledFromValue(valuePath string, valueSpec map[string]interfac return false, false, err } if found { - return true, true, nil + return true, false, nil } return false, false, nil } From 22db95ff04e2a532a356b83bb4dabd2fb4a1b56e Mon Sep 17 00:00:00 2001 From: Xinnan Wen Date: Sat, 11 Jan 2020 19:56:01 -0800 Subject: [PATCH 3/3] resolve conflict --- pkg/translate/translate_value.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/translate/translate_value.go b/pkg/translate/translate_value.go index 03288d55f..df18745ba 100644 --- a/pkg/translate/translate_value.go +++ b/pkg/translate/translate_value.go @@ -116,7 +116,7 @@ func (t *ReverseTranslator) initAPIAndComponentMapping(vs version.MinorVersion) func (t *ReverseTranslator) initK8SMapping(valueTree map[string]interface{}) error { outputMapping := make(map[string]*Translation) for valKey, componentName := range t.ValuesToComponentName { - cnEnabled, err := name.IsComponentEnabledFromValue(valKey, valueTree) + cnEnabled, _, err := name.IsComponentEnabledFromValue(valKey, valueTree) if err != nil { return err } @@ -220,7 +220,7 @@ func (t *ReverseTranslator) TranslateTree(valueTree map[string]interface{}, cpSp // tree, based on feature/component inheritance relationship. func (t *ReverseTranslator) setEnablementAndNamespacesFromValue(valueSpec map[string]interface{}, root map[string]interface{}) error { for cnv, cni := range t.ValuesToComponentName { - enabled, err := name.IsComponentEnabledFromValue(cnv, valueSpec) + enabled, _, err := name.IsComponentEnabledFromValue(cnv, valueSpec) if err != nil { return err }