Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Check enablement from values part as well #727

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 39 additions & 7 deletions pkg/name/name.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,22 @@ var (
CNIComponentName,
}
allComponentNamesMap = make(map[ComponentName]bool)

// ComponentNameToHelmComponentPath defines mapping from component name to helm component root path.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be fairly easy to create this programmatically, maybe when you create a translator/reverse translator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the checkenablement part does not depend on translator so I don’t want to get this mapping from the translator, not sure whether there is better way possibly?

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably should depend on it, or actually should go into Translator. Name is a very low level pkg and shouldn't know anything about things like helm so anyway this function probably doesn't belong there now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ostromart translation is the process to convert between operator spec and helm values spec, however to get enablement values or some other paths from one of them does not reply on the other, right? I think it is ok to put them in names now as it does not need a process of translation. I add a TODO to refactor this part later but hope to get this merged soon to fix related bugs first.

// TODO: merge this with the componentMaps defined in translateConfig
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() {
Expand Down Expand Up @@ -100,7 +116,20 @@ 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) {
// 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
}
if componentName == IngressComponentName {
return len(controlPlaneSpec.Components.IngressGateways) != 0, nil
}
Expand Down Expand Up @@ -128,25 +157,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, false, 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.
Expand Down
4 changes: 2 additions & 2 deletions pkg/translate/translate_value.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down