Merge pull request #70967 from mikedanese/fixtropts

Fix broken defaulting and validation in TokenRequest enablement

cmd/kube-apiserver/app/BUILD

@@ -14,7 +14,6 @@ go_library(
         "//pkg/api/legacyscheme:go_default_library",
         "//pkg/capabilities:go_default_library",
         "//pkg/controller/serviceaccount:go_default_library",
-        "//pkg/features:go_default_library",
         "//pkg/generated/openapi:go_default_library",
         "//pkg/kubeapiserver:go_default_library",
         "//pkg/kubeapiserver/admission:go_default_library",

cmd/kube-apiserver/app/options/BUILD

@@ -21,10 +21,12 @@ go_library(
         "//pkg/kubelet/client:go_default_library",
         "//pkg/master/ports:go_default_library",
         "//pkg/master/reconcilers:go_default_library",
+        "//pkg/serviceaccount:go_default_library",
         "//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/server/options:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/storage/storagebackend:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/flag:go_default_library",
         "//staging/src/k8s.io/kube-aggregator/pkg/apiserver/scheme:go_default_library",
     ],

cmd/kube-apiserver/app/options/options.go

@@ -27,13 +27,12 @@ import (
 	"k8s.io/apiserver/pkg/storage/storagebackend"
 	apiserverflag "k8s.io/apiserver/pkg/util/flag"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	_ "k8s.io/kubernetes/pkg/features" // add the kubernetes feature gates
 	kubeoptions "k8s.io/kubernetes/pkg/kubeapiserver/options"
 	kubeletclient "k8s.io/kubernetes/pkg/kubelet/client"
 	"k8s.io/kubernetes/pkg/master/ports"
 	"k8s.io/kubernetes/pkg/master/reconcilers"
-
+	"k8s.io/kubernetes/pkg/serviceaccount"
-	// add the kubernetes feature gates
-	_ "k8s.io/kubernetes/pkg/features"
 )
 
 // ServerRunOptions runs a kubernetes api server.
@@ -71,6 +70,8 @@ type ServerRunOptions struct {
 	EndpointReconcilerType string
 
 	ServiceAccountSigningKeyFile     string
+	ServiceAccountIssuer             serviceaccount.TokenGenerator
+	ServiceAccountTokenMaxExpiration time.Duration
 }
 
 // NewServerRunOptions creates a new ServerRunOptions object with default parameters

cmd/kube-apiserver/app/options/validation.go

@@ -17,74 +17,89 @@ limitations under the License.
 package options
 
 import (
+	"errors"
 	"fmt"
 
 	apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	aggregatorscheme "k8s.io/kube-aggregator/pkg/apiserver/scheme"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // TODO: Longer term we should read this from some config store, rather than a flag.
 func validateClusterIPFlags(options *ServerRunOptions) []error {
-	errors := []error{}
+	var errs []error
+
 	if options.ServiceClusterIPRange.IP == nil {
-		errors = append(errors, fmt.Errorf("no --service-cluster-ip-range specified"))
+		errs = append(errs, errors.New("no --service-cluster-ip-range specified"))
 	}
 	var ones, bits = options.ServiceClusterIPRange.Mask.Size()
 	if bits-ones > 20 {
-		errors = append(errors, fmt.Errorf("specified --service-cluster-ip-range is too large"))
+		errs = append(errs, errors.New("specified --service-cluster-ip-range is too large"))
 	}
-	return errors
+
+	return errs
 }
 
 func validateServiceNodePort(options *ServerRunOptions) []error {
-	errors := []error{}
+	var errs []error
+
 	if options.KubernetesServiceNodePort < 0 || options.KubernetesServiceNodePort > 65535 {
-		errors = append(errors, fmt.Errorf("--kubernetes-service-node-port %v must be between 0 and 65535, inclusive. If 0, the Kubernetes master service will be of type ClusterIP", options.KubernetesServiceNodePort))
+		errs = append(errs, fmt.Errorf("--kubernetes-service-node-port %v must be between 0 and 65535, inclusive. If 0, the Kubernetes master service will be of type ClusterIP", options.KubernetesServiceNodePort))
 	}
 
 	if options.KubernetesServiceNodePort > 0 && !options.ServiceNodePortRange.Contains(options.KubernetesServiceNodePort) {
-		errors = append(errors, fmt.Errorf("kubernetes service port range %v doesn't contain %v", options.ServiceNodePortRange, (options.KubernetesServiceNodePort)))
+		errs = append(errs, fmt.Errorf("kubernetes service port range %v doesn't contain %v", options.ServiceNodePortRange, (options.KubernetesServiceNodePort)))
 	}
-	return errors
+	return errs
 }
 
-// Validate checks ServerRunOptions and return a slice of found errors.
+func validateTokenRequest(options *ServerRunOptions) []error {
+	var errs []error
+
+	enableAttempted := options.ServiceAccountSigningKeyFile != "" ||
+		options.Authentication.ServiceAccounts.Issuer != "" ||
+		len(options.Authentication.APIAudiences) != 0
+
+	enableSucceeded := options.ServiceAccountIssuer != nil
+
+	if enableAttempted && !utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) {
+		errs = append(errs, errors.New("the TokenRequest feature is not enabled but --service-account-signing-key-file, --service-account-issuer and/or --api-audiences flags were passed"))
+	}
+
+	if utilfeature.DefaultFeatureGate.Enabled(features.BoundServiceAccountTokenVolume) && !utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) {
+		errs = append(errs, errors.New("the BoundServiceAccountTokenVolume feature depends on the TokenRequest feature, but the TokenRequest features is not enabled"))
+	}
+
+	if !enableAttempted && utilfeature.DefaultFeatureGate.Enabled(features.BoundServiceAccountTokenVolume) {
+		errs = append(errs, errors.New("--service-account-signing-key-file and --service-account-issuer are required flags"))
+	}
+
+	if enableAttempted && !enableSucceeded {
+		errs = append(errs, errors.New("--service-account-signing-key-file, --service-account-issuer, and --api-audiences should be specified together"))
+	}
+
+	return errs
+}
+
+// Validate checks ServerRunOptions and return a slice of found errs.
 func (s *ServerRunOptions) Validate() []error {
-	var errors []error
+	var errs []error
-	if errs := s.Etcd.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := validateClusterIPFlags(s); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := validateServiceNodePort(s); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.SecureServing.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.Authentication.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.Authorization.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.Audit.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.Admission.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
-	if errs := s.InsecureServing.Validate(); len(errs) > 0 {
-		errors = append(errors, errs...)
-	}
 	if s.MasterCount <= 0 {
-		errors = append(errors, fmt.Errorf("--apiserver-count should be a positive number, but value '%d' provided", s.MasterCount))
+		errs = append(errs, fmt.Errorf("--apiserver-count should be a positive number, but value '%d' provided", s.MasterCount))
-	}
-	if errs := s.APIEnablement.Validate(legacyscheme.Scheme, apiextensionsapiserver.Scheme, aggregatorscheme.Scheme); len(errs) > 0 {
-		errors = append(errors, errs...)
 	}
+	errs = append(errs, s.Etcd.Validate()...)
+	errs = append(errs, validateClusterIPFlags(s)...)
+	errs = append(errs, validateServiceNodePort(s)...)
+	errs = append(errs, s.SecureServing.Validate()...)
+	errs = append(errs, s.Authentication.Validate()...)
+	errs = append(errs, s.Authorization.Validate()...)
+	errs = append(errs, s.Audit.Validate()...)
+	errs = append(errs, s.Admission.Validate()...)
+	errs = append(errs, s.InsecureServing.Validate()...)
+	errs = append(errs, s.APIEnablement.Validate(legacyscheme.Scheme, apiextensionsapiserver.Scheme, aggregatorscheme.Scheme)...)
+	errs = append(errs, validateTokenRequest(s)...)
 
-	return errors
+	return errs
 }

