From 2e2ae029c331a8c941b11c8aa79cc640fd29f62d Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 27 Feb 2024 11:37:16 -0800 Subject: [PATCH] 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 | 2 +- test/instrumentation/main.go | 77 +++++++++---------- test/instrumentation/main_test.go | 118 +----------------------------- 3 files changed, 44 insertions(+), 153 deletions(-) diff --git a/test/instrumentation/error.go b/test/instrumentation/error.go index 0443f4bfefb..a9960713310 100644 --- a/test/instrumentation/error.go +++ b/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" diff --git a/test/instrumentation/main.go b/test/instrumentation/main.go index 3cd12bb1a93..1df7b3a7632 100644 --- a/test/instrumentation/main.go +++ b/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 = os.Getenv("GOOS") - KUBE_ROOT string = os.Getenv("KUBE_ROOT") + GOOS string = findGOOS() 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) { - // parse directory path - var pathPrefix string - if strings.Contains(importExpr, kubeURLRoot) { - // search k/k local checkout - pathPrefix = KUBE_ROOT - importExpr = strings.Replace(importExpr, kubeURLRoot, "", 1) - } else if strings.Contains(importExpr, "k8s.io/klog/v2") || strings.Contains(importExpr, "k8s.io/util") { - pathPrefix = strings.Join([]string{KUBE_ROOT, "vendor"}, string(os.PathSeparator)) - } else if strings.Contains(importExpr, "k8s.io/") { - // search k/k/staging local checkout - pathPrefix = strings.Join([]string{KUBE_ROOT, "staging", "src"}, string(os.PathSeparator)) - } else if strings.Contains(importExpr, ".") { - // not stdlib -> prefix with GOMODCACHE - // pathPrefix = strings.Join([]string{KUBE_ROOT, "vendor"}, string(os.PathSeparator)) +func findPkgDir(pkg string) (string, error) { + // Use Go's module mechanism. + cmd := exec.Command("go", "list", "-find", "-json=Dir", pkg) + out, err := cmd.CombinedOutput() + if err != nil { + return "", fmt.Errorf("running `go list` failed: %w\n\n%s", err, string(out)) + } + result := struct { + Dir string + }{} + if err := json.Unmarshal(out, &result); err != nil { + return "", fmt.Errorf("json unmarshal of `go list` failed: %w", err) + } + if result.Dir != "" { + return result.Dir, nil + } - // this requires implementing SIV, skip for now - 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 + return "", fmt.Errorf("empty respose from `go list`") } 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 - // fmt.Fprint(os.Stderr, err.Error() + "\n") - continue + return nil, fmt.Errorf("can't handle import '%s': %w", im.Path.Value, err) + } + 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) - continue + return nil, fmt.Errorf("failed to read import directory %s: %w", importDirectory, err) } for _, file := range files { diff --git a/test/instrumentation/main_test.go b/test/instrumentation/main_test.go index 2b1f12c7eeb..be1e2b8179d 100644 --- a/test/instrumentation/main_test.go +++ b/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) - } - }) - } -}