fix linting error with lookup function (#7969)

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
(cherry picked from commit bb47286f)

pkg/action/action.go

@@ -17,10 +17,13 @@ limitations under the License.
 package action
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"path"
+	"path/filepath"
 	"regexp"
+	"strings"
 
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -30,9 +33,13 @@ import (
 	"k8s.io/client-go/rest"
 
 	"helm.sh/helm/v3/internal/experimental/registry"
+	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
+	"helm.sh/helm/v3/pkg/engine"
 	"helm.sh/helm/v3/pkg/kube"
+	"helm.sh/helm/v3/pkg/postrender"
 	"helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/releaseutil"
 	"helm.sh/helm/v3/pkg/storage"
 	"helm.sh/helm/v3/pkg/storage/driver"
 	"helm.sh/helm/v3/pkg/time"
@@ -86,6 +93,132 @@ type Configuration struct {
 	Log func(string, ...interface{})
 }
 
+// renderResources renders the templates in a chart
+//
+// TODO: This function is badly in need of a refactor.
+func (c *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, dryRun bool) ([]*release.Hook, *bytes.Buffer, string, error) {
+	hs := []*release.Hook{}
+	b := bytes.NewBuffer(nil)
+
+	caps, err := c.getCapabilities()
+	if err != nil {
+		return hs, b, "", err
+	}
+
+	if ch.Metadata.KubeVersion != "" {
+		if !chartutil.IsCompatibleRange(ch.Metadata.KubeVersion, caps.KubeVersion.String()) {
+			return hs, b, "", errors.Errorf("chart requires kubeVersion: %s which is incompatible with Kubernetes %s", ch.Metadata.KubeVersion, caps.KubeVersion.String())
+		}
+	}
+
+	var files map[string]string
+	var err2 error
+
+	// A `helm template` or `helm install --dry-run` should not talk to the remote cluster.
+	// It will break in interesting and exotic ways because other data (e.g. discovery)
+	// is mocked. It is not up to the template author to decide when the user wants to
+	// connect to the cluster. So when the user says to dry run, respect the user's
+	// wishes and do not connect to the cluster.
+	if !dryRun && c.RESTClientGetter != nil {
+		rest, err := c.RESTClientGetter.ToRESTConfig()
+		if err != nil {
+			return hs, b, "", err
+		}
+		files, err2 = engine.RenderWithClient(ch, values, rest)
+	} else {
+		files, err2 = engine.Render(ch, values)
+	}
+
+	if err2 != nil {
+		return hs, b, "", err2
+	}
+
+	// NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource,
+	// pull it out of here into a separate file so that we can actually use the output of the rendered
+	// text file. We have to spin through this map because the file contains path information, so we
+	// look for terminating NOTES.txt. We also remove it from the files so that we don't have to skip
+	// it in the sortHooks.
+	var notesBuffer bytes.Buffer
+	for k, v := range files {
+		if strings.HasSuffix(k, notesFileSuffix) {
+			if subNotes || (k == path.Join(ch.Name(), "templates", notesFileSuffix)) {
+				// If buffer contains data, add newline before adding more
+				if notesBuffer.Len() > 0 {
+					notesBuffer.WriteString("\n")
+				}
+				notesBuffer.WriteString(v)
+			}
+			delete(files, k)
+		}
+	}
+	notes := notesBuffer.String()
+
+	// Sort hooks, manifests, and partials. Only hooks and manifests are returned,
+	// as partials are not used after renderer.Render. Empty manifests are also
+	// removed here.
+	hs, manifests, err := releaseutil.SortManifests(files, caps.APIVersions, releaseutil.InstallOrder)
+	if err != nil {
+		// By catching parse errors here, we can prevent bogus releases from going
+		// to Kubernetes.
+		//
+		// We return the files as a big blob of data to help the user debug parser
+		// errors.
+		for name, content := range files {
+			if strings.TrimSpace(content) == "" {
+				continue
+			}
+			fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content)
+		}
+		return hs, b, "", err
+	}
+
+	// Aggregate all valid manifests into one big doc.
+	fileWritten := make(map[string]bool)
+
+	if includeCrds {
+		for _, crd := range ch.CRDObjects() {
+			if outputDir == "" {
+				fmt.Fprintf(b, "---\n# Source: %s\n%s\n", crd.Name, string(crd.File.Data[:]))
+			} else {
+				err = writeToFile(outputDir, crd.Filename, string(crd.File.Data[:]), fileWritten[crd.Name])
+				if err != nil {
+					return hs, b, "", err
+				}
+				fileWritten[crd.Name] = true
+			}
+		}
+	}
+
+	for _, m := range manifests {
+		if outputDir == "" {
+			fmt.Fprintf(b, "---\n# Source: %s\n%s\n", m.Name, m.Content)
+		} else {
+			newDir := outputDir
+			if useReleaseName {
+				newDir = filepath.Join(outputDir, releaseName)
+			}
+			// NOTE: We do not have to worry about the post-renderer because
+			// output dir is only used by `helm template`. In the next major
+			// release, we should move this logic to template only as it is not
+			// used by install or upgrade
+			err = writeToFile(newDir, m.Name, m.Content, fileWritten[m.Name])
+			if err != nil {
+				return hs, b, "", err
+			}
+			fileWritten[m.Name] = true
+		}
+	}
+
+	if pr != nil {
+		b, err = pr.Run(b)
+		if err != nil {
+			return hs, b, notes, errors.Wrap(err, "error while running post render on files")
+		}
+	}
+
+	return hs, b, notes, nil
+}
+
 // RESTClientGetter gets the rest client
 type RESTClientGetter interface {
 	ToRESTConfig() (*rest.Config, error)

pkg/action/install.go

@@ -39,7 +39,6 @@ import (
 	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/downloader"
-	"helm.sh/helm/v3/pkg/engine"
 	"helm.sh/helm/v3/pkg/getter"
 	"helm.sh/helm/v3/pkg/kube"
 	kubefake "helm.sh/helm/v3/pkg/kube/fake"
@@ -232,7 +231,7 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release.
 	rel := i.createRelease(chrt, vals)
 
 	var manifestDoc *bytes.Buffer
-	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer)
+	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, i.DryRun)
 	// Even for errors, attach this if available
 	if manifestDoc != nil {
 		rel.Manifest = manifestDoc.String()
@@ -475,125 +474,6 @@ func (i *Install) replaceRelease(rel *release.Release) error {
 	return i.recordRelease(last)
 }
 
-// renderResources renders the templates in a chart
-func (c *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer) ([]*release.Hook, *bytes.Buffer, string, error) {
-	hs := []*release.Hook{}
-	b := bytes.NewBuffer(nil)
-
-	caps, err := c.getCapabilities()
-	if err != nil {
-		return hs, b, "", err
-	}
-
-	if ch.Metadata.KubeVersion != "" {
-		if !chartutil.IsCompatibleRange(ch.Metadata.KubeVersion, caps.KubeVersion.String()) {
-			return hs, b, "", errors.Errorf("chart requires kubeVersion: %s which is incompatible with Kubernetes %s", ch.Metadata.KubeVersion, caps.KubeVersion.String())
-		}
-	}
-
-	var files map[string]string
-	var err2 error
-
-	if c.RESTClientGetter != nil {
-		rest, err := c.RESTClientGetter.ToRESTConfig()
-		if err != nil {
-			return hs, b, "", err
-		}
-		files, err2 = engine.RenderWithClient(ch, values, rest)
-	} else {
-		files, err2 = engine.Render(ch, values)
-	}
-
-	if err2 != nil {
-		return hs, b, "", err2
-	}
-
-	// NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource,
-	// pull it out of here into a separate file so that we can actually use the output of the rendered
-	// text file. We have to spin through this map because the file contains path information, so we
-	// look for terminating NOTES.txt. We also remove it from the files so that we don't have to skip
-	// it in the sortHooks.
-	var notesBuffer bytes.Buffer
-	for k, v := range files {
-		if strings.HasSuffix(k, notesFileSuffix) {
-			if subNotes || (k == path.Join(ch.Name(), "templates", notesFileSuffix)) {
-				// If buffer contains data, add newline before adding more
-				if notesBuffer.Len() > 0 {
-					notesBuffer.WriteString("\n")
-				}
-				notesBuffer.WriteString(v)
-			}
-			delete(files, k)
-		}
-	}
-	notes := notesBuffer.String()
-
-	// Sort hooks, manifests, and partials. Only hooks and manifests are returned,
-	// as partials are not used after renderer.Render. Empty manifests are also
-	// removed here.
-	hs, manifests, err := releaseutil.SortManifests(files, caps.APIVersions, releaseutil.InstallOrder)
-	if err != nil {
-		// By catching parse errors here, we can prevent bogus releases from going
-		// to Kubernetes.
-		//
-		// We return the files as a big blob of data to help the user debug parser
-		// errors.
-		for name, content := range files {
-			if strings.TrimSpace(content) == "" {
-				continue
-			}
-			fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content)
-		}
-		return hs, b, "", err
-	}
-
-	// Aggregate all valid manifests into one big doc.
-	fileWritten := make(map[string]bool)
-
-	if includeCrds {
-		for _, crd := range ch.CRDObjects() {
-			if outputDir == "" {
-				fmt.Fprintf(b, "---\n# Source: %s\n%s\n", crd.Name, string(crd.File.Data[:]))
-			} else {
-				err = writeToFile(outputDir, crd.Filename, string(crd.File.Data[:]), fileWritten[crd.Name])
-				if err != nil {
-					return hs, b, "", err
-				}
-				fileWritten[crd.Name] = true
-			}
-		}
-	}
-
-	for _, m := range manifests {
-		if outputDir == "" {
-			fmt.Fprintf(b, "---\n# Source: %s\n%s\n", m.Name, m.Content)
-		} else {
-			newDir := outputDir
-			if useReleaseName {
-				newDir = filepath.Join(outputDir, releaseName)
-			}
-			// NOTE: We do not have to worry about the post-renderer because
-			// output dir is only used by `helm template`. In the next major
-			// release, we should move this logic to template only as it is not
-			// used by install or upgrade
-			err = writeToFile(newDir, m.Name, m.Content, fileWritten[m.Name])
-			if err != nil {
-				return hs, b, "", err
-			}
-			fileWritten[m.Name] = true
-		}
-	}
-
-	if pr != nil {
-		b, err = pr.Run(b)
-		if err != nil {
-			return hs, b, notes, errors.Wrap(err, "error while running post render on files")
-		}
-	}
-
-	return hs, b, notes, nil
-}
-
 // write the <data> to <output-dir>/<name>. <append> controls if the file is created or content will be appended
 func writeToFile(outputDir string, name string, data string, append bool) error {
 	outfileName := strings.Join([]string{outputDir, name}, string(filepath.Separator))

pkg/action/install_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"helm.sh/helm/v3/internal/test"
+	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
 	kubefake "helm.sh/helm/v3/pkg/kube/fake"
 	"helm.sh/helm/v3/pkg/release"
@@ -240,6 +241,27 @@ func TestInstallRelease_DryRun(t *testing.T) {
 	is.Equal(res.Info.Description, "Dry run complete")
 }
 
+// Regression test for #7955: Lookup must not connect to Kubernetes on a dry-run.
+func TestInstallRelease_DryRun_Lookup(t *testing.T) {
+	is := assert.New(t)
+	instAction := installAction(t)
+	instAction.DryRun = true
+	vals := map[string]interface{}{}
+
+	mockChart := buildChart(withSampleTemplates())
+	mockChart.Templates = append(mockChart.Templates, &chart.File{
+		Name: "templates/lookup",
+		Data: []byte(`goodbye: {{ lookup "v1" "Namespace" "" "___" }}`),
+	})
+
+	res, err := instAction.Run(mockChart, vals)
+	if err != nil {
+		t.Fatalf("Failed install: %s", err)
+	}
+
+	is.Contains(res.Manifest, "goodbye: map[]")
+}
+
 func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) {
 	is := assert.New(t)
 	instAction := installAction(t)

pkg/action/upgrade.go

@@ -201,7 +201,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		return nil, nil, err
 	}
 
-	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer)
+	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, u.DryRun)
 	if err != nil {
 		return nil, nil, err
 	}

