Fix issue with variables not substituted in uri-referenced manifests (

#5451) (#5711)

* Add integration test highlighting the issue

The devfile used is the same as devfile-deploy.yaml,
except that the the Kubernetes component is referenced via the URI.
This is why the same integration test logic is being reused here.

* Add higher-level function in 'libdevfile' allowing to load component resource manifest and substitute variables

This works with both Inlined or Uri resources.

Notes:
- Ideally, it would make more sense to rely on the same logic used in Devfile to
  substitute variables (defined in the 'variables' package),
  but this function is not exported;
  and the exported ones substitute variables only in the URI name,
  not in the content itself (it is not fetched),
  which is actually the issue we are trying to solve here.

* Switch to using 'uri' Kubernetes components in test Devfiles

This seems to be a much more realistic case when referencing external
Kubernetes manifests.

Notes:
Some tests, like for `odo deploy`, still test 'Inlined' manifests.

* Pass the component name to 'GetComponentResourceManifestContentWithVariablesResolved'

As suggested in review, this makes more sense
now that we are passing the entire devfile object

* Rename 'GetComponentResourceManifestContentWithVariablesResolved' with 'GetK8sManifestWithVariablesSubstituted'

Previous name was a bit long ^^

* Remove extra parentheses in error string returned by 'ComponentsWithSameNameError#Error', as suggested in review

* Revert "Switch to using 'uri' Kubernetes components in test Devfiles"

This reverts commit df1f19e.

This will be done in a separate PR, with ideally
changes that should allow to use the same set of
tests for testing both Inlined and
URI-referenced manifests.

pkg/deploy/deploy.go

@@ -59,7 +59,7 @@ func (o *deployHandler) ApplyImage(img v1alpha2.Component) error {
 func (o *deployHandler) ApplyKubernetes(kubernetes v1alpha2.Component) error {
 
 	// Validate if the GVRs represented by Kubernetes inlined components are supported by the underlying cluster
-	_, err := service.ValidateResourceExist(o.kubeClient, kubernetes, o.path)
+	_, err := service.ValidateResourceExist(o.kubeClient, o.devfileObj, kubernetes, o.path)
 	if err != nil {
 		return err
 	}
@@ -75,7 +75,7 @@ func (o *deployHandler) ApplyKubernetes(kubernetes v1alpha2.Component) error {
 	odolabels.SetProjectType(annotations, component.GetComponentTypeFromDevfileMetadata(o.devfileObj.Data.GetMetadata()))
 
 	// Get the Kubernetes component
-	u, err := libdevfile.GetK8sComponentAsUnstructured(kubernetes.Kubernetes, o.path, devfilefs.DefaultFs{})
+	u, err := libdevfile.GetK8sComponentAsUnstructured(o.devfileObj, kubernetes.Name, o.path, devfilefs.DefaultFs{})
 	if err != nil {
 		return err
 	}

pkg/devfile/adapters/kubernetes/component/adapter.go

@@ -196,13 +196,13 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
 	}
 
 	// validate if the GVRs represented by Kubernetes inlined components are supported by the underlying cluster
-	err = service.ValidateResourcesExist(a.Client, k8sComponents, a.Context)
+	err = service.ValidateResourcesExist(a.Client, a.Devfile, k8sComponents, a.Context)
 	if err != nil {
 		return err
 	}
 
 	// create the Kubernetes objects from the manifest and delete the ones not in the devfile
-	err = service.PushKubernetesResources(a.Client, k8sComponents, labels, annotations, a.Context)
+	err = service.PushKubernetesResources(a.Client, a.Devfile, k8sComponents, labels, annotations, a.Context)
 	if err != nil {
 		return fmt.Errorf("failed to create service(s) associated with the component: %w", err)
 	}
@@ -246,14 +246,14 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
 
 	// Update all services with owner references
 	err = a.Client.TryWithBlockOwnerDeletion(ownerReference, func(ownerRef metav1.OwnerReference) error {
-		return service.UpdateServicesWithOwnerReferences(a.Client, k8sComponents, ownerRef, a.Context)
+		return service.UpdateServicesWithOwnerReferences(a.Client, a.Devfile, k8sComponents, ownerRef, a.Context)
 	})
 	if err != nil {
 		return err
 	}
 
 	// create the Kubernetes objects from the manifest and delete the ones not in the devfile
-	needRestart, err := service.PushLinks(a.Client, k8sComponents, labels, a.deployment, a.Context)
+	needRestart, err := service.PushLinks(a.Client, a.Devfile, k8sComponents, labels, a.deployment, a.Context)
 	if err != nil {
 		return fmt.Errorf("failed to create service(s) associated with the component: %w", err)
 	}

pkg/libdevfile/command_apply.go

@@ -37,7 +37,7 @@ func (o *applyCommand) Execute(handler Handler) error {
 	}
 
 	if len(devfileComponents) != 1 {
-		return NewComponentsWithSameNameError()
+		return NewComponentsWithSameNameError(o.command.Apply.Component)
 	}
 
 	component, err := newComponent(o.devfileObj, devfileComponents[0])

pkg/libdevfile/component_kubernetes_utils.go

@@ -6,19 +6,16 @@ import (
 	"github.com/devfile/library/pkg/devfile/parser/data/v2/common"
 	devfilefs "github.com/devfile/library/pkg/testingutil/filesystem"
 	"github.com/ghodss/yaml"
-	"github.com/redhat-developer/odo/pkg/util"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 // GetK8sComponentAsUnstructured parses the Inlined/URI K8s of the devfile K8s component
-func GetK8sComponentAsUnstructured(component *v1alpha2.KubernetesComponent, context string, fs devfilefs.Filesystem) (unstructured.Unstructured, error) {
-	strCRD := component.Inlined
-	var err error
-	if component.Uri != "" {
-		strCRD, err = util.GetDataFromURI(component.Uri, context, fs)
-		if err != nil {
-			return unstructured.Unstructured{}, err
-		}
+func GetK8sComponentAsUnstructured(devfileObj parser.DevfileObj, componentName string,
+	context string, fs devfilefs.Filesystem) (unstructured.Unstructured, error) {
+
+	strCRD, err := GetK8sManifestWithVariablesSubstituted(devfileObj, componentName, context, fs)
+	if err != nil {
+		return unstructured.Unstructured{}, err
 	}
 
 	// convert the YAML definition into map[string]interface{} since it's needed to create dynamic resource
@@ -40,7 +37,7 @@ func ListKubernetesComponents(devfileObj parser.DevfileObj, path string) (list [
 	var u unstructured.Unstructured
 	for _, kComponent := range components {
 		if kComponent.Kubernetes != nil {
-			u, err = GetK8sComponentAsUnstructured(kComponent.Kubernetes, path, devfilefs.DefaultFs{})
+			u, err = GetK8sComponentAsUnstructured(devfileObj, kComponent.Name, path, devfilefs.DefaultFs{})
 			if err != nil {
 				return
 			}

pkg/libdevfile/errors.go

@@ -64,14 +64,18 @@ func (e ComponentNotExistError) Error() string {
 	return fmt.Sprintf("component %q does not exists", e.name)
 }
 
-type ComponentsWithSameNameError struct{}
+type ComponentsWithSameNameError struct {
+	name string
+}
 
-func NewComponentsWithSameNameError() ComponentsWithSameNameError {
-	return ComponentsWithSameNameError{}
+func NewComponentsWithSameNameError(name string) ComponentsWithSameNameError {
+	return ComponentsWithSameNameError{
+		name: name,
+	}
 }
 
 func (e ComponentsWithSameNameError) Error() string {
-	return "more than one component with the same name, should not happen"
+	return fmt.Sprintf("more than one component with the same name %q, should not happen", e.name)
 }
 
 // ComponentTypeNotFoundError is returned when no component with the specified type has been found in Devfile

pkg/libdevfile/libdevfile.go

@@ -2,11 +2,17 @@ package libdevfile
 
 import (
 	"fmt"
+	"regexp"
+	"strings"
 
 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
+	"github.com/devfile/api/v2/pkg/validation/variables"
 	"github.com/devfile/library/pkg/devfile/parser"
 	"github.com/devfile/library/pkg/devfile/parser/data"
 	"github.com/devfile/library/pkg/devfile/parser/data/v2/common"
+	devfilefs "github.com/devfile/library/pkg/testingutil/filesystem"
+
+	"github.com/redhat-developer/odo/pkg/util"
 )
 
 type Handler interface {
@@ -193,3 +199,87 @@ func GetEndpointsFromDevfile(devfileObj parser.DevfileObj, ignoreExposures []v1a
 	}
 	return endpoints, nil
 }
+
+// GetK8sManifestWithVariablesSubstituted returns the full content of either a Kubernetes or an Openshift
+// Devfile component, either Inlined or referenced via a URI.
+// No matter how the component is defined, it returns the content with all variables substituted
+// using the global variables map defined in `devfileObj`.
+// An error is returned if the content references an invalid variable key not defined in the Devfile object.
+func GetK8sManifestWithVariablesSubstituted(devfileObj parser.DevfileObj, devfileCmpName string,
+	context string, fs devfilefs.Filesystem) (string, error) {
+
+	components, err := devfileObj.Data.GetComponents(common.DevfileOptions{FilterByName: devfileCmpName})
+	if err != nil {
+		return "", err
+	}
+
+	if len(components) == 0 {
+		return "", NewComponentNotExistError(devfileCmpName)
+	}
+
+	if len(components) != 1 {
+		return "", NewComponentsWithSameNameError(devfileCmpName)
+	}
+
+	devfileCmp := components[0]
+	componentType, err := common.GetComponentType(devfileCmp)
+	if err != nil {
+		return "", err
+	}
+
+	var content, uri string
+	switch componentType {
+	case v1alpha2.KubernetesComponentType:
+		content = devfileCmp.Kubernetes.Inlined
+		if devfileCmp.Kubernetes.Uri != "" {
+			uri = devfileCmp.Kubernetes.Uri
+		}
+
+	case v1alpha2.OpenshiftComponentType:
+		content = devfileCmp.Openshift.Inlined
+		if devfileCmp.Openshift.Uri != "" {
+			uri = devfileCmp.Openshift.Uri
+		}
+
+	default:
+		return "", fmt.Errorf("unexpected component type %s", componentType)
+	}
+
+	if uri != "" {
+		return loadResourceManifestFromUriAndResolveVariables(devfileObj, uri, context, fs)
+	}
+	return substituteVariables(devfileObj.Data.GetDevfileWorkspaceSpec().Variables, content)
+}
+
+func loadResourceManifestFromUriAndResolveVariables(devfileObj parser.DevfileObj, uri string,
+	context string, fs devfilefs.Filesystem) (string, error) {
+	content, err := util.GetDataFromURI(uri, context, fs)
+	if err != nil {
+		return content, err
+	}
+	return substituteVariables(devfileObj.Data.GetDevfileWorkspaceSpec().Variables, content)
+}
+
+// substituteVariables validates the string for a global variable in the given `devfileObj` and replaces it.
+// An error is returned if the string references an invalid variable key not defined in the Devfile object.
+//
+//Inspired from variables.validateAndReplaceDataWithVariable, which is unfortunately not exported
+func substituteVariables(devfileVars map[string]string, val string) (string, error) {
+	// example of the regex: {{variable}} / {{ variable }}
+	matches := regexp.MustCompile(`\{\{\s*(.*?)\s*\}\}`).FindAllStringSubmatch(val, -1)
+	var invalidKeys []string
+	for _, match := range matches {
+		varValue, ok := devfileVars[match[1]]
+		if !ok {
+			invalidKeys = append(invalidKeys, match[1])
+		} else {
+			val = strings.Replace(val, match[0], varValue, -1)
+		}
+	}
+
+	if len(invalidKeys) > 0 {
+		return val, &variables.InvalidKeysError{Keys: invalidKeys}
+	}
+
+	return val, nil
+}