From 54809de387195a06d489cf07d206e911bec0e588 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 7 Aug 2017 14:03:16 -0700 Subject: [PATCH] Rewrite staging import verifier in Go Signed-off-by: Steve Kuznetsov --- cmd/BUILD | 1 + cmd/importverifier/BUILD | 34 ++++ cmd/importverifier/importverifier.go | 268 ++++++++++++++++++++++++++ hack/staging-import-restrictions.json | 102 ++++++++++ hack/verify-staging-imports.sh | 87 ++------- 5 files changed, 417 insertions(+), 75 deletions(-) create mode 100644 cmd/importverifier/BUILD create mode 100644 cmd/importverifier/importverifier.go create mode 100644 hack/staging-import-restrictions.json diff --git a/cmd/BUILD b/cmd/BUILD index 668d665382a..a715d22ca77 100644 --- a/cmd/BUILD +++ b/cmd/BUILD @@ -24,6 +24,7 @@ filegroup( "//cmd/genyaml:all-srcs", "//cmd/gke-certificates-controller:all-srcs", "//cmd/hyperkube:all-srcs", + "//cmd/importverifier:all-srcs", "//cmd/kube-apiserver:all-srcs", "//cmd/kube-controller-manager:all-srcs", "//cmd/kube-proxy:all-srcs", diff --git a/cmd/importverifier/BUILD b/cmd/importverifier/BUILD new file mode 100644 index 00000000000..69ac54727fb --- /dev/null +++ b/cmd/importverifier/BUILD @@ -0,0 +1,34 @@ +package(default_visibility = ["//visibility:public"]) + +licenses(["notice"]) + +load( + "@io_bazel_rules_go//go:def.bzl", + "go_binary", + "go_library", +) + +go_binary( + name = "importverifier", + library = ":go_default_library", + tags = ["automanaged"], +) + +go_library( + name = "go_default_library", + srcs = ["importverifier.go"], + tags = ["automanaged"], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], +) diff --git a/cmd/importverifier/importverifier.go b/cmd/importverifier/importverifier.go new file mode 100644 index 00000000000..b3188a375e4 --- /dev/null +++ b/cmd/importverifier/importverifier.go @@ -0,0 +1,268 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "io/ioutil" + "log" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// Package is a subset of cmd/go.Package +type Package struct { + Dir string `json:",omitempty"` // directory containing package sources + ImportPath string `json:",omitempty"` // import path of package in dir + Imports []string `json:",omitempty"` // import paths used by this package + TestImports []string `json:",omitempty"` // imports from TestGoFiles + XTestImports []string `json:",omitempty"` // imports from XTestGoFiles +} + +// ImportRestriction describes a set of allowable import +// trees for a tree of source code +type ImportRestriction struct { + // BaseDir is the root of the package tree that is + // restricted by this configuration, given as a + // relative path from the root of the repository + BaseDir string `json:"baseImportPath"` + // IgnoredSubTrees are roots of sub-trees of the + // BaseDir for which we do not want to enforce + // any import restrictions whatsoever, given as + // relative paths from the root of the repository + IgnoredSubTrees []string `json:"ignoredSubTrees,omitempty"` + // AllowedImports are roots of package trees that + // are allowed to be imported from the BaseDir, + // given as paths that would be used in a Go + // import statement + AllowedImports []string `json:"allowedImports"` +} + +// ForbiddenImportsFor determines all of the forbidden +// imports for a package given the import restrictions +func (i *ImportRestriction) ForbiddenImportsFor(pkg Package) ([]string, error) { + if restricted, err := i.isRestrictedDir(pkg.Dir); err != nil { + return []string{}, err + } else if !restricted { + return []string{}, nil + } + + return i.forbiddenImportsFor(pkg), nil +} + +// isRestrictedDir determines if the source directory has +// any restrictions placed on it by this configuration. +// A path will be restricted if: +// - it falls under the base import path +// - it does not fall under any of the ignored sub-trees +func (i *ImportRestriction) isRestrictedDir(dir string) (bool, error) { + if under, err := isPathUnder(i.BaseDir, dir); err != nil { + return false, err + } else if !under { + return false, nil + } + + for _, ignored := range i.IgnoredSubTrees { + if under, err := isPathUnder(ignored, dir); err != nil { + return false, err + } else if under { + return false, nil + } + } + + return true, nil +} + +// isPathUnder determines if path is under base +func isPathUnder(base, path string) (bool, error) { + absBase, err := filepath.Abs(base) + if err != nil { + return false, err + } + absPath, err := filepath.Abs(path) + if err != nil { + return false, err + } + + relPath, err := filepath.Rel(absBase, absPath) + if err != nil { + return false, err + } + + // if path is below base, the relative path + // from base to path will not start with `../` + return !strings.HasPrefix(relPath, "."), nil +} + +// forbiddenImportsFor determines all of the forbidden +// imports for a package given the import restrictions +// and returns a deduplicated list of them +func (i *ImportRestriction) forbiddenImportsFor(pkg Package) []string { + forbiddenImportSet := map[string]struct{}{} + for _, imp := range append(pkg.Imports, append(pkg.TestImports, pkg.XTestImports...)...) { + path := extractVendorPath(imp) + if i.isForbidden(path) { + forbiddenImportSet[path] = struct{}{} + } + } + + var forbiddenImports []string + for imp := range forbiddenImportSet { + forbiddenImports = append(forbiddenImports, imp) + } + return forbiddenImports +} + +// extractVendorPath removes a vendor prefix if one exists +func extractVendorPath(path string) string { + vendorPath := "/vendor/" + if !strings.Contains(path, vendorPath) { + return path + } + + return path[strings.Index(path, vendorPath)+len(vendorPath):] +} + +// isForbidden determines if an import is forbidden, +// which is true when the import is: +// - of a package under the rootPackage +// - is not of the base import path or a sub-package of it +// - is not of an allowed path or a sub-package of one +func (i *ImportRestriction) isForbidden(imp string) bool { + importsBelowRoot := strings.HasPrefix(imp, rootPackage) + importsBelowBase := strings.HasPrefix(imp, i.BaseDir) + importsAllowed := false + for _, allowed := range i.AllowedImports { + importsAllowed = importsAllowed || strings.HasPrefix(imp, allowed) + } + + return importsBelowRoot && !importsBelowBase && !importsAllowed +} + +var rootPackage string + +func main() { + if len(os.Args) != 3 { + log.Fatalf("Usage: %s ROOT RESTRICTIONS.json", os.Args[0]) + } + + rootPackage = os.Args[1] + configFile := os.Args[2] + importRestrictions, err := loadImportRestrictions(configFile) + if err != nil { + log.Fatalf("Failed to load import restrictions: %v", err) + } + + foundForbiddenImports := false + for _, restriction := range importRestrictions { + log.Printf("Inspecting imports under %s...\n", restriction.BaseDir) + packages, err := resolvePackageTree(restriction.BaseDir) + if err != nil { + log.Fatalf("Failed to resolve package tree: %v", err) + } else if len(packages) == 0 { + log.Fatalf("Found no packages under tree %s", restriction.BaseDir) + } + + log.Printf("- validating imports for %d packages in the tree", len(packages)) + restrictionViolated := false + for _, pkg := range packages { + if forbidden, err := restriction.ForbiddenImportsFor(pkg); err != nil { + log.Fatalf("-- failed to validate imports: %v", err) + } else if len(forbidden) != 0 { + logForbiddenPackages(pkg.ImportPath, forbidden) + restrictionViolated = true + } + } + if restrictionViolated { + foundForbiddenImports = true + log.Println("- FAIL") + } else { + log.Println("- OK") + } + } + + if foundForbiddenImports { + os.Exit(1) + } +} + +func loadImportRestrictions(configFile string) ([]ImportRestriction, error) { + config, err := ioutil.ReadFile(configFile) + if err != nil { + return nil, fmt.Errorf("failed to load configuration from %s: %v", configFile, err) + } + + var importRestrictions []ImportRestriction + if err := json.Unmarshal(config, &importRestrictions); err != nil { + return nil, fmt.Errorf("failed to unmarshal from %s: %v", configFile, err) + } + + return importRestrictions, nil +} + +func resolvePackageTree(treeBase string) ([]Package, error) { + cmd := "go" + args := []string{"list", "-json", fmt.Sprintf("%s...", treeBase)} + stdout, err := exec.Command(cmd, args...).Output() + if err != nil { + var message string + if ee, ok := err.(*exec.ExitError); ok { + message = fmt.Sprintf("%v\n%v", ee, ee.Stderr) + } else { + message = fmt.Sprintf("%v", err) + } + return nil, fmt.Errorf("failed to run `%s %s`: %v", cmd, strings.Join(args, " "), message) + } + + packages, err := decodePackages(bytes.NewReader(stdout)) + if err != nil { + return nil, fmt.Errorf("failed to decode packages: %v", err) + } + + return packages, nil +} + +func decodePackages(r io.Reader) ([]Package, error) { + // `go list -json` concatenates package definitions + // instead of emitting a single valid JSON, so we + // need to stream the output to decode it into the + // data we are looking for instead of just using a + // simple JSON decoder on stdout + var packages []Package + decoder := json.NewDecoder(r) + for decoder.More() { + var pkg Package + if err := decoder.Decode(&pkg); err != nil { + return nil, fmt.Errorf("invalid package: %v", err) + } + packages = append(packages, pkg) + } + + return packages, nil +} + +func logForbiddenPackages(base string, forbidden []string) { + log.Printf("-- found forbidden imports for %s:\n", base) + for _, forbiddenPackage := range forbidden { + log.Printf("--- %s\n", forbiddenPackage) + } +} diff --git a/hack/staging-import-restrictions.json b/hack/staging-import-restrictions.json new file mode 100644 index 00000000000..3d421f24495 --- /dev/null +++ b/hack/staging-import-restrictions.json @@ -0,0 +1,102 @@ +[ + { + "baseImportPath": "./vendor/k8s.io/apimachinery/", + "allowedImports": [ + "k8s.io/apimachinery", + "k8s.io/kube-openapi" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/api/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/kube-gen/", + "ignoredSubTrees": [ + "./vendor/k8s.io/kube-gen/test" + ], + "allowedImports": [ + "k8s.io/gengo", + "k8s.io/kube-gen", + "k8s.io/kube-openapi" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/kube-gen/test/", + "allowedImports": [ + "k8s.io/apimachinery", + "k8s.io/client-go", + "k8s.io/gengo", + "k8s.io/kube-gen/test", + "k8s.io/kube-openapi" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/client-go/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery", + "k8s.io/client-go" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/apiserver/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery", + "k8s.io/apiserver", + "k8s.io/client-go", + "k8s.io/kube-openapi" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/metrics/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery", + "k8s.io/client-go", + "k8s.io/metrics" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/kube-aggregator/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery", + "k8s.io/apiserver", + "k8s.io/client-go", + "k8s.io/kube-aggregator", + "k8s.io/kube-openapi" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/sample-apiserver/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apimachinery", + "k8s.io/apiserver", + "k8s.io/client-go", + "k8s.io/sample-apiserver" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/apiextensions-apiserver/", + "allowedImports": [ + "k8s.io/api", + "k8s.io/apiextensions-apiserver", + "k8s.io/apimachinery", + "k8s.io/apiserver", + "k8s.io/client-go" + ] + }, + { + "baseImportPath": "./vendor/k8s.io/kube-openapi/", + "allowedImports": [ + "k8s.io/kube-openapi", + "k8s.io/gengo" + ] + } +] \ No newline at end of file diff --git a/hack/verify-staging-imports.sh b/hack/verify-staging-imports.sh index fd9d7666a3a..30b8247e2f4 100755 --- a/hack/verify-staging-imports.sh +++ b/hack/verify-staging-imports.sh @@ -23,82 +23,19 @@ source "${KUBE_ROOT}/hack/lib/init.sh" kube::golang::setup_env -function print_forbidden_imports () { - set -o errexit # this was unset by || - local REPO="${1%%/*}" # everything in front of the / +make -C "${KUBE_ROOT}" WHAT=cmd/importverifier - # find packages with extended glob support of bash (supports inversion) - local PACKAGES=($( - shopt -s extglob; - eval ls -d -1 ./vendor/k8s.io/${1}/ - )) +# Find binary +importverifier=$(kube::util::find-binary "importverifier") - shift - local RE="" - local SEP="" - for CLAUSE in "$@"; do - RE+="${SEP}${CLAUSE}" - SEP='\|' - done - local FORBIDDEN=$( - go list -f $'{{with $package := .ImportPath}}{{range $.Imports}}{{$package}} imports {{.}}\n{{end}}{{end}}' "${PACKAGES[@]/%/...}" | - sed 's|^k8s.io/kubernetes/vendor/||;s| k8s.io/kubernetes/vendor/| |' | - grep -v " k8s.io/${REPO}" | - grep " k8s.io/" | - grep -v -e "imports \(${RE}\)" - ) - if [ -n "${FORBIDDEN}" ]; then - echo "${REPO} has a forbidden dependency:" - echo - echo "${FORBIDDEN}" | sed 's/^/ /' - echo - return 1 - fi - local TEST_FORBIDDEN=$( - go list -f $'{{with $package := .ImportPath}}{{range $.TestImports}}{{$package}} imports {{.}}\n{{end}}{{end}}' "${PACKAGES[@]/%/...}" | - sed 's|^k8s.io/kubernetes/vendor/||;s| k8s.io/kubernetes/vendor/| |' | - grep -v " k8s.io/${REPO}" | - grep " k8s.io/" | - grep -v -e "imports \(${RE}\)" - ) - if [ -n "${TEST_FORBIDDEN}" ]; then - echo "${REPO} has a forbidden dependency in test code:" - echo - echo "${TEST_FORBIDDEN}" | sed 's/^/ /' - echo - return 1 - fi - return 0 -} - -RC=0 -print_forbidden_imports apimachinery k8s.io/kube-openapi || RC=1 -print_forbidden_imports api k8s.io/apimachinery || RC=1 -print_forbidden_imports kube-gen k8s.io/apimachinery k8s.io/client-go k8s.io/gengo k8s.io/kube-openapi || RC=1 -print_forbidden_imports 'kube-gen/!(test)' k8s.io/gengo k8s.io/kube-openapi || RC=1 -print_forbidden_imports kube-gen/test k8s.io/apimachinery k8s.io/client-go || RC=1 -print_forbidden_imports client-go k8s.io/apimachinery k8s.io/api || RC=1 -print_forbidden_imports apiserver k8s.io/apimachinery k8s.io/client-go k8s.io/api k8s.io/kube-openapi || RC=1 -print_forbidden_imports metrics k8s.io/apimachinery k8s.io/client-go k8s.io/api || RC=1 -print_forbidden_imports kube-aggregator k8s.io/apimachinery k8s.io/client-go k8s.io/apiserver k8s.io/api k8s.io/kube-openapi || RC=1 -print_forbidden_imports sample-apiserver k8s.io/apimachinery k8s.io/client-go k8s.io/apiserver k8s.io/api || RC=1 -print_forbidden_imports apiextensions-apiserver k8s.io/apimachinery k8s.io/client-go k8s.io/apiserver k8s.io/api || RC=1 -print_forbidden_imports kube-openapi k8s.io/gengo || RC=1 -if [ ${RC} != 0 ]; then - exit ${RC} +if [[ ! -x "$importverifier" ]]; then + { + echo "It looks as if you don't have a compiled importverifier binary" + echo + echo "If you are running from a clone of the git repo, please run" + echo "'make WHAT=cmd/importverifier'." + } >&2 + exit 1 fi -if grep -rq '// import "k8s.io/kubernetes/' 'staging/'; then - echo 'file has "// import "k8s.io/kubernetes/"' - exit 1 -fi - -for EXAMPLE in vendor/k8s.io/client-go/examples/{in-cluster-client-configuration,out-of-cluster-client-configuration} vendor/k8s.io/apiextensions-apiserver/examples ; do - test -d "${EXAMPLE}" # make sure example is still there - if go list -f '{{ join .Deps "\n" }}' "./${EXAMPLE}/..." | sort | uniq | grep -q k8s.io/client-go/plugin; then - echo "${EXAMPLE} imports client-go plugins by default, but shouldn't." - exit 1 - fi -done - -exit 0 +"${importverifier}" "k8s.io/" "${KUBE_ROOT}/hack/staging-import-restrictions.json" \ No newline at end of file