pkg/engine/engine.go

@@ -172,7 +172,10 @@ func (e Engine) initFunMap(t *template.Template, referenceTpls map[string]render
 		}
 		return val, nil
 	}
-	if e.config != nil {
+
+	// If we are not linting and have a cluster connection, provide a Kubernetes-backed
+	// implementation.
+	if !e.LintMode && e.config != nil {
 		funcMap["lookup"] = NewLookupFunction(e.config)
 	}
 

pkg/engine/engine_test.go

@@ -70,7 +70,7 @@ func TestFuncMap(t *testing.T) {
 	}
 
 	// Test for Engine-specific template functions.
-	expect := []string{"include", "required", "tpl", "toYaml", "fromYaml", "toToml", "toJson", "fromJson"}
+	expect := []string{"include", "required", "tpl", "toYaml", "fromYaml", "toToml", "toJson", "fromJson", "lookup"}
 	for _, f := range expect {
 		if _, ok := fns[f]; !ok {
 			t.Errorf("Expected add-on function %q", f)

pkg/engine/funcs.go

@@ -62,6 +62,11 @@ func funcMap() template.FuncMap {
 		"include":  func(string, interface{}) string { return "not implemented" },
 		"tpl":      func(string, interface{}) interface{} { return "not implemented" },
 		"required": func(string, interface{}) (interface{}, error) { return "not implemented", nil },
+		// Provide a placeholder for the "lookup" function, which requires a kubernetes
+		// connection.
+		"lookup": func(string, string, string, string) (map[string]interface{}, error) {
+			return map[string]interface{}{}, nil
+		},
 	}
 
 	for k, v := range extra {

pkg/engine/funcs_test.go

@@ -94,6 +94,11 @@ func TestFuncs(t *testing.T) {
 		tpl:    `{{ fromYamlArray . }}`,
 		expect: `[error unmarshaling JSON: while decoding JSON: json: cannot unmarshal object into Go value of type []interface {}]`,
 		vars:   `hello: world`,
+	}, {
+		// This should never result in a network lookup. Regression for #7955
+		tpl:    `{{ lookup "v1" "Namespace" "" "unlikelynamespace99999999" }}`,
+		expect: `map[]`,
+		vars:   `["one", "two"]`,
 	}}
 
 	for _, tt := range tests {

pkg/engine/lookup_func.go

@@ -32,8 +32,12 @@ import (
 
 type lookupFunc = func(apiversion string, resource string, namespace string, name string) (map[string]interface{}, error)
 
-// NewLookupFunction returns a function for looking up objects in the cluster. If the resource does not exist, no error
-// is raised.
+// NewLookupFunction returns a function for looking up objects in the cluster.
+//
+// If the resource does not exist, no error is raised.
+//
+// This function is considered deprecated, and will be renamed in Helm 4. It will no
+// longer be a public function.
 func NewLookupFunction(config *rest.Config) lookupFunc {
 	return func(apiversion string, resource string, namespace string, name string) (map[string]interface{}, error) {
 		var client dynamic.ResourceInterface