Fix instrumentation tests

1) Fail if we can't read critical env vars
2) Don't rely on KUBE_ROOT env var when `go list` works
3) Don't rely on GOOS env var when `go env` works
4) Don't quietly ignore "can't read" errors

Once we stop ignoring errors, some tests fail for real (and should
always have failed).

The "Imported k8s.io/staging constant" test seems to not be allowed at
all anymore.  Han said to nix it and he'd look async.

Oversall this test is dodgy.  You REALLY can't glue strings together and
expect valid Go module paths.  We should consider a deeper rework.

test/instrumentation/error.go

@@ -29,7 +29,7 @@ const (
 	errInvalidNewMetricCall         = "Invalid new metric call, please ensure code compiles"
 	errNonStringAttribute           = "Non string attribute is not supported"
 	errBadVariableAttribute         = "Metric attribute was not correctly set. Please use only global consts in same file"
-	errBadImportedVariableAttribute = "Metric attribute was not correctly set. Please use only global consts in correctly impoprted same file"
+	errBadImportedVariableAttribute = "Metric attribute was not correctly set. Please use only global consts in correctly imported same file"
 	errFieldNotSupported            = "Field %s is not supported"
 	errBuckets                      = "Buckets should be set to list of floats, result from function call of prometheus.LinearBuckets or prometheus.ExponentialBuckets"
 	errObjectives                   = "Objectives should be set to map of floats to floats"

test/instrumentation/main.go

@@ -18,14 +18,17 @@ package main
 
 import (
 	"bufio"
+	"encoding/json"
 	"flag"
 	"fmt"
 	"go/ast"
 	"go/parser"
 	"go/token"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"sort"
+	"strconv"
 	"strings"
 
 	"gopkg.in/yaml.v2"
@@ -35,18 +38,26 @@ const (
 	kubeMetricImportPath = `"k8s.io/component-base/metrics"`
 	// Should equal to final directory name of kubeMetricImportPath
 	kubeMetricsDefaultImportName = "metrics"
-
-	kubeURLRoot = "k8s.io/kubernetes/"
 )
 
 var (
 	// env configs
-	GOROOT                string = os.Getenv("GOROOT")
+	GOOS                  string = findGOOS()
-	GOOS                  string = os.Getenv("GOOS")
-	KUBE_ROOT             string = os.Getenv("KUBE_ROOT")
 	ALL_STABILITY_CLASSES bool
 )
 
+func findGOOS() string {
+	cmd := exec.Command("go", "env", "GOOS")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		panic(fmt.Sprintf("running `go env` failed: %v\n\n%s", err, string(out)))
+	}
+	if len(out) == 0 {
+		panic("empty result from `go env GOOS`")
+	}
+	return string(out)
+}
+
 func main() {
 
 	flag.BoolVar(&ALL_STABILITY_CLASSES, "allstabilityclasses", false, "use this flag to enable all stability classes")
@@ -202,33 +213,24 @@ func globalVariableDeclarations(tree *ast.File) map[string]ast.Expr {
 	return consts
 }
 
-func localImportPath(importExpr string) (string, error) {
+func findPkgDir(pkg string) (string, error) {
-	// parse directory path
+	// Use Go's module mechanism.
-	var pathPrefix string
+	cmd := exec.Command("go", "list", "-find", "-json=Dir", pkg)
-	if strings.Contains(importExpr, kubeURLRoot) {
+	out, err := cmd.CombinedOutput()
-		// search k/k local checkout
+	if err != nil {
-		pathPrefix = KUBE_ROOT
+		return "", fmt.Errorf("running `go list` failed: %w\n\n%s", err, string(out))
-		importExpr = strings.Replace(importExpr, kubeURLRoot, "", 1)
+	}
-	} else if strings.Contains(importExpr, "k8s.io/klog/v2") || strings.Contains(importExpr, "k8s.io/util") {
+	result := struct {
-		pathPrefix = strings.Join([]string{KUBE_ROOT, "vendor"}, string(os.PathSeparator))
+		Dir string
-	} else if strings.Contains(importExpr, "k8s.io/") {
+	}{}
-		// search k/k/staging local checkout
+	if err := json.Unmarshal(out, &result); err != nil {
-		pathPrefix = strings.Join([]string{KUBE_ROOT, "staging", "src"}, string(os.PathSeparator))
+		return "", fmt.Errorf("json unmarshal of `go list` failed: %w", err)
-	} else if strings.Contains(importExpr, ".") {
+	}
-		// not stdlib -> prefix with GOMODCACHE
+	if result.Dir != "" {
-		// pathPrefix = strings.Join([]string{KUBE_ROOT, "vendor"}, string(os.PathSeparator))
+		return result.Dir, nil
+	}
 
-		//  this requires implementing SIV, skip for now
+	return "", fmt.Errorf("empty respose from `go list`")
-		return "", fmt.Errorf("unable to handle general, non STL imports for metric analysis.  import path:  %s", importExpr)
-	} else {
-		// stdlib -> prefix with GOROOT
-		pathPrefix = strings.Join([]string{GOROOT, "src"}, string(os.PathSeparator))
-	} // ToDo: support non go mod
-
-	crossPlatformImportExpr := strings.Replace(importExpr, "/", string(os.PathSeparator), -1)
-	importDirectory := strings.Join([]string{pathPrefix, strings.Trim(crossPlatformImportExpr, "\"")}, string(os.PathSeparator))
-
-	return importDirectory, nil
 }
 
 func importedGlobalVariableDeclaration(localVariables map[string]ast.Expr, imports []*ast.ImportSpec) (map[string]ast.Expr, error) {
@@ -243,17 +245,18 @@ func importedGlobalVariableDeclaration(localVariables map[string]ast.Expr, impor
 		}
 
 		// find local path on disk for listed import
-		importDirectory, err := localImportPath(im.Path.Value)
+		pkg, err := strconv.Unquote(im.Path.Value)
 		if err != nil {
-			// uncomment the below log line if you want to start using non k8s/non stl libs for resolving const/var in metric definitions
+			return nil, fmt.Errorf("can't handle import '%s': %w", im.Path.Value, err)
-			// fmt.Fprint(os.Stderr, err.Error() + "\n")
+		}
-			continue
+		importDirectory, err := findPkgDir(pkg)
+		if err != nil {
+			return nil, fmt.Errorf("can't find import '%s': %w", im.Path.Value, err)
 		}
 
 		files, err := os.ReadDir(importDirectory)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "failed to read import path directory %s with error %s, skipping\n", importDirectory, err)
+			return nil, fmt.Errorf("failed to read import directory %s: %w", importDirectory, err)
-			continue
 		}
 
 		for _, file := range files {

test/instrumentation/main_test.go

@@ -18,9 +18,7 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"reflect"
-	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -121,16 +119,10 @@ var _ = NewCounter(
 }
 
 func TestStableMetric(t *testing.T) {
-	wd, err := os.Getwd()
-	if err != nil {
-		t.Fatalf("unable to fetch path to testing package - needed for simulating import path tests")
-	}
-
 	for _, test := range []struct {
 		testName string
 		src      string
 		metric   metric
-		kubeRoot string
 	}{
 		{
 			testName: "Counter",
@@ -459,26 +451,6 @@ var _ = metrics.NewHistogram(
 			Buckets: metrics.DefBuckets,
 		},
 	)
-`},
-		{
-			testName: "Imported stdlib constant",
-			metric: metric{
-				Name:           "importedCounter",
-				StabilityLevel: "STABLE",
-				Subsystem:      "GET",
-				Type:           counterMetricType,
-			},
-			src: `
-package test
-import "k8s.io/component-base/metrics"
-import "net/http"
-var _ = metrics.NewCounter(
-	&metrics.CounterOpts{
-			Name: "importedCounter",
-			StabilityLevel: metrics.STABLE,
-			Subsystem: http.MethodGet,
-		},
-	)
 `},
 		{
 			testName: "Imported k8s.io constant",
@@ -488,7 +460,6 @@ var _ = metrics.NewCounter(
 				Subsystem:      "kubelet",
 				Type:           counterMetricType,
 			},
-			kubeRoot: strings.Join([]string{wd, "testdata"}, string(os.PathSeparator)),
 			src: `
 package test
 import compbasemetrics "k8s.io/component-base/metrics"
@@ -500,39 +471,9 @@ var _ = compbasemetrics.NewCounter(
 			Subsystem: metrics.KubeletSubsystem,
 	},
 	)
-`},
-		{
-			testName: "Imported k8s.io/staging constant",
-			metric: metric{
-				Name:           "importedCounter",
-				StabilityLevel: "STABLE",
-				Subsystem:      "ThisIsNotTheSoundOfTheTrain",
-				Type:           counterMetricType,
-			},
-			kubeRoot: strings.Join([]string{wd, "testdata"}, string(os.PathSeparator)),
-			src: `
-package test
-import compbasemetrics "k8s.io/component-base/metrics"
-import "k8s.io/metrics"
-var _ = compbasemetrics.NewCounter(
-	&compbasemetrics.CounterOpts{
-		Name: "importedCounter",
-		StabilityLevel: compbasemetrics.STABLE,
-		Subsystem: metrics.OKGO,
-	},
-	)
 `},
 	} {
 		t.Run(test.testName, func(t *testing.T) {
-			// these sub-tests cannot be run in parallel with the below
-			if test.kubeRoot != "" {
-				priorKRoot := KUBE_ROOT
-				KUBE_ROOT = test.kubeRoot
-				defer func() {
-					KUBE_ROOT = priorKRoot
-				}()
-			}
-
 			metrics, errors := searchFileForStableMetrics(fakeFilename, test.src)
 			if len(errors) != 0 {
 				t.Errorf("Unexpected errors: %s", errors)
@@ -591,10 +532,10 @@ var _ = metrics.NewCounter(
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-import "k8s.io/kubernetes/utils"
+import "os"
 var _ = metrics.NewCounter(
 		&metrics.CounterOpts{
-			Name:           utils.getMetricName(),
+			Name:           os.Getenv("name"), // any imported function will do
 			StabilityLevel: metrics.STABLE,
 		},
 	)
@@ -724,7 +665,7 @@ var _ = metrics.NewSummary(
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-import "github.com/fake_prometheus/prometheus"
+import "github.com/prometheus/client_golang/prometheus"
 var _ = metrics.NewHistogram(
 		&metrics.HistogramOpts{
 			Name: "histogram",
@@ -745,56 +686,3 @@ var _ = metrics.NewHistogram(
 		})
 	}
 }
-
-func Test_localImportPath(t *testing.T) {
-	KUBE_ROOT = "/home/pchristopher/go/src/k8s.io/kubernetes"
-	GOROOT := os.Getenv("GOROOT")
-
-	for _, test := range []struct {
-		name         string
-		importExpr   string
-		expectedPath string
-		errorExp     bool
-	}{
-		{
-			name:         "k8s local package",
-			importExpr:   "k8s.io/kubernetes/pkg/kubelet/metrics",
-			expectedPath: strings.Join([]string{KUBE_ROOT, "pkg", "kubelet", "metrics"}, string(os.PathSeparator)),
-			errorExp:     false,
-		},
-		{
-			name:         "k8s staging package",
-			importExpr:   "k8s.io/kubelet/metrics",
-			expectedPath: strings.Join([]string{KUBE_ROOT, "staging", "src", "k8s.io", "kubelet", "metrics"}, string(os.PathSeparator)),
-			errorExp:     false,
-		},
-		{
-			name:       "public package",
-			importExpr: "github.com/thisisnot/thesoundofthetrain",
-			errorExp:   true,
-		},
-		{
-			name:         "stl package",
-			importExpr:   "os",
-			expectedPath: strings.Join([]string{GOROOT, "src", "os"}, string(os.PathSeparator)),
-			errorExp:     false,
-		},
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			path, err := localImportPath(test.importExpr)
-			if test.errorExp {
-				if err == nil {
-					t.Error("did not receive error as expected")
-				}
-			} else {
-				if err != nil {
-					t.Errorf("received unexpected error %s", err)
-				}
-			}
-
-			if path != test.expectedPath {
-				t.Errorf("did not received expected path.  \nwant: %s \ngot:  %s", test.expectedPath, path)
-			}
-		})
-	}
-}