From 8ec1958667e66fb3da2a1f1428998f59f8b027f2 Mon Sep 17 00:00:00 2001 From: Michael Taufen Date: Sun, 24 Dec 2017 19:19:46 -0600 Subject: [PATCH] All Kubelet flags should be explicitly registered This explicitly registers Kubelet flags from libraries that were registering flags globally, and stops parsing the global flag set. In general, we should always be explicit about flags we register and parse, so that we maintain control over our command-line API. --- cmd/kubelet/BUILD | 1 + cmd/kubelet/app/options/BUILD | 13 ++ cmd/kubelet/app/options/globalflags.go | 167 ++++++++++++++++++ cmd/kubelet/kubelet.go | 33 +++- .../azure/azure_credentials.go | 2 +- pkg/credentialprovider/gcp/jwt.go | 6 +- pkg/version/verflag/verflag.go | 8 +- .../k8s.io/apiserver/pkg/util/logs/logs.go | 10 +- test/e2e/BUILD | 2 +- test/e2e/e2e.go | 2 +- 10 files changed, 231 insertions(+), 13 deletions(-) create mode 100644 cmd/kubelet/app/options/globalflags.go diff --git a/cmd/kubelet/BUILD b/cmd/kubelet/BUILD index a5b72a1e9ec..b077cfa4b8f 100644 --- a/cmd/kubelet/BUILD +++ b/cmd/kubelet/BUILD @@ -24,6 +24,7 @@ go_library( "//pkg/client/metrics/prometheus:go_default_library", "//pkg/version/prometheus:go_default_library", "//pkg/version/verflag:go_default_library", + "//vendor/github.com/golang/glog:go_default_library", "//vendor/github.com/spf13/pflag:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library", diff --git a/cmd/kubelet/app/options/BUILD b/cmd/kubelet/app/options/BUILD index aca4a34882f..0d8a49e1e96 100644 --- a/cmd/kubelet/app/options/BUILD +++ b/cmd/kubelet/app/options/BUILD @@ -10,12 +10,15 @@ go_library( name = "go_default_library", srcs = [ "container_runtime.go", + "globalflags.go", "options.go", ], importpath = "k8s.io/kubernetes/cmd/kubelet/app/options", deps = [ "//pkg/apis/componentconfig:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/credentialprovider/azure:go_default_library", + "//pkg/credentialprovider/gcp:go_default_library", "//pkg/features:go_default_library", "//pkg/kubelet/apis/kubeletconfig:go_default_library", "//pkg/kubelet/apis/kubeletconfig/scheme:go_default_library", @@ -24,10 +27,20 @@ go_library( "//pkg/kubelet/config:go_default_library", "//pkg/kubelet/types:go_default_library", "//pkg/util/taints:go_default_library", + "//pkg/version/verflag:go_default_library", + "//vendor/github.com/golang/glog:go_default_library", + "//vendor/github.com/google/cadvisor/container/common:go_default_library", + "//vendor/github.com/google/cadvisor/container/containerd:go_default_library", + "//vendor/github.com/google/cadvisor/container/docker:go_default_library", + "//vendor/github.com/google/cadvisor/container/raw:go_default_library", + "//vendor/github.com/google/cadvisor/machine:go_default_library", + "//vendor/github.com/google/cadvisor/manager:go_default_library", + "//vendor/github.com/google/cadvisor/storage:go_default_library", "//vendor/github.com/spf13/pflag:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library", + "//vendor/k8s.io/apiserver/pkg/util/logs:go_default_library", ], ) diff --git a/cmd/kubelet/app/options/globalflags.go b/cmd/kubelet/app/options/globalflags.go new file mode 100644 index 00000000000..85829930c0b --- /dev/null +++ b/cmd/kubelet/app/options/globalflags.go @@ -0,0 +1,167 @@ +/* +Copyright 2018 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 options + +import ( + "flag" + "fmt" + "os" + "strings" + + "github.com/spf13/pflag" + + // libs that provide registration functions + "k8s.io/apiserver/pkg/util/logs" + "k8s.io/kubernetes/pkg/version/verflag" + + // ensure libs have a chance to globally register their flags + _ "github.com/golang/glog" + _ "github.com/google/cadvisor/container/common" + _ "github.com/google/cadvisor/container/containerd" + _ "github.com/google/cadvisor/container/docker" + _ "github.com/google/cadvisor/container/raw" + _ "github.com/google/cadvisor/machine" + _ "github.com/google/cadvisor/manager" + _ "github.com/google/cadvisor/storage" + _ "k8s.io/kubernetes/pkg/credentialprovider/azure" + _ "k8s.io/kubernetes/pkg/credentialprovider/gcp" +) + +// AddGlobalFlags explicitly registers flags that libraries (glog, verflag, etc.) register +// against the global flagsets from "flag" and "github.com/spf13/pflag". +// We do this in order to prevent unwanted flags from leaking into the Kubelet's flagset. +func AddGlobalFlags(fs *pflag.FlagSet) { + addGlogFlags(fs) + addCadvisorFlags(fs) + verflag.AddFlags(fs) + logs.AddFlags(fs) +} + +// normalize replaces underscores with hyphens +// we should always use hyphens instead of underscores when registering kubelet flags +func normalize(s string) string { + return strings.Replace(s, "_", "-", -1) +} + +// register adds a flag to local that targets the Value associated with the Flag named globalName in global +func register(global *flag.FlagSet, local *pflag.FlagSet, globalName string) { + if f := global.Lookup(globalName); f != nil { + f.Name = normalize(f.Name) + local.AddFlag(pflag.PFlagFromGoFlag(f)) + } else { + panic(fmt.Sprintf("failed to find flag in global flagset (flag): %s", globalName)) + } +} + +// pflagRegister adds a flag to local that targets the Value associated with the Flag named globalName in global +func pflagRegister(global, local *pflag.FlagSet, globalName string) { + if f := global.Lookup(globalName); f != nil { + f.Name = normalize(f.Name) + local.AddFlag(f) + } else { + panic(fmt.Sprintf("failed to find flag in global flagset (pflag): %s", globalName)) + } +} + +// registerDeprecated registers the flag with register, and then marks it deprecated +func registerDeprecated(global *flag.FlagSet, local *pflag.FlagSet, globalName, deprecated string) { + register(global, local, globalName) + local.Lookup(normalize(globalName)).Deprecated = deprecated +} + +// pflagRegisterDeprecated registers the flag with pflagRegister, and then marks it deprecated +func pflagRegisterDeprecated(global, local *pflag.FlagSet, globalName, deprecated string) { + pflagRegister(global, local, globalName) + local.Lookup(normalize(globalName)).Deprecated = deprecated +} + +// addCredentialProviderFlags adds flags from k8s.io/kubernetes/pkg/credentialprovider +func addCredentialProviderFlags(fs *pflag.FlagSet) { + // lookup flags in global flag set and re-register the values with our flagset + global := pflag.CommandLine + local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError) + + // Note this is deprecated in the library that provides it, so we just allow that deprecation + // notice to pass through our registration here. + pflagRegister(global, local, "google-json-key") + // TODO(#58034): This is not a static file, so it's not quite as straightforward as --google-json-key. + // We need to figure out how ACR users can dynamically provide pull credentials before we can deprecate this. + pflagRegister(global, local, "azure-container-registry-config") + + fs.AddFlagSet(local) +} + +// addGlogFlags adds flags from github.com/golang/glog +func addGlogFlags(fs *pflag.FlagSet) { + // lookup flags in global flag set and re-register the values with our flagset + global := flag.CommandLine + local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError) + + register(global, local, "logtostderr") + register(global, local, "alsologtostderr") + register(global, local, "v") + register(global, local, "stderrthreshold") + register(global, local, "vmodule") + register(global, local, "log_backtrace_at") + register(global, local, "log_dir") + + fs.AddFlagSet(local) +} + +// addCadvisorFlags adds flags from cadvisor +func addCadvisorFlags(fs *pflag.FlagSet) { + // lookup flags in global flag set and re-register the values with our flagset + global := flag.CommandLine + local := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError) + + // These flags were also implicit from cadvisor, but are actually used by something in the core repo: + // TODO(mtaufen): This one is stil used by our salt, but for heaven's sake it's even deprecated in cadvisor + register(global, local, "docker_root") + // e2e node tests rely on this + register(global, local, "housekeeping_interval") + + // These flags were implicit from cadvisor, and are mistakes that should be registered deprecated: + const deprecated = "This is a cadvisor flag that was mistakenly registered with the Kubelet. Due to legacy concerns, it will follow the standard CLI deprecation timeline before being removed." + + registerDeprecated(global, local, "application_metrics_count_limit", deprecated) + registerDeprecated(global, local, "boot_id_file", deprecated) + registerDeprecated(global, local, "container_hints", deprecated) + registerDeprecated(global, local, "containerd", deprecated) + registerDeprecated(global, local, "docker", deprecated) + registerDeprecated(global, local, "docker_env_metadata_whitelist", deprecated) + registerDeprecated(global, local, "docker_only", deprecated) + registerDeprecated(global, local, "docker-tls", deprecated) + registerDeprecated(global, local, "docker-tls-ca", deprecated) + registerDeprecated(global, local, "docker-tls-cert", deprecated) + registerDeprecated(global, local, "docker-tls-key", deprecated) + registerDeprecated(global, local, "enable_load_reader", deprecated) + registerDeprecated(global, local, "event_storage_age_limit", deprecated) + registerDeprecated(global, local, "event_storage_event_limit", deprecated) + registerDeprecated(global, local, "global_housekeeping_interval", deprecated) + registerDeprecated(global, local, "log_cadvisor_usage", deprecated) + registerDeprecated(global, local, "machine_id_file", deprecated) + registerDeprecated(global, local, "storage_driver_user", deprecated) + registerDeprecated(global, local, "storage_driver_password", deprecated) + registerDeprecated(global, local, "storage_driver_host", deprecated) + registerDeprecated(global, local, "storage_driver_db", deprecated) + registerDeprecated(global, local, "storage_driver_table", deprecated) + registerDeprecated(global, local, "storage_driver_secure", deprecated) + registerDeprecated(global, local, "storage_driver_buffer_duration", deprecated) + + // finally, add cadvisor flags to the provided flagset + fs.AddFlagSet(local) +} diff --git a/cmd/kubelet/kubelet.go b/cmd/kubelet/kubelet.go index 891aace197c..76b86233dc6 100644 --- a/cmd/kubelet/kubelet.go +++ b/cmd/kubelet/kubelet.go @@ -24,6 +24,7 @@ import ( "fmt" "os" + "github.com/golang/glog" "github.com/spf13/pflag" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -36,25 +37,43 @@ import ( "k8s.io/kubernetes/pkg/version/verflag" ) +func parseFlagSet(fs *pflag.FlagSet, args []string) error { + if err := fs.Parse(args); err != nil { + return err + } + fs.VisitAll(func(flag *pflag.Flag) { + glog.V(2).Infof("FLAG: --%s=%q", flag.Name, flag.Value) + }) + return nil +} + func die(err error) { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) } func main() { - // construct KubeletFlags object and register command line flags mapping - kubeletFlags := options.NewKubeletFlags() - kubeletFlags.AddFlags(pflag.CommandLine) + fs := pflag.NewFlagSet(os.Args[0], pflag.ExitOnError) + // set the normalize func, similar to k8s.io/apiserver/pkg/util/flag/flags.go:InitFlags + fs.SetNormalizeFunc(flag.WordSepNormalizeFunc) + // explicitly add flags from libs that register global flags + options.AddGlobalFlags(fs) - // construct KubeletConfiguration object and register command line flags mapping + // register kubelet flags + kubeletFlags := options.NewKubeletFlags() + kubeletFlags.AddFlags(fs) + + // register kubelet config flags defaultConfig, err := options.NewKubeletConfiguration() if err != nil { die(err) } - options.AddKubeletConfigFlags(pflag.CommandLine, defaultConfig) + options.AddKubeletConfigFlags(fs, defaultConfig) - // parse the command line flags into the respective objects - flag.InitFlags() + // parse flags + if err := parseFlagSet(fs, os.Args[1:]); err != nil { + die(err) + } // initialize logging and defer flush logs.InitLogs() diff --git a/pkg/credentialprovider/azure/azure_credentials.go b/pkg/credentialprovider/azure/azure_credentials.go index e48d8133f55..6edf6fe30ad 100644 --- a/pkg/credentialprovider/azure/azure_credentials.go +++ b/pkg/credentialprovider/azure/azure_credentials.go @@ -34,7 +34,7 @@ import ( ) var flagConfigFile = pflag.String("azure-container-registry-config", "", - "Path to the file container Azure container registry configuration information.") + "Path to the file containing Azure container registry configuration information.") const dummyRegistryEmail = "name@contoso.com" diff --git a/pkg/credentialprovider/gcp/jwt.go b/pkg/credentialprovider/gcp/jwt.go index b34c0fcaaf0..d187560a351 100644 --- a/pkg/credentialprovider/gcp/jwt.go +++ b/pkg/credentialprovider/gcp/jwt.go @@ -31,10 +31,11 @@ import ( const ( storageReadOnlyScope = "https://www.googleapis.com/auth/devstorage.read_only" + jwtFileFlagName = "google-json-key" ) var ( - flagJwtFile = pflag.String("google-json-key", "", + flagJwtFile = pflag.String(jwtFileFlagName, "", "The Google Cloud Platform Service Account JSON Key to use for authentication.") ) @@ -49,6 +50,9 @@ type jwtProvider struct { // init registers the various means by which credentials may // be resolved on GCP. func init() { + pflag.CommandLine.MarkDeprecated(jwtFileFlagName, "Will be removed in a future version. "+ + "To maintain node-level authentication, credentials should instead be included in a docker "+ + "config.json file, located inside the Kubelet's --root-dir.") credentialprovider.RegisterCredentialProvider("google-jwt-key", &credentialprovider.CachingDockerConfigProvider{ Provider: &jwtProvider{ diff --git a/pkg/version/verflag/verflag.go b/pkg/version/verflag/verflag.go index 5809c3aa8f4..0c078052774 100644 --- a/pkg/version/verflag/verflag.go +++ b/pkg/version/verflag/verflag.go @@ -85,10 +85,16 @@ func Version(name string, value versionValue, usage string) *versionValue { return p } +const versionFlagName = "version" + var ( - versionFlag = Version("version", VersionFalse, "Print version information and quit") + versionFlag = Version(versionFlagName, VersionFalse, "Print version information and quit") ) +func AddFlags(fs *flag.FlagSet) { + fs.AddFlag(flag.Lookup(versionFlagName)) +} + // PrintAndExitIfRequested will check if the -version flag was passed // and, if so, print the version and exit. func PrintAndExitIfRequested() { diff --git a/staging/src/k8s.io/apiserver/pkg/util/logs/logs.go b/staging/src/k8s.io/apiserver/pkg/util/logs/logs.go index a3909583a7c..c5ba084a59e 100644 --- a/staging/src/k8s.io/apiserver/pkg/util/logs/logs.go +++ b/staging/src/k8s.io/apiserver/pkg/util/logs/logs.go @@ -26,13 +26,21 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -var logFlushFreq = pflag.Duration("log-flush-frequency", 5*time.Second, "Maximum number of seconds between log flushes") +const logFlushFreqFlagName = "log-flush-frequency" + +var logFlushFreq = pflag.Duration(logFlushFreqFlagName, 5*time.Second, "Maximum number of seconds between log flushes") // TODO(thockin): This is temporary until we agree on log dirs and put those into each cmd. func init() { flag.Set("logtostderr", "true") } +// AddFlags registers this package's flags on arbitrary FlagSets, such that they point to the +// same value as the global flags. +func AddFlags(fs *pflag.FlagSet) { + fs.AddFlag(pflag.Lookup(logFlushFreqFlagName)) +} + // GlogWriter serves as a bridge between the standard log package and the glog package. type GlogWriter struct{} diff --git a/test/e2e/BUILD b/test/e2e/BUILD index a619f53166f..88af77029ab 100644 --- a/test/e2e/BUILD +++ b/test/e2e/BUILD @@ -48,7 +48,6 @@ go_library( "//pkg/api/v1/pod:go_default_library", "//pkg/cloudprovider/providers/azure:go_default_library", "//pkg/cloudprovider/providers/gce:go_default_library", - "//pkg/kubectl/util/logs:go_default_library", "//pkg/version:go_default_library", "//test/e2e/common:go_default_library", "//test/e2e/framework:go_default_library", @@ -71,6 +70,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library", + "//vendor/k8s.io/apiserver/pkg/util/logs:go_default_library", "//vendor/k8s.io/client-go/kubernetes:go_default_library", ], ) diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index d456bee4153..63211b6892e 100644 --- a/test/e2e/e2e.go +++ b/test/e2e/e2e.go @@ -32,10 +32,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtimeutils "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apiserver/pkg/util/logs" clientset "k8s.io/client-go/kubernetes" "k8s.io/kubernetes/pkg/cloudprovider/providers/azure" gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" - "k8s.io/kubernetes/pkg/kubectl/util/logs" "k8s.io/kubernetes/pkg/version" commontest "k8s.io/kubernetes/test/e2e/common" "k8s.io/kubernetes/test/e2e/framework"