Enforce restrictions on dependency relationships between packages

Makefile

@@ -39,8 +39,8 @@ GAS ?= $(GOPATH)/bin/gas$(BIN_ARCH)
 MISSPELL ?= $(GOPATH)/bin/misspell$(BIN_ARCH)
 
 .PHONY: all tools clean test check distro \
-	goversion goimports gopath govet gofmt misspell gas golint \
-	isos tethers apiservers copyright
+	goversion goimports gopath govet gofmt misspell gas golint copyright enforce-deps \
+	isos tethers apiservers
 
 .DEFAULT_GOAL := all
 
@@ -154,7 +154,7 @@ misspell: $(MISSPELL)
 # convenience targets
 all: components tethers isos vic-machine imagec
 tools: $(GOIMPORTS) $(GVT) $(GOLINT) $(SWAGGER) $(GAS) $(MISSPELL) goversion
-check: goversion goimports gofmt misspell govet golint copyright whitespace gas
+check: goversion goimports gofmt misspell govet golint copyright whitespace gas enforce-deps
 apiservers: $(portlayerapi) $(docker-engine-api) $(serviceapi)
 components: check apiservers $(vicadmin) $(rpctool)
 isos: $(appliance) $(bootstrap)
@@ -240,6 +240,11 @@ govet:
 gas: $(GAS)
 	@echo checking security problems
 
+# requires swagger generation to avoid warnings about unused rules
+enforce-deps: $(portlayerapi-client) $(portlayerapi-server) $(serviceapi-server)
+	@echo checking dependencies...
+	@infra/scripts/go-deps-enforcement.sh
+
 vendor: $(GVT)
 	@echo restoring vendor
 	$(GVT) restore
@@ -478,20 +483,20 @@ clean: cleandeps
 	@rm -f ./lib/apiservers/portlayer/restapi/doc.go
 	@rm -f ./lib/apiservers/portlayer/restapi/embedded_spec.go
 	@rm -f ./lib/apiservers/portlayer/restapi/server.go
-	@rm -rf ./lib/apiservers/portlayer/client/
-	@rm -rf ./lib/apiservers/portlayer/cmd/
-	@rm -rf ./lib/apiservers/portlayer/models/
-	@rm -rf ./lib/apiservers/portlayer/restapi/operations/
+	@rm -rf ./lib/apiservers/portlayer/client/*
+	@rm -rf ./lib/apiservers/portlayer/cmd/*
+	@rm -rf ./lib/apiservers/portlayer/models/*
+	@rm -rf ./lib/apiservers/portlayer/restapi/operations/*
 	@rm -rf ./lib/config/dynamic/admiral/client
 	@rm -rf ./lib/config/dynamic/admiral/models
 	@rm -rf ./lib/config/dynamic/admiral/operations
 
 	@rm -f ./lib/apiservers/service/restapi/doc.go
 	@rm -f ./lib/apiservers/service/restapi/embedded_spec.go
 	@rm -f ./lib/apiservers/service/restapi/server.go
-	@rm -rf ./lib/apiservers/service/restapi/cmd/
-	@rm -rf ./lib/apiservers/service/restapi/models/
-	@rm -rf ./lib/apiservers/service/restapi/operations/
+	@rm -rf ./lib/apiservers/service/restapi/cmd/*
+	@rm -rf ./lib/apiservers/service/restapi/models/*
+	@rm -rf ./lib/apiservers/service/restapi/operations/*
 
 	@rm -f *.log
 	@rm -f *.pem

cmd/.godeps_rules

@@ -0,0 +1,4 @@
+# By default, packages in cmd should depend only on packages under lib, pkg or vendor
+^lib/*
+^pkg/*
+^vendor/*

cmd/vic-machine-server/.godeps_rules

@@ -0,0 +1,8 @@
+# By default, packages in cmd should depend only on packages under lib, pkg or vendor
+^lib/*
+^pkg/*
+^vendor/*
+
+# TODO[#8296]: vic-machine-server transitively depends on vic-machine code, but should not
+^cmd/vic-machine/common/*
+^cmd/vic-machine/create/*

cmd/vic-machine/.godeps_rules

@@ -0,0 +1,7 @@
+# By default, packages in cmd should depend only on packages under lib, pkg or vendor
+^lib/*
+^pkg/*
+^vendor/*
+
+# vic-machine packages can also depend on common vic-machine code
+^cmd/vic-machine/common/*

cmd/vic-machine/inspect/.godeps_rules

@@ -0,0 +1,11 @@
+# By default, packages in cmd should depend only on packages under lib, pkg or vendor
+^lib/*
+^pkg/*
+^vendor/*
+
+# vic-machine packages can also depend on common vic-machine code
+^cmd/vic-machine/common/*
+
+# TODO[#8297]: vic-machine inspect currently depends on sibling commands, but should not
+^cmd/vic-machine/converter/*
+^cmd/vic-machine/create/*

cmd/vicadmin/.godeps_rules

@@ -0,0 +1,7 @@
+# By default, packages in cmd should depend only on packages under lib, pkg or vendor
+^lib/*
+^pkg/*
+^vendor/*
+
+# TODO[#8296]: vicadmin currently depends on vic-machine code, but should not
+^cmd/vic-machine/common/*

infra/scripts/go-deps-enforcement.sh

@@ -0,0 +1,173 @@
+#!/bin/bash
+# Copyright 2018 VMware, Inc. All Rights Reserved.
+#
+# 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.
+set -e -o pipefail +h && [ -n "$DEBUG" ] && set -x
+
+IFS=$'\n'
+
+ENFORCE=('cmd/' 'lib/' 'pkg/')
+RULES_FILE=.godeps_rules
+
+# Returns a list of all packages under the supplied list of directories
+# Arguments
+# *: directories to search under
+#
+# Returns:
+# go packages under those directories
+find-packages () {
+    find "${@}" -type f -name '*.go' -exec dirname {} \; | sort --unique
+}
+
+# Returns a list of all rules files
+# Arguments
+# *: directories to search under
+#
+# Returns:
+# rule files within searched directories
+find-rules () {
+    find "${@}" -type f -name "$RULES_FILE" | sort --unique
+}
+
+
+# Returns the path to the "nearest" rule file for a given package
+# Arguments
+# 1: package path
+#
+# Returns:
+# path to rule file (defaulting to /dev/null)
+find-rule () {
+    local path="${1?Package path must be provided}"
+    shift
+
+    while [[ "$path" != "." ]]; do
+        if [ -e "$path/$RULES_FILE" ]; then
+            echo "$path/$RULES_FILE"
+            return
+        fi
+
+        path="$(dirname "$path")"
+    done
+
+    echo /dev/null
+}
+
+# Returns the rules from the specified rules file, omitting comments
+# Arguments
+# 1: path to rules file
+#
+# Returns:
+# contents of the file, excluding blank lines and lines beginning with "#"
+get-rules () {
+    local rules="${1?Rules file must be provided}"
+    shift
+
+    grep -v -e '^$' -e '^#' "$rules"
+}
+
+# Returns all dependencies for a given package
+# Arguments
+# 1: package path
+#
+# Returns:
+# a list of direct and transitive dependencies
+get-deps () {
+    local package="${1?Package must be provided}"
+    shift
+
+    infra/scripts/go-deps.sh "$package"
+}
+
+# Returns any invalid dependencies for a given package by filtering the full set
+# of dependencies based on the supplied rules
+# Arguments
+# 1: package path
+# 2: path to rules file
+#
+# Returns:
+# a list of invalid dependencies
+filter-deps () {
+    local package="${1?Package must be provided}"
+    local rules="${2?Rules file must be provided}"
+    shift 2
+
+    get-deps "$package" | grep -v -e "^$package/*" -f <(get-rules "$rules") || true
+}
+
+# Returns any unused rules for a given set of packages (does not identify duplicate rules!)
+# Arguments
+# 1: the dependencies
+# 2: path to rules file
+#
+# Returns:
+# a list of rules which match none of the dependencies
+filter-rules () {
+    local deps="${1?Dependencies must be provided}"
+    local rules="${2?Rules file must be provided}"
+    shift 2
+
+    local unmatched=()
+    for rule in $(get-rules "$rules"); do
+        matches=$(echo "$deps" | grep -c -e "$rule" || true)
+
+        if [ "$matches" = "0" ]; then
+            unmatched+=("$rule")
+        fi
+    done
+
+    echo "${unmatched[*]}"
+}
+
+
+rc=0
+
+for package in $(find-packages "${ENFORCE[@]}"); do
+    rules="$(find-rule "$package")"
+
+    invalid=$(filter-deps "$package" "$rules")
+
+    if [ ! -z "$invalid" ]; then
+        echo "Unexpected dependencies in $package:"
+        echo "$invalid" | sed -e 's/^/  /'
+        echo "See $rules for details."
+        echo ""
+
+        rc=$(( rc | 1 ))
+    fi
+done
+
+export VIC_CACHE_DEPS=true # We've just calculated all of these; no need to do so again
+
+for rules in $(find-rules "${ENFORCE[@]}"); do
+    directory=$(dirname "$rules")
+
+    deps=""
+    for package in $(find-packages "$directory"); do
+        if [ "$rules" != "$(find-rule "$package")" ]; then
+            continue # This package is using a more specific rules file
+        fi
+        deps+="$(get-deps "$package")"
+    done
+
+    unused="$(filter-rules "$deps" "$rules")"
+
+    if [ ! -z "$unused" ]; then
+        echo "Unused rules in $rules:"
+        echo "$unused" | sed -e 's/^/  /'
+        echo ""
+
+        rc=$(( rc | 2 ))
+    fi
+done
+
+exit $rc

infra/scripts/go-deps.sh

@@ -23,12 +23,13 @@
 # If VIC_CACHE_DEPS environment variable is defined, this script will attempt to read
 # cached dependencies from disk if those exist. If they are not cached, dependencies will be
 # regenerated and cached.
+set -e -o pipefail +h && [ -n "$DEBUG" ] && set -x
 
 cache_dir=.godeps_cache
 
-pkg=$1
-flags=$2
-cachedname=`echo .$1.godeps_cache | sed 's/\//_/g'`
+pkg="$1"
+flags="$2"
+cachedname=$(echo ".$1.godeps_cache" | sed 's/\//_/g')
 
 if [ -d "$pkg" ]; then
 
@@ -37,9 +38,9 @@ if [ -d "$pkg" ]; then
         echo "Generating deps for $pkg" >&2
     fi
 
-    mkdir -p $cache_dir
+    mkdir -p "$cache_dir"
     # generate the cache if not present or if not using the cached result
-    if [ -z "$VIC_CACHE_DEPS" -o ! -f $cache_dir/$cachedname ]; then
+    if [ -z "$VIC_CACHE_DEPS" ] || [ ! -f "$cache_dir/$cachedname" ]; then
             go list -f '{{join .Deps "\n"}}' github.com/vmware/vic/"$pkg" 2>/dev/null | \
                 xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' 2>/dev/null | \
                 sed -e 's:github.com/vmware/vic/\(.*\)$:\1/*:' > "$cache_dir/$cachedname"

lib/.godeps_rules

@@ -0,0 +1,3 @@
+# By default, packages in lib should depend only on packages under pkg or vendor
+^pkg/*
+^vendor/*

lib/apiservers/engine/.godeps_rules

@@ -0,0 +1,31 @@
+# By default, packages in lib should depend only on packages under pkg or vendor
+^pkg/*
+^vendor/*
+
+# lib/apiservers/engine subpackages can depend on other subpackages
+^lib/apiservers/engine/*
+
+# lib/apiservers/engine acts as a client of the portlayer API
+^lib/apiservers/portlayer/client/*
+^lib/apiservers/portlayer/models/*
+
+# lib/apiservers/engine also depends on portlayer utilities and events
+^lib/portlayer/util/*
+^lib/portlayer/event/events/*
+
+# lib/apiservers/engine also depends on other lib packages
+^lib/archive/*
+^lib/config/*
+^lib/constants/*
+^lib/imagec/*
+#^lib/iolog/*
+^lib/metadata/*
+^lib/migration/*
+^lib/spec/*
+#^lib/tether/shared/*
+
+# lib/tether/shared also depends on lib/system
+#^lib/system/*
+
+# lib/system also depends on lib/etcconf
+#^lib/etcconf/*

lib/apiservers/engine/backends/middleware/.godeps_rules

@@ -0,0 +1,40 @@
+# By default, packages in lib should depend only on packages under pkg or vendor
+^pkg/*
+^vendor/*
+
+# lib/apiservers/engine subpackages can depend on other subpackages
+#^lib/apiservers/engine/*
+
+# lib/apiservers/engine acts as a client of the portlayer API
+#^lib/apiservers/portlayer/client/*
+#^lib/apiservers/portlayer/models/*
+
+# lib/apiservers/engine also depends on portlayer utilities and events
+^lib/portlayer/util/*
+^lib/portlayer/event/events/*
+
+# lib/apiservers/engine also depends on other lib packages
+#^lib/archive/*
+^lib/config/*
+^lib/constants/*
+#^lib/imagec/*
+^lib/iolog/*
+#^lib/metadata/*
+^lib/migration/*
+^lib/spec/*
+^lib/tether/shared/*
+
+# lib/tether/shared also depends on lib/system
+^lib/system/*
+
+# lib/system also depends on lib/etcconf
+^lib/etcconf/*
+
+# TODO[#8302]: lib/apiservers/engine/backends/middleware depends on lib/dns,
+# which in turn depends on a variety of code from the portlayer
+^lib/dns/*
+^lib/portlayer/event/*
+^lib/portlayer/exec/*
+^lib/portlayer/network/*
+^lib/portlayer/store/*
+^lib/guest/*

lib/apiservers/portlayer/client/.godeps_rules

@@ -0,0 +1,6 @@
+# By default, packages in lib should depend only on packages under pkg or vendor
+#^pkg/*
+^vendor/*
+
+# portlayer API client may reference portlayer API model objects
+^lib/apiservers/portlayer/models/*