Skip to content

Commit baaa38c

Browse files
richabankerrichabanker
committed
Remove mutation of authn options by binding flag setters to a tracking boolean in options
1 parent 67bdb11 commit baaa38c

File tree

4 files changed

+452
-98
lines changed

4 files changed

+452
-98
lines changed

‎pkg/kubeapiserver/options/authentication.go‎

Lines changed: 26 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,9 @@ type BuiltInAuthenticationOptions struct {
100100

101101
// AnonymousAuthenticationOptions contains anonymous authentication options for API Server
102102
type AnonymousAuthenticationOptions struct {
103-
Allow bool
104-
areFlagsSet func() bool
103+
Allow bool
104+
// FlagsSet tracks whether any of the configuration options were set via a command-line flag.
105+
FlagsSet bool
105106
}
106107

107108
// BootstrapTokenAuthenticationOptions contains bootstrap token authentication options for API Server
@@ -121,8 +122,8 @@ type OIDCAuthenticationOptions struct {
121122
SigningAlgs []string
122123
RequiredClaims map[string]string
123124

124-
// areFlagsConfigured is a function that returns true if any of the oidc-* flags are configured.
125-
areFlagsConfigured func() bool
125+
// FlagsSet tracks whether any of the configuration options were set via a command-line flag.
126+
FlagsSet bool
126127
}
127128

128129
// ServiceAccountAuthenticationOptions contains service account authentication options for API Server
@@ -183,8 +184,7 @@ func (o *BuiltInAuthenticationOptions) WithAll() *BuiltInAuthenticationOptions {
183184
// WithAnonymous set default value for anonymous authentication
184185
func (o *BuiltInAuthenticationOptions) WithAnonymous() *BuiltInAuthenticationOptions {
185186
o.Anonymous = &AnonymousAuthenticationOptions{
186-
Allow: true,
187-
areFlagsSet: func() bool { return false },
187+
Allow: true,
188188
}
189189
return o
190190
}
@@ -204,9 +204,8 @@ func (o *BuiltInAuthenticationOptions) WithClientCert() *BuiltInAuthenticationOp
204204
// WithOIDC set default value for OIDC authentication
205205
func (o *BuiltInAuthenticationOptions) WithOIDC() *BuiltInAuthenticationOptions {
206206
o.OIDC = &OIDCAuthenticationOptions{
207-
areFlagsConfigured: func() bool { return false },
208-
UsernameClaim: "sub",
209-
SigningAlgs: []string{"RS256"},
207+
UsernameClaim: "sub",
208+
SigningAlgs: []string{"RS256"},
210209
}
211210
return o
212211
}
@@ -337,10 +336,7 @@ func (o *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {
337336
"Enables anonymous requests to the secure port of the API server. "+
338337
"Requests that are not rejected by another authentication method are treated as anonymous requests. "+
339338
"Anonymous requests have a username of system:anonymous, and a group name of system:unauthenticated.")
340-
341-
o.Anonymous.areFlagsSet = func() bool {
342-
return fs.Changed("anonymous-auth")
343-
}
339+
trackProvidedFlag(fs, "anonymous-auth", &o.Anonymous.FlagsSet)
344340
}
345341

346342
if o.BootstrapToken != nil {
@@ -357,54 +353,51 @@ func (o *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {
357353
fs.StringVar(&o.OIDC.IssuerURL, oidcIssuerURLFlag, o.OIDC.IssuerURL, ""+
358354
"The URL of the OpenID issuer, only HTTPS scheme will be accepted. "+
359355
"If set, it will be used to verify the OIDC JSON Web Token (JWT).")
356+
trackProvidedFlag(fs, oidcIssuerURLFlag, &o.OIDC.FlagsSet)
360357

361358
fs.StringVar(&o.OIDC.ClientID, oidcClientIDFlag, o.OIDC.ClientID, ""+
362359
"The client ID for the OpenID Connect client, must be set if oidc-issuer-url is set.")
360+
trackProvidedFlag(fs, oidcClientIDFlag, &o.OIDC.FlagsSet)
363361

364362
fs.StringVar(&o.OIDC.CAFile, oidcCAFileFlag, o.OIDC.CAFile, ""+
365363
"If set, the OpenID server's certificate will be verified by one of the authorities "+
366364
"in the oidc-ca-file, otherwise the host's root CA set will be used.")
365+
trackProvidedFlag(fs, oidcCAFileFlag, &o.OIDC.FlagsSet)
367366

368367
fs.StringVar(&o.OIDC.UsernameClaim, oidcUsernameClaimFlag, o.OIDC.UsernameClaim, ""+
369368
"The OpenID claim to use as the user name. Note that claims other than the default ('sub') "+
370369
"is not guaranteed to be unique and immutable. This flag is experimental, please see "+
371370
"the authentication documentation for further details.")
371+
trackProvidedFlag(fs, oidcUsernameClaimFlag, &o.OIDC.FlagsSet)
372372

373373
fs.StringVar(&o.OIDC.UsernamePrefix, oidcUsernamePrefixFlag, o.OIDC.UsernamePrefix, ""+
374374
"If provided, all usernames will be prefixed with this value. If not provided, "+
375375
"username claims other than 'email' are prefixed by the issuer URL to avoid "+
376376
"clashes. To skip any prefixing, provide the value '-'.")
377+
trackProvidedFlag(fs, oidcUsernamePrefixFlag, &o.OIDC.FlagsSet)
377378

378379
fs.StringVar(&o.OIDC.GroupsClaim, oidcGroupsClaimFlag, o.OIDC.GroupsClaim, ""+
379380
"If provided, the name of a custom OpenID Connect claim for specifying user groups. "+
380381
"The claim value is expected to be a string or array of strings. This flag is experimental, "+
381382
"please see the authentication documentation for further details.")
383+
trackProvidedFlag(fs, oidcGroupsClaimFlag, &o.OIDC.FlagsSet)
382384

383385
fs.StringVar(&o.OIDC.GroupsPrefix, oidcGroupsPrefixFlag, o.OIDC.GroupsPrefix, ""+
384386
"If provided, all groups will be prefixed with this value to prevent conflicts with "+
385387
"other authentication strategies.")
388+
trackProvidedFlag(fs, oidcGroupsPrefixFlag, &o.OIDC.FlagsSet)
386389

387390
fs.StringSliceVar(&o.OIDC.SigningAlgs, oidcSigningAlgsFlag, o.OIDC.SigningAlgs, ""+
388391
"Comma-separated list of allowed JOSE asymmetric signing algorithms. JWTs with a "+
389392
"supported 'alg' header values are: RS256, RS384, RS512, ES256, ES384, ES512, PS256, PS384, PS512. "+
390393
"Values are defined by RFC 7518 https://tools.ietf.org/html/rfc7518#section-3.1.")
394+
trackProvidedFlag(fs, oidcSigningAlgsFlag, &o.OIDC.FlagsSet)
391395

392396
fs.Var(cliflag.NewMapStringStringNoSplit(&o.OIDC.RequiredClaims), oidcRequiredClaimFlag, ""+
393397
"A key=value pair that describes a required claim in the ID Token. "+
394398
"If set, the claim is verified to be present in the ID Token with a matching value. "+
395399
"Repeat this flag to specify multiple claims.")
396-
397-
o.OIDC.areFlagsConfigured = func() bool {
398-
return fs.Changed(oidcIssuerURLFlag) ||
399-
fs.Changed(oidcClientIDFlag) ||
400-
fs.Changed(oidcCAFileFlag) ||
401-
fs.Changed(oidcUsernameClaimFlag) ||
402-
fs.Changed(oidcUsernamePrefixFlag) ||
403-
fs.Changed(oidcGroupsClaimFlag) ||
404-
fs.Changed(oidcGroupsPrefixFlag) ||
405-
fs.Changed(oidcSigningAlgsFlag) ||
406-
fs.Changed(oidcRequiredClaimFlag)
407-
}
400+
trackProvidedFlag(fs, oidcRequiredClaimFlag, &o.OIDC.FlagsSet)
408401
}
409402

410403
if o.RequestHeader != nil {
@@ -572,7 +565,7 @@ func (o *BuiltInAuthenticationOptions) ToAuthenticationConfig() (kubeauthenticat
572565
// Set up anonymous authenticator from config file or flags
573566
if o.Anonymous != nil {
574567
switch {
575-
case ret.AuthenticationConfig.Anonymous != nil && o.Anonymous.areFlagsSet():
568+
case ret.AuthenticationConfig.Anonymous != nil && o.Anonymous.FlagsSet:
576569
// Flags and config file are mutually exclusive
577570
return kubeauthenticator.Config{}, field.Forbidden(field.NewPath("anonymous"), "--anonynous-auth flag cannot be set when anonymous field is configured in authentication configuration file")
578571
case ret.AuthenticationConfig.Anonymous != nil:
@@ -823,12 +816,17 @@ func (o *BuiltInAuthenticationOptions) ApplyAuthorization(authorization *BuiltIn
823816
}
824817
}
825818

819+
func trackProvidedFlag(fs *pflag.FlagSet, flagName string, provided *bool) {
820+
f := fs.Lookup(flagName)
821+
f.Value = cliflag.NewTracker(f.Value, provided)
822+
}
823+
826824
func (o *BuiltInAuthenticationOptions) validateOIDCOptions() []error {
827825
var allErrors []error
828826

829827
// Existing validation when jwt authenticator is configured with oidc-* flags
830828
if len(o.AuthenticationConfigFile) == 0 {
831-
if o.OIDC != nil && o.OIDC.areFlagsConfigured() && (len(o.OIDC.IssuerURL) == 0 || len(o.OIDC.ClientID) == 0) {
829+
if o.OIDC != nil && o.OIDC.FlagsSet && (len(o.OIDC.IssuerURL) == 0 || len(o.OIDC.ClientID) == 0) {
832830
allErrors = append(allErrors, fmt.Errorf("oidc-issuer-url and oidc-client-id must be specified together when any oidc-* flags are set"))
833831
}
834832

@@ -843,7 +841,7 @@ func (o *BuiltInAuthenticationOptions) validateOIDCOptions() []error {
843841
}
844842

845843
// Authentication config file and oidc-* flags are mutually exclusive
846-
if o.OIDC != nil && o.OIDC.areFlagsConfigured() {
844+
if o.OIDC != nil && o.OIDC.FlagsSet {
847845
allErrors = append(allErrors, fmt.Errorf("authentication-config file and oidc-* flags are mutually exclusive"))
848846
}
849847

‎pkg/kubeapiserver/options/authentication_test.go‎

Lines changed: 59 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,11 @@ func TestAuthenticationValidate(t *testing.T) {
7171
{
7272
name: "test when OIDC and ServiceAccounts are valid",
7373
testOIDC: &OIDCAuthenticationOptions{
74-
UsernameClaim: "sub",
75-
SigningAlgs: []string{"RS256"},
76-
IssuerURL: "https://testIssuerURL",
77-
ClientID: "testClientID",
78-
areFlagsConfigured: func() bool { return true },
74+
UsernameClaim: "sub",
75+
SigningAlgs: []string{"RS256"},
76+
IssuerURL: "https://testIssuerURL",
77+
ClientID: "testClientID",
78+
FlagsSet: true,
7979
},
8080
testSA: &ServiceAccountAuthenticationOptions{
8181
Issuers: []string{"http://foo.bar.com"},
@@ -85,10 +85,10 @@ func TestAuthenticationValidate(t *testing.T) {
8585
{
8686
name: "test when OIDC is invalid",
8787
testOIDC: &OIDCAuthenticationOptions{
88-
UsernameClaim: "sub",
89-
SigningAlgs: []string{"RS256"},
90-
IssuerURL: "https://testIssuerURL",
91-
areFlagsConfigured: func() bool { return true },
88+
UsernameClaim: "sub",
89+
SigningAlgs: []string{"RS256"},
90+
IssuerURL: "https://testIssuerURL",
91+
FlagsSet: true,
9292
},
9393
testSA: &ServiceAccountAuthenticationOptions{
9494
Issuers: []string{"http://foo.bar.com"},
@@ -99,11 +99,11 @@ func TestAuthenticationValidate(t *testing.T) {
9999
{
100100
name: "test when ServiceAccounts doesn't have key file",
101101
testOIDC: &OIDCAuthenticationOptions{
102-
UsernameClaim: "sub",
103-
SigningAlgs: []string{"RS256"},
104-
IssuerURL: "https://testIssuerURL",
105-
ClientID: "testClientID",
106-
areFlagsConfigured: func() bool { return true },
102+
UsernameClaim: "sub",
103+
SigningAlgs: []string{"RS256"},
104+
IssuerURL: "https://testIssuerURL",
105+
ClientID: "testClientID",
106+
FlagsSet: true,
107107
},
108108
testSA: &ServiceAccountAuthenticationOptions{
109109
Issuers: []string{"http://foo.bar.com"},
@@ -113,11 +113,11 @@ func TestAuthenticationValidate(t *testing.T) {
113113
{
114114
name: "test when ServiceAccounts doesn't have issuer",
115115
testOIDC: &OIDCAuthenticationOptions{
116-
UsernameClaim: "sub",
117-
SigningAlgs: []string{"RS256"},
118-
IssuerURL: "https://testIssuerURL",
119-
ClientID: "testClientID",
120-
areFlagsConfigured: func() bool { return true },
116+
UsernameClaim: "sub",
117+
SigningAlgs: []string{"RS256"},
118+
IssuerURL: "https://testIssuerURL",
119+
ClientID: "testClientID",
120+
FlagsSet: true,
121121
},
122122
testSA: &ServiceAccountAuthenticationOptions{
123123
Issuers: []string{},
@@ -127,11 +127,11 @@ func TestAuthenticationValidate(t *testing.T) {
127127
{
128128
name: "test when ServiceAccounts has empty string as issuer",
129129
testOIDC: &OIDCAuthenticationOptions{
130-
UsernameClaim: "sub",
131-
SigningAlgs: []string{"RS256"},
132-
IssuerURL: "https://testIssuerURL",
133-
ClientID: "testClientID",
134-
areFlagsConfigured: func() bool { return true },
130+
UsernameClaim: "sub",
131+
SigningAlgs: []string{"RS256"},
132+
IssuerURL: "https://testIssuerURL",
133+
ClientID: "testClientID",
134+
FlagsSet: true,
135135
},
136136
testSA: &ServiceAccountAuthenticationOptions{
137137
Issuers: []string{""},
@@ -141,11 +141,11 @@ func TestAuthenticationValidate(t *testing.T) {
141141
{
142142
name: "test when ServiceAccounts has duplicate issuers",
143143
testOIDC: &OIDCAuthenticationOptions{
144-
UsernameClaim: "sub",
145-
SigningAlgs: []string{"RS256"},
146-
IssuerURL: "https://testIssuerURL",
147-
ClientID: "testClientID",
148-
areFlagsConfigured: func() bool { return true },
144+
UsernameClaim: "sub",
145+
SigningAlgs: []string{"RS256"},
146+
IssuerURL: "https://testIssuerURL",
147+
ClientID: "testClientID",
148+
FlagsSet: true,
149149
},
150150
testSA: &ServiceAccountAuthenticationOptions{
151151
Issuers: []string{"http://foo.bar.com", "http://foo.bar.com"},
@@ -155,11 +155,11 @@ func TestAuthenticationValidate(t *testing.T) {
155155
{
156156
name: "test when ServiceAccount has bad issuer",
157157
testOIDC: &OIDCAuthenticationOptions{
158-
UsernameClaim: "sub",
159-
SigningAlgs: []string{"RS256"},
160-
IssuerURL: "https://testIssuerURL",
161-
ClientID: "testClientID",
162-
areFlagsConfigured: func() bool { return true },
158+
UsernameClaim: "sub",
159+
SigningAlgs: []string{"RS256"},
160+
IssuerURL: "https://testIssuerURL",
161+
ClientID: "testClientID",
162+
FlagsSet: true,
163163
},
164164
testSA: &ServiceAccountAuthenticationOptions{
165165
Issuers: []string{"http://[::1]:namedport"},
@@ -169,11 +169,11 @@ func TestAuthenticationValidate(t *testing.T) {
169169
{
170170
name: "test when ServiceAccounts has invalid JWKSURI",
171171
testOIDC: &OIDCAuthenticationOptions{
172-
UsernameClaim: "sub",
173-
SigningAlgs: []string{"RS256"},
174-
IssuerURL: "https://testIssuerURL",
175-
ClientID: "testClientID",
176-
areFlagsConfigured: func() bool { return true },
172+
UsernameClaim: "sub",
173+
SigningAlgs: []string{"RS256"},
174+
IssuerURL: "https://testIssuerURL",
175+
ClientID: "testClientID",
176+
FlagsSet: true,
177177
},
178178
testSA: &ServiceAccountAuthenticationOptions{
179179
KeyFiles: []string{"cert", "key"},
@@ -185,11 +185,11 @@ func TestAuthenticationValidate(t *testing.T) {
185185
{
186186
name: "test when ServiceAccounts has invalid JWKSURI (not https scheme)",
187187
testOIDC: &OIDCAuthenticationOptions{
188-
UsernameClaim: "sub",
189-
SigningAlgs: []string{"RS256"},
190-
IssuerURL: "https://testIssuerURL",
191-
ClientID: "testClientID",
192-
areFlagsConfigured: func() bool { return true },
188+
UsernameClaim: "sub",
189+
SigningAlgs: []string{"RS256"},
190+
IssuerURL: "https://testIssuerURL",
191+
ClientID: "testClientID",
192+
FlagsSet: true,
193193
},
194194
testSA: &ServiceAccountAuthenticationOptions{
195195
KeyFiles: []string{"cert", "key"},
@@ -201,11 +201,11 @@ func TestAuthenticationValidate(t *testing.T) {
201201
{
202202
name: "test when WebHook has invalid retry attempts",
203203
testOIDC: &OIDCAuthenticationOptions{
204-
UsernameClaim: "sub",
205-
SigningAlgs: []string{"RS256"},
206-
IssuerURL: "https://testIssuerURL",
207-
ClientID: "testClientID",
208-
areFlagsConfigured: func() bool { return true },
204+
UsernameClaim: "sub",
205+
SigningAlgs: []string{"RS256"},
206+
IssuerURL: "https://testIssuerURL",
207+
ClientID: "testClientID",
208+
FlagsSet: true,
209209
},
210210
testSA: &ServiceAccountAuthenticationOptions{
211211
KeyFiles: []string{"cert", "key"},
@@ -234,11 +234,11 @@ func TestAuthenticationValidate(t *testing.T) {
234234
name: "test when authentication config file and oidc-* flags are set",
235235
testAuthenticationConfigFile: "configfile",
236236
testOIDC: &OIDCAuthenticationOptions{
237-
UsernameClaim: "sub",
238-
SigningAlgs: []string{"RS256"},
239-
IssuerURL: "https://testIssuerURL",
240-
ClientID: "testClientID",
241-
areFlagsConfigured: func() bool { return true },
237+
UsernameClaim: "sub",
238+
SigningAlgs: []string{"RS256"},
239+
IssuerURL: "https://testIssuerURL",
240+
ClientID: "testClientID",
241+
FlagsSet: true,
242242
},
243243
expectErr: "authentication-config file and oidc-* flags are mutually exclusive",
244244
},
@@ -247,8 +247,8 @@ func TestAuthenticationValidate(t *testing.T) {
247247
disabledFeatures: []featuregate.Feature{features.AnonymousAuthConfigurableEndpoints},
248248
testAuthenticationConfigFile: "configfile",
249249
testAnonymous: &AnonymousAuthenticationOptions{
250-
Allow: true,
251-
areFlagsSet: func() bool { return true },
250+
Allow: true,
251+
FlagsSet: true,
252252
},
253253
},
254254
}
@@ -413,7 +413,8 @@ func TestBuiltInAuthenticationOptionsAddFlags(t *testing.T) {
413413
expected := &BuiltInAuthenticationOptions{
414414
APIAudiences: []string{"foo"},
415415
Anonymous: &AnonymousAuthenticationOptions{
416-
Allow: true,
416+
Allow: true,
417+
FlagsSet: true,
417418
},
418419
BootstrapToken: &BootstrapTokenAuthenticationOptions{
419420
Enable: true,
@@ -428,6 +429,7 @@ func TestBuiltInAuthenticationOptionsAddFlags(t *testing.T) {
428429
UsernameClaim: "sub",
429430
UsernamePrefix: "-",
430431
SigningAlgs: []string{"RS256"},
432+
FlagsSet: true,
431433
},
432434
RequestHeader: &apiserveroptions.RequestHeaderAuthenticationOptions{
433435
ClientCAFile: "testdata/root.pem",
@@ -470,19 +472,6 @@ func TestBuiltInAuthenticationOptionsAddFlags(t *testing.T) {
470472
t.Fatal(err)
471473
}
472474

473-
if !opts.OIDC.areFlagsConfigured() {
474-
t.Fatal("OIDC flags should be configured")
475-
}
476-
// nil these out because you cannot compare functions
477-
opts.OIDC.areFlagsConfigured = nil
478-
479-
if !opts.Anonymous.areFlagsSet() {
480-
t.Fatalf("Anonymous flags should be configured")
481-
}
482-
483-
// nil these out because you cannot compare functions
484-
opts.Anonymous.areFlagsSet = nil
485-
486475
if !reflect.DeepEqual(opts, expected) {
487476
t.Error(cmp.Diff(opts, expected, cmp.AllowUnexported(OIDCAuthenticationOptions{}, AnonymousAuthenticationOptions{})))
488477
}

0 commit comments

Comments
 (0)