Add infrastructure to document env var usage. (#12727)

- Introduce the pkg/env package containing a few functions to query environment variable values. It keeps track of the variables requested so they can be documented. - Extend pkg/collateral to recognize and output the environment variables used in the process. This is what is needed to make this stuff show up on istio.io. - Update all relevant call sites to use the new infrsstructure. It's still missing descriptions for all the variables, that'll be up to component authors. I'll file issues to get that work done. - Fixed bugs in the node_agent_k8s code that was using env vars as the default for Cobra command-line arguments, resulting in potentially variable default values produced in the generated docs. Default values need to be static.
istio · Mar 25, 2019 · 1664a07 · 1664a07
1 parent 035664a
commit 1664a07
Show file tree

Hide file tree

Showing 25 changed files with 819 additions and 339 deletions.
diff --git a/galley/pkg/server/configmap.go b/galley/pkg/server/configmap.go
@@ -23,8 +23,8 @@ import (
 	"github.com/fsnotify/fsnotify"
 	yaml "gopkg.in/yaml.v2"
 
+	envvar "istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/filewatcher"
-	"istio.io/istio/pkg/mcp/env"
 	"istio.io/istio/pkg/mcp/server"
 )
 
@@ -42,11 +42,11 @@ var (
 var (
 	// For the purposes of logging rate limiting authz failures, this controls how
 	// many authz failures are logs as a burst every AUTHZ_FAILURE_LOG_FREQ.
-	authzFailureLogBurstSize = env.Integer("AUTHZ_FAILURE_LOG_BURST_SIZE", 1)
+	authzFailureLogBurstSize = envvar.RegisterIntVar("AUTHZ_FAILURE_LOG_BURST_SIZE", 1, "").Get()
 
 	// For the purposes of logging rate limiting authz failures, this controls how
 	// frequently bursts of authz failures are logged.
-	authzFailureLogFreq = env.Duration("AUTHZ_FAILURE_LOG_FREQ", time.Minute)
+	authzFailureLogFreq = envvar.RegisterDurationVar("AUTHZ_FAILURE_LOG_FREQ", time.Minute, "").Get()
 )
 
 func watchAccessList(stopCh <-chan struct{}, accessListFile string) (*server.ListAuthChecker, error) {

diff --git a/istioctl/cmd/istioctl/kubeinject.go b/istioctl/cmd/istioctl/kubeinject.go
@@ -20,7 +20,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"strings"
 
 	"github.com/ghodss/yaml"
 	"github.com/spf13/cobra"
@@ -32,6 +31,7 @@ import (
 	"istio.io/istio/pilot/cmd"
 	"istio.io/istio/pilot/pkg/kube/inject"
 	"istio.io/istio/pilot/pkg/model"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/kube"
 	"istio.io/istio/pkg/log"
 	"istio.io/istio/pkg/version"
@@ -155,6 +155,8 @@ var (
 )
 
 var (
+	useBuiltinDefaultsVar = env.RegisterBoolVar("ISTIOCTL_USE_BUILTIN_DEFAULTS", false, "")
+
 	injectCmd = &cobra.Command{
 		Use:   "kube-inject",
 		Short: "Inject Envoy sidecar into Kubernetes pod resources",
@@ -265,8 +267,9 @@ istioctl kube-inject -f deployment.yaml -o deployment-injected.yaml --injectConf
 			// hub and tag params only work with ISTIOCTL_USE_BUILTIN_DEFAULTS
 			// so must be specified together. hub and tag no longer have defaults.
 			if hub != "" || tag != "" {
+
 				// ISTIOCTL_USE_BUILTIN_DEFAULTS is used to have legacy behavior.
-				if !getBoolEnv("ISTIOCTL_USE_BUILTIN_DEFAULTS", false) {
+				if !useBuiltinDefaultsVar.Get() {
 					return errors.New("one of injectConfigFile or injectConfigMapName is required\n" +
 						"use the following command to get the current injector file\n" +
 						"kubectl -n istio-system get configmap istio-sidecar-injector " +
@@ -334,13 +337,6 @@ istioctl kube-inject -f deployment.yaml -o deployment-injected.yaml --injectConf
 	}
 )
 
-func getBoolEnv(key string, defaultVal bool) bool {
-	if svalue, ok := os.LookupEnv(key); ok {
-		return strings.ToLower(svalue) == "true" || svalue == "1"
-	}
-	return defaultVal
-}
-
 const (
 	defaultMeshConfigMapName   = "istio"
 	defaultInjectConfigMapName = "istio-sidecar-injector"

diff --git a/mixer/adapter/kubernetesenv/kubernetesenv.go b/mixer/adapter/kubernetesenv/kubernetesenv.go
@@ -28,7 +28,6 @@ import (
 	"errors"
 	"fmt"
 	"net"
-	"os"
 	"strings"
 	"sync"
 	"time"
@@ -42,6 +41,7 @@ import (
 	"istio.io/istio/mixer/adapter/kubernetesenv/config"
 	ktmpl "istio.io/istio/mixer/adapter/kubernetesenv/template"
 	"istio.io/istio/mixer/pkg/adapter"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/kube/secretcontroller"
 )
 
@@ -117,12 +117,14 @@ func (b *builder) Validate() (ce *adapter.ConfigErrors) {
 	return
 }
 
+var kubeConfigVar = env.RegisterStringVar("KUBECONFIG", "", "")
+
 func (b *builder) Build(ctx context.Context, env adapter.Env) (adapter.Handler, error) {
 	paramsProto := b.adapterConfig
 	var controller cacheController
 	var controllers = make(map[string]cacheController)
 
-	path, exists := os.LookupEnv("KUBECONFIG")
+	path, exists := kubeConfigVar.Lookup()
 	if !exists {
 		path = paramsProto.KubeconfigPath
 	}
@@ -395,14 +397,14 @@ func (b *builder) deleteCacheController(clusterID string) error {
 	return nil
 }
 
+var clusterNsVar = env.RegisterStringVar("POD_NAMESPACE", defaultClusterRegistriesNamespace, "")
+
 func initMultiClusterSecretController(b *builder, kubeconfig string, env adapter.Env) (err error) {
 	var clusterNs string
 
 	paramsProto := b.adapterConfig
 	if clusterNs = paramsProto.ClusterRegistriesNamespace; clusterNs == "" {
-		if clusterNs = os.Getenv("POD_NAMESPACE"); clusterNs == "" {
-			clusterNs = defaultClusterRegistriesNamespace
-		}
+		clusterNs = clusterNsVar.Get()
 	}
 
 	kubeClient, err := b.newClientFn(kubeconfig, env)

diff --git a/mixer/pkg/config/mcp/backend.go b/mixer/pkg/config/mcp/backend.go
@@ -30,6 +30,7 @@ import (
 	mcp "istio.io/api/mcp/v1alpha1"
 	"istio.io/istio/galley/pkg/metadata/kube"
 	"istio.io/istio/mixer/pkg/config/store"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/log"
 	"istio.io/istio/pkg/mcp/client"
 	"istio.io/istio/pkg/mcp/configz"
@@ -131,6 +132,8 @@ type state struct {
 	synced map[string]bool // by collection
 }
 
+var useMCPLegacyVar = env.RegisterBoolVar("USE_MCP_LEGACY", false, "")
+
 // Init implements store.Backend.Init.
 func (b *backend) Init(kinds []string) error {
 	m, err := constructMapping(kinds, kube.Types)
@@ -201,8 +204,8 @@ func (b *backend) Init(kinds []string) error {
 	}
 
 	// TODO - temporarily support both the new and old stack during transition
-	if os.Getenv("USE_MCP_LEGACY") == "1" {
-		log.Infof("USE_MCP_LEGACY=1 - using legacy MCP client stack")
+	if useMCPLegacyVar.Get() {
+		log.Infof("USE_MCP_LEGACY=true - using legacy MCP client stack")
 
 		cl := mcp.NewAggregatedMeshConfigServiceClient(conn)
 		c := client.New(cl, options)

diff --git a/mixer/pkg/protobuf/yaml/dynamic/auth.go b/mixer/pkg/protobuf/yaml/dynamic/auth.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/url"
-	"os"
 
 	"github.com/pkg/errors"
 	"golang.org/x/oauth2"
@@ -31,6 +30,7 @@ import (
 	"google.golang.org/grpc/credentials/oauth"
 
 	policypb "istio.io/api/policy/v1beta1"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/log"
 )
 
@@ -99,6 +99,8 @@ func getWhitelistSAN(cert []byte) []*url.URL {
 	return whitelistSAN
 }
 
+var bypassVerificationVar = env.RegisterBoolVar("BYPASS_OOP_MTLS_SAN_VERIFICATION", false, "")
+
 func buildMTLSDialOption(mtlsCfg *policypb.Mutual) ([]grpc.DialOption, error) {
 	// load peer cert/key.
 	pk := mtlsCfg.GetPrivateKey()
@@ -145,7 +147,7 @@ func buildMTLSDialOption(mtlsCfg *policypb.Mutual) ([]grpc.DialOption, error) {
 		if _, err = certs[0].Verify(opts); err != nil {
 			return err
 		}
-		if os.Getenv("BYPASS_OOP_MTLS_SAN_VERIFICATION") == "true" {
+		if bypassVerificationVar.Get() {
 			log.Infof("BYPASS_OOP_MTLS_SAN_VERIFICATION=true - bypassing mtls custom SAN verification")
 			return nil
 		}

diff --git a/mixer/pkg/runtime/lang/lang.go b/mixer/pkg/runtime/lang/lang.go
@@ -16,13 +16,12 @@
 package lang
 
 import (
-	"os"
-
 	"istio.io/api/policy/v1beta1"
 	"istio.io/istio/mixer/pkg/lang/ast"
 	"istio.io/istio/mixer/pkg/lang/cel"
 	"istio.io/istio/mixer/pkg/lang/checker"
 	"istio.io/istio/mixer/pkg/lang/compiled"
+	"istio.io/istio/pkg/env"
 )
 
 type (
@@ -53,9 +52,11 @@ const (
 	LanguageRuntimeAnnotation = "policy.istio.io/lang"
 )
 
+var langVar = env.RegisterStringVar("ISTIO_LANG", "", "")
+
 // GetLanguageRuntime reads an override from a resource annotation
 func GetLanguageRuntime(annotations map[string]string) LanguageRuntime {
-	if override, has := os.LookupEnv("ISTIO_LANG"); has {
+	if override, has := langVar.Lookup(); has {
 		return fromString(override)
 	}
 	return fromString(annotations[LanguageRuntimeAnnotation])

diff --git a/pilot/cmd/pilot-agent/main.go b/pilot/cmd/pilot-agent/main.go
@@ -29,8 +29,6 @@ import (
 	"text/template"
 	"time"
 
-	"istio.io/istio/pkg/spiffe"
-
 	"github.com/gogo/protobuf/types"
 	"github.com/spf13/cobra"
 	"github.com/spf13/cobra/doc"
@@ -43,8 +41,10 @@ import (
 	"istio.io/istio/pilot/pkg/serviceregistry"
 	"istio.io/istio/pkg/cmd"
 	"istio.io/istio/pkg/collateral"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/features/pilot"
 	"istio.io/istio/pkg/log"
+	"istio.io/istio/pkg/spiffe"
 	"istio.io/istio/pkg/version"
 )
 
@@ -89,6 +89,12 @@ var (
 
 	wg sync.WaitGroup
 
+	instanceIPVar        = env.RegisterStringVar("INSTANCE_IP", "", "")
+	podNameVar           = env.RegisterStringVar("POD_NAME", "", "")
+	podNamespaceVar      = env.RegisterStringVar("POD_NAMESPACE", "", "")
+	istioNamespaceVar    = env.RegisterStringVar("ISTIO_NAMESPACE", "", "")
+	kubeAppProberNameVar = env.RegisterStringVar(status.KubeAppProberEnvName, "", "")
+
 	rootCmd = &cobra.Command{
 		Use:          "pilot-agent",
 		Short:        "Istio Pilot agent.",
@@ -118,7 +124,7 @@ var (
 			if len(proxyIP) != 0 {
 				role.IPAddresses = append(role.IPAddresses, proxyIP)
 			} else {
-				envIP := os.Getenv("INSTANCE_IP")
+				envIP := instanceIPVar.Get()
 				if len(envIP) > 0 {
 					role.IPAddresses = append(role.IPAddresses, envIP)
 				}
@@ -137,7 +143,7 @@ var (
 
 			if len(role.ID) == 0 {
 				if registry == serviceregistry.KubernetesRegistry {
-					role.ID = os.Getenv("POD_NAME") + "." + os.Getenv("POD_NAMESPACE")
+					role.ID = podNameVar.Get() + "." + podNamespaceVar.Get()
 				} else if registry == serviceregistry.ConsulRegistry {
 					role.ID = role.IPAddresses[0] + ".service.consul"
 				} else {
@@ -213,15 +219,15 @@ var (
 					if len(parts) == 1 {
 						// namespace of pilot is not part of discovery address use
 						// pod namespace e.g. istio-pilot:15005
-						ns = os.Getenv("POD_NAMESPACE")
+						ns = podNamespaceVar.Get()
 					} else if len(parts) == 2 {
 						// namespace is found in the discovery address
 						// e.g. istio-pilot.istio-system:15005
 						ns = parts[1]
 					} else {
 						// discovery address is a remote address. For remote clusters
 						// only support the default config, or env variable
-						ns = os.Getenv("ISTIO_NAMESPACE")
+						ns = istioNamespaceVar.Get()
 						if ns == "" {
 							ns = model.IstioSystemNamespace
 						}
@@ -292,8 +298,8 @@ var (
 			if controlPlaneBootstrap {
 				if templateFile != "" && proxyConfig.CustomConfigFile == "" {
 					opts := make(map[string]string)
-					opts["PodName"] = os.Getenv("POD_NAME")
-					opts["PodNamespace"] = os.Getenv("POD_NAMESPACE")
+					opts["PodName"] = podNameVar.Get()
+					opts["PodNamespace"] = podNamespaceVar.Get()
 
 					mixerSAN := getSAN(ns, envoy.MixerSvcAccName, role.MixerIdentity)
 					log.Infof("MixerSAN %#v", mixerSAN)
@@ -302,7 +308,7 @@ var (
 					}
 
 					// protobuf encoding of IP_ADDRESS type
-					opts["PodIP"] = base64.StdEncoding.EncodeToString(net.ParseIP(os.Getenv("INSTANCE_IP")))
+					opts["PodIP"] = base64.StdEncoding.EncodeToString(net.ParseIP(instanceIPVar.Get()))
 
 					if proxyConfig.ControlPlaneAuthPolicy == meshconfig.AuthenticationPolicy_MUTUAL_TLS {
 						opts["ControlPlaneAuth"] = "enable"
@@ -344,7 +350,7 @@ var (
 					return err
 				}
 
-				prober := os.Getenv(status.KubeAppProberEnvName)
+				prober := kubeAppProberNameVar.Get()
 				statusServer, err := status.NewServer(status.Config{
 					AdminPort:          proxyAdminPort,
 					StatusPort:         statusPort,
@@ -398,7 +404,7 @@ func setSpiffeTrustDomain(domain string) {
 		pilotTrustDomain := role.TrustDomain
 		if len(pilotTrustDomain) == 0 {
 			if registry == serviceregistry.KubernetesRegistry &&
-				(domain == os.Getenv("POD_NAMESPACE")+".svc.cluster.local" || domain == "") {
+				(domain == podNamespaceVar.Get()+".svc.cluster.local" || domain == "") {
 				pilotTrustDomain = "cluster.local"
 			} else if registry == serviceregistry.ConsulRegistry &&
 				(domain == "service.consul" || domain == "") {
@@ -428,7 +434,7 @@ func getSAN(ns string, defaultSA string, overrideIdentity string) []string {
 func getDNSDomain(domain string) string {
 	if len(domain) == 0 {
 		if registry == serviceregistry.KubernetesRegistry {
-			domain = os.Getenv("POD_NAMESPACE") + ".svc.cluster.local"
+			domain = podNamespaceVar.Get() + ".svc.cluster.local"
 		} else if registry == serviceregistry.ConsulRegistry {
 			domain = "service.consul"
 		} else {

diff --git a/pilot/pkg/bootstrap/server.go b/pilot/pkg/bootstrap/server.go
@@ -26,7 +26,6 @@ import (
 	"os"
 	"path"
 	"reflect"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -70,6 +69,7 @@ import (
 	"istio.io/istio/pilot/pkg/serviceregistry/kube"
 	srmemory "istio.io/istio/pilot/pkg/serviceregistry/memory"
 	"istio.io/istio/pkg/ctrlz"
+	"istio.io/istio/pkg/env"
 	"istio.io/istio/pkg/features/pilot"
 	"istio.io/istio/pkg/filewatcher"
 	istiokeepalive "istio.io/istio/pkg/keepalive"
@@ -211,11 +211,13 @@ type Server struct {
 	fileWatcher      filewatcher.FileWatcher
 }
 
+var podNamespaceVar = env.RegisterStringVar("POD_NAMESPACE", "", "")
+
 // NewServer creates a new Server instance based on the provided arguments.
 func NewServer(args PilotArgs) (*Server, error) {
 	// If the namespace isn't set, try looking it up from the environment.
 	if args.Namespace == "" {
-		args.Namespace = os.Getenv("POD_NAMESPACE")
+		args.Namespace = podNamespaceVar.Get()
 	}
 	if args.KeepaliveOptions == nil {
 		args.KeepaliveOptions = istiokeepalive.DefaultOption()
@@ -494,6 +496,8 @@ func (c *mockController) AppendInstanceHandler(f func(*model.ServiceInstance, mo
 
 func (c *mockController) Run(<-chan struct{}) {}
 
+var useMCPLegacyVar = env.RegisterBoolVar("USE_MCP_LEGACY", false, "")
+
 func (s *Server) initMCPConfigController(args *PilotArgs) error {
 	clientNodeID := ""
 	collections := make([]sink.CollectionOptions, len(model.IstioConfigTypes))
@@ -518,9 +522,9 @@ func (s *Server) initMCPConfigController(args *PilotArgs) error {
 
 	// TODO - temporarily support both the new and old stack during transition
 	var useLegacyMCPStack bool
-	if os.Getenv("USE_MCP_LEGACY") == "1" {
+	if useMCPLegacyVar.Get() {
 		useLegacyMCPStack = true
-		log.Infof("USE_MCP_LEGACY=1 - using legacy MCP client stack")
+		log.Infof("USE_MCP_LEGACY=true - using legacy MCP client stack")
 	} else {
 		log.Infof("Using new MCP client sink stack")
 	}
@@ -1198,14 +1202,7 @@ func (s *Server) grpcServerOptions(options *istiokeepalive.Options) []grpc.Serve
 
 	// Temp setting, default should be enough for most supported environments. Can be used for testing
 	// envoy with lower values.
-	var maxStreams int
-	maxStreamsEnv := pilot.MaxConcurrentStreams
-	if len(maxStreamsEnv) > 0 {
-		maxStreams, _ = strconv.Atoi(maxStreamsEnv)
-	}
-	if maxStreams == 0 {
-		maxStreams = 100000
-	}
+	maxStreams := pilot.MaxConcurrentStreams
 
 	grpcOptions := []grpc.ServerOption{
 		grpc.UnaryInterceptor(middleware.ChainUnaryServer(interceptors...)),