Merge pull request #2948 from twz123/prevent-leaking-global-flags

Prevent leaking global flags from dependencies

.golangci.yml

@@ -22,6 +22,14 @@ linters-settings:
   depguard:
     packages:
       - gopkg.in/yaml*
+    additional-guards:
+      # Only allow usages of the k8s cloud provider from within the k0s cloud
+      # provider package. This is to ensure that it's not leaking global flags
+      # into k0s.
+      - packages:
+          - k8s.io/cloud-provider*
+        ignore-file-rules:
+          - "**/pkg/k0scloudprovider/*.go"
   golint:
     min-confidence: 0
   goheader:

cmd/kubectl/kubectl.go

@@ -32,7 +32,6 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
 )
 
 func checkKubectlInPath() {
@@ -65,9 +64,6 @@ func (h *kubectlPluginHandler) Execute(executablePath string, cmdArgs, environme
 }
 
 func NewK0sKubectlCmd() *cobra.Command {
-	_ = pflag.CommandLine.MarkHidden("log-flush-frequency")
-	_ = pflag.CommandLine.MarkHidden("version")
-
 	var idx int
 	for i, arg := range os.Args {
 		if arg == "kubectl" || arg == "kc" {

cmd/root_test.go

@@ -0,0 +1,68 @@
+/*
+Copyright 2023 k0s 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 cmd_test
+
+import (
+	"bytes"
+	"io"
+	"testing"
+
+	"github.com/k0sproject/k0s/cmd"
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/assert"
+	"golang.org/x/exp/slices"
+)
+
+// TestRootCmd_Flags ensures that no unwanted global flags have been registered
+// and leak into k0s. This happens rather quickly, e.g. if some dependency puts
+// stuff into pflag.CommandLine.
+func TestRootCmd_Flags(t *testing.T) {
+	expectedVisibleFlags := []string{"help"}
+	expectedHiddenFlags := []string{
+		"version", // registered by k0scloudprovider; unwanted but unavoidable
+	}
+
+	var stderr bytes.Buffer
+
+	underTest := cmd.NewRootCmd()
+	underTest.SetArgs(nil)
+	underTest.SetOut(io.Discard) // Don't care about the usage output here
+	underTest.SetErr(&stderr)
+
+	err := underTest.Execute()
+
+	assert.NoError(t, err)
+	assert.Empty(t, stderr.String(), "Something has been written to stderr")
+
+	// This has to happen after the command has been executed.
+	// Cobra will have populated everything by then.
+	var visibleFlags []string
+	var hiddenFlags []string
+	underTest.Flags().VisitAll(func(f *pflag.Flag) {
+		if f.Hidden {
+			hiddenFlags = append(hiddenFlags, f.Name)
+		} else {
+			visibleFlags = append(visibleFlags, f.Name)
+		}
+	})
+
+	slices.Sort(visibleFlags)
+	slices.Sort(hiddenFlags)
+
+	assert.Equal(t, expectedVisibleFlags, visibleFlags, "visible flags changed unexpectedly")
+	assert.Equal(t, expectedHiddenFlags, hiddenFlags, "hidden flags changed unexpectedly")
+}

pkg/config/cli.go

@@ -28,8 +28,7 @@ import (
 	aproot "github.com/k0sproject/k0s/pkg/autopilot/controller/root"
 	"github.com/k0sproject/k0s/pkg/component/manager"
 	"github.com/k0sproject/k0s/pkg/constant"
-
-	cloudprovider "k8s.io/cloud-provider"
+	"github.com/k0sproject/k0s/pkg/k0scloudprovider"
 
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -226,7 +225,7 @@ func GetControllerFlags() *pflag.FlagSet {
 	flagset.BoolVar(&controllerOpts.NoTaints, "no-taints", false, "disable default taints for controller node")
 	flagset.BoolVar(&controllerOpts.EnableK0sCloudProvider, "enable-k0s-cloud-provider", false, "enables the k0s-cloud-provider (default false)")
 	flagset.DurationVar(&controllerOpts.K0sCloudProviderUpdateFrequency, "k0s-cloud-provider-update-frequency", 2*time.Minute, "the frequency of k0s-cloud-provider node updates")
-	flagset.IntVar(&controllerOpts.K0sCloudProviderPort, "k0s-cloud-provider-port", cloudprovider.CloudControllerManagerPort, "the port that k0s-cloud-provider binds on")
+	flagset.IntVar(&controllerOpts.K0sCloudProviderPort, "k0s-cloud-provider-port", k0scloudprovider.DefaultBindPort, "the port that k0s-cloud-provider binds on")
 	flagset.AddFlagSet(GetCriSocketFlag())
 	flagset.BoolVar(&controllerOpts.EnableDynamicConfig, "enable-dynamic-config", false, "enable cluster-wide dynamic config based on custom resource")
 	flagset.BoolVar(&controllerOpts.EnableMetricsScraper, "enable-metrics-scraper", false, "enable scraping metrics from the controller components (kube-scheduler, kube-controller-manager)")

pkg/k0scloudprovider/command.go

@@ -21,14 +21,32 @@ import (
 	"time"
 
 	"github.com/sirupsen/logrus"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"github.com/spf13/pflag"
+
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/cloud-provider/app"
 	"k8s.io/cloud-provider/app/config"
 	"k8s.io/cloud-provider/options"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	cliflag "k8s.io/component-base/cli/flag"
+	"k8s.io/component-base/version/verflag"
+)
+
+const (
+	// DefaultBindPort is the default port for the cloud controller manager
+	// server. This value may be overridden by a flag at startup.
+	//
+	// (The constant has been aliased from k8s.io/cloud-provider, as importing
+	// that package directly calls some init functions that register unwanted
+	// global CLI flags. This package's init function suppresses those flags.)
+	DefaultBindPort = cloudprovider.CloudControllerManagerPort
 )
 
+func init() {
+	hideUndesiredGlobalFlags()
+}
+
 type Command func(stopCh <-chan struct{})
 
 type Config struct {
@@ -60,7 +78,7 @@ func NewCommand(c Config) (Command, error) {
 
 	cloudInitializer := func(*config.CompletedConfig) cloudprovider.Interface {
 		// Returns the "k0s cloud provider" using the specified `AddressCollector`
-		return NewProvider(c.AddressCollector)
+		return newProvider(c.AddressCollector)
 	}
 
 	// K0s only supports the cloud-node controller, so only use that.
@@ -86,3 +104,14 @@ func NewCommand(c Config) (Command, error) {
 		}
 	}, nil
 }
+
+// hideUndesiredGlobalFlags hides some global flags registered by k8s.io
+// components, so that they aren't displayed in help texts. These flags will
+// still be accepted by Cobra, i.e. they won't cause flag parsing errors, but
+// that's all that can be done, since Cobra doesn't allow removing flags, nor is
+// there a way to intercept and suppress their addition.
+func hideUndesiredGlobalFlags() {
+	var flagsToHide pflag.FlagSet
+	verflag.AddFlags(&flagsToHide)
+	flagsToHide.VisitAll(func(f *pflag.Flag) { f.Hidden = true })
+}

pkg/k0scloudprovider/provider.go

@@ -30,9 +30,9 @@ type provider struct {
 
 var _ cloudprovider.Interface = (*provider)(nil)
 
-// NewProvider creates a new `cloudprovider.Interfaces` using the
-// provided `AddressCollector`
-func NewProvider(ac AddressCollector) cloudprovider.Interface {
+// newProvider creates a new cloud provider using the provided
+// `AddressCollector`
+func newProvider(ac AddressCollector) *provider {
 	return &provider{
 		instances: newInstancesV2(ac),
 	}