cmd/kube-apiserver/app/server.go

@@ -33,7 +33,6 @@ import (
 
 	"github.com/go-openapi/spec"
 	"github.com/spf13/cobra"
-	"k8s.io/klog"
 
 	extensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -50,13 +49,13 @@ import (
 	serveroptions "k8s.io/apiserver/pkg/server/options"
 	serverstorage "k8s.io/apiserver/pkg/server/storage"
 	"k8s.io/apiserver/pkg/storage/etcd3/preflight"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	apiserverflag "k8s.io/apiserver/pkg/util/flag"
 	"k8s.io/apiserver/pkg/util/webhook"
 	clientgoinformers "k8s.io/client-go/informers"
 	clientgoclientset "k8s.io/client-go/kubernetes"
 	certutil "k8s.io/client-go/util/cert"
 	cloudprovider "k8s.io/cloud-provider"
+	"k8s.io/klog"
 	aggregatorapiserver "k8s.io/kube-aggregator/pkg/apiserver"
 	aggregatorscheme "k8s.io/kube-aggregator/pkg/apiserver/scheme"
 	openapi "k8s.io/kube-openapi/pkg/common"
@@ -64,7 +63,6 @@ import (
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/capabilities"
 	serviceaccountcontroller "k8s.io/kubernetes/pkg/controller/serviceaccount"
-	"k8s.io/kubernetes/pkg/features"
 	generatedopenapi "k8s.io/kubernetes/pkg/generated/openapi"
 	"k8s.io/kubernetes/pkg/kubeapiserver"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
@@ -78,13 +76,12 @@ import (
 	"k8s.io/kubernetes/pkg/registry/cachesize"
 	rbacrest "k8s.io/kubernetes/pkg/registry/rbac/rest"
 	"k8s.io/kubernetes/pkg/serviceaccount"
-	"k8s.io/kubernetes/pkg/version"
-	"k8s.io/kubernetes/pkg/version/verflag"
-	"k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/bootstrap"
-
 	utilflag "k8s.io/kubernetes/pkg/util/flag"
 	_ "k8s.io/kubernetes/pkg/util/reflector/prometheus" // for reflector metric registration
 	_ "k8s.io/kubernetes/pkg/util/workqueue/prometheus" // for workqueue metric registration
+	"k8s.io/kubernetes/pkg/version"
+	"k8s.io/kubernetes/pkg/version/verflag"
+	"k8s.io/kubernetes/plugin/pkg/auth/authenticator/token/bootstrap"
 )
 
 const etcdRetryLimit = 60
@@ -318,50 +315,6 @@ func CreateKubeAPIServerConfig(
 		return
 	}
 
-	var (
-		issuer        serviceaccount.TokenGenerator
-		apiAudiences  []string
-		maxExpiration time.Duration
-	)
-
-	if s.ServiceAccountSigningKeyFile != "" ||
-		s.Authentication.ServiceAccounts.Issuer != "" ||
-		len(s.Authentication.APIAudiences) > 0 {
-		if !utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) {
-			lastErr = fmt.Errorf("the TokenRequest feature is not enabled but --service-account-signing-key-file, --service-account-issuer and/or --service-account-api-audiences flags were passed")
-			return
-		}
-		if s.ServiceAccountSigningKeyFile == "" ||
-			s.Authentication.ServiceAccounts.Issuer == "" ||
-			len(s.Authentication.APIAudiences) == 0 ||
-			len(s.Authentication.ServiceAccounts.KeyFiles) == 0 {
-			lastErr = fmt.Errorf("service-account-signing-key-file, service-account-issuer, service-account-api-audiences and service-account-key-file should be specified together")
-			return
-		}
-		sk, err := certutil.PrivateKeyFromFile(s.ServiceAccountSigningKeyFile)
-		if err != nil {
-			lastErr = fmt.Errorf("failed to parse service-account-issuer-key-file: %v", err)
-			return
-		}
-		if s.Authentication.ServiceAccounts.MaxExpiration != 0 {
-			lowBound := time.Hour
-			upBound := time.Duration(1<<32) * time.Second
-			if s.Authentication.ServiceAccounts.MaxExpiration < lowBound ||
-				s.Authentication.ServiceAccounts.MaxExpiration > upBound {
-				lastErr = fmt.Errorf("the serviceaccount max expiration is out of range, must be between 1 hour to 2^32 seconds")
-				return
-			}
-		}
-
-		issuer, err = serviceaccount.JWTTokenGenerator(s.Authentication.ServiceAccounts.Issuer, sk)
-		if err != nil {
-			lastErr = fmt.Errorf("failed to build token generator: %v", err)
-			return
-		}
-		apiAudiences = s.Authentication.APIAudiences
-		maxExpiration = s.Authentication.ServiceAccounts.MaxExpiration
-	}
-
 	config = &master.Config{
 		GenericConfig: genericConfig,
 		ExtraConfig: master.ExtraConfig{
@@ -393,9 +346,8 @@ func CreateKubeAPIServerConfig(
 			EndpointReconcilerType: reconcilers.Type(s.EndpointReconcilerType),
 			MasterCount:            s.MasterCount,
 
-			ServiceAccountIssuer:        issuer,
+			ServiceAccountIssuer:        s.ServiceAccountIssuer,
-			APIAudiences:                apiAudiences,
+			ServiceAccountMaxExpiration: s.ServiceAccountTokenMaxExpiration,
-			ServiceAccountMaxExpiration: maxExpiration,
 
 			VersionedInformers: versionedInformers,
 		},
@@ -606,6 +558,27 @@ func Complete(s *options.ServerRunOptions) (completedServerRunOptions, error) {
 		}
 	}
 
+	if s.ServiceAccountSigningKeyFile != "" && s.Authentication.ServiceAccounts.Issuer != "" {
+		sk, err := certutil.PrivateKeyFromFile(s.ServiceAccountSigningKeyFile)
+		if err != nil {
+			return options, fmt.Errorf("failed to parse service-account-issuer-key-file: %v", err)
+		}
+		if s.Authentication.ServiceAccounts.MaxExpiration != 0 {
+			lowBound := time.Hour
+			upBound := time.Duration(1<<32) * time.Second
+			if s.Authentication.ServiceAccounts.MaxExpiration < lowBound ||
+				s.Authentication.ServiceAccounts.MaxExpiration > upBound {
+				return options, fmt.Errorf("the serviceaccount max expiration must be between 1 hour to 2^32 seconds")
+			}
+		}
+
+		s.ServiceAccountIssuer, err = serviceaccount.JWTTokenGenerator(s.Authentication.ServiceAccounts.Issuer, sk)
+		if err != nil {
+			return options, fmt.Errorf("failed to build token generator: %v", err)
+		}
+		s.ServiceAccountTokenMaxExpiration = s.Authentication.ServiceAccounts.MaxExpiration
+	}
+
 	if s.Etcd.EnableWatchCache {
 		klog.V(2).Infof("Initializing cache sizes based on %dMB limit", s.GenericServerRunOptions.TargetRAMMB)
 		sizes := cachesize.NewHeuristicWatchCacheSizes(s.GenericServerRunOptions.TargetRAMMB)

pkg/master/BUILD

@@ -104,7 +104,6 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/discovery:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/registry/generic:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/server:go_default_library",

pkg/master/master.go

@@ -58,7 +58,6 @@ import (
 	storageapiv1beta1 "k8s.io/api/storage/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilnet "k8s.io/apimachinery/pkg/util/net"
-	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/endpoints/discovery"
 	"k8s.io/apiserver/pkg/registry/generic"
 	genericapiserver "k8s.io/apiserver/pkg/server"
@@ -169,8 +168,6 @@ type ExtraConfig struct {
 	ServiceAccountIssuer        serviceaccount.TokenGenerator
 	ServiceAccountMaxExpiration time.Duration
 
-	APIAudiences authenticator.Audiences
-
 	VersionedInformers informers.SharedInformerFactory
 }
 
@@ -324,7 +321,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 			LoopbackClientConfig:        c.GenericConfig.LoopbackClientConfig,
 			ServiceAccountIssuer:        c.ExtraConfig.ServiceAccountIssuer,
 			ServiceAccountMaxExpiration: c.ExtraConfig.ServiceAccountMaxExpiration,
-			APIAudiences:                c.ExtraConfig.APIAudiences,
+			APIAudiences:                c.GenericConfig.Authentication.APIAudiences,
 		}
 		m.InstallLegacyAPI(&c, c.GenericConfig.RESTOptionsGetter, legacyRESTStorageProvider)
 	}

test/integration/auth/svcaccttoken_test.go

@@ -89,7 +89,7 @@ func TestServiceAccountTokenCreate(t *testing.T) {
 	}
 	masterConfig.ExtraConfig.ServiceAccountIssuer = tokenGenerator
 	masterConfig.ExtraConfig.ServiceAccountMaxExpiration = maxExpirationDuration
-	masterConfig.ExtraConfig.APIAudiences = aud
+	masterConfig.GenericConfig.Authentication.APIAudiences = aud
 
 	master, _, closeFn := framework.RunAMaster(masterConfig)
 	defer closeFn()