Merge pull request #23661 from spxtr/oidc

Remove retries from most oidc tests.

pkg/apiserver/authenticator/authn.go

@@ -138,7 +138,15 @@ func newAuthenticatorFromTokenFile(tokenAuthFile string) (authenticator.Request,
 
 // newAuthenticatorFromOIDCIssuerURL returns an authenticator.Request or an error.
 func newAuthenticatorFromOIDCIssuerURL(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (authenticator.Request, error) {
-	tokenAuthenticator, err := oidc.New(issuerURL, clientID, caFile, usernameClaim, groupsClaim)
+	tokenAuthenticator, err := oidc.New(oidc.OIDCOptions{
+		IssuerURL:     issuerURL,
+		ClientID:      clientID,
+		CAFile:        caFile,
+		UsernameClaim: usernameClaim,
+		GroupsClaim:   groupsClaim,
+		MaxRetries:    oidc.DefaultRetries,
+		RetryBackoff:  oidc.DefaultBackoff,
+	})
 	if err != nil {
 		return nil, err
 	}

plugin/pkg/auth/authenticator/token/oidc/oidc.go

@@ -34,38 +34,52 @@ import (
 	"k8s.io/kubernetes/pkg/util/net"
 )
 
-var (
+const (
-	maxRetries   = 5
+	DefaultRetries = 5
-	retryBackoff = time.Second * 3
+	DefaultBackoff = time.Second * 3
 )
 
+type OIDCOptions struct {
+	IssuerURL     string
+	ClientID      string
+	CAFile        string
+	UsernameClaim string
+	GroupsClaim   string
+
+	// 0 disables retry
+	MaxRetries   int
+	RetryBackoff time.Duration
+}
+
 type OIDCAuthenticator struct {
 	clientConfig     oidc.ClientConfig
 	client           *oidc.Client
 	usernameClaim    string
 	groupsClaim      string
 	stopSyncProvider chan struct{}
+	maxRetries       int
+	retryBackoff     time.Duration
 }
 
 // New creates a new OpenID Connect client with the given issuerURL and clientID.
 // NOTE(yifan): For now we assume the server provides the "jwks_uri" so we don't
 // need to manager the key sets by ourselves.
-func New(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (*OIDCAuthenticator, error) {
+func New(opts OIDCOptions) (*OIDCAuthenticator, error) {
 	var cfg oidc.ProviderConfig
 	var err error
 	var roots *x509.CertPool
 
-	url, err := url.Parse(issuerURL)
+	url, err := url.Parse(opts.IssuerURL)
 	if err != nil {
 		return nil, err
 	}
 
 	if url.Scheme != "https" {
-		return nil, fmt.Errorf("'oidc-issuer-url' (%q) has invalid scheme (%q), require 'https'", issuerURL, url.Scheme)
+		return nil, fmt.Errorf("'oidc-issuer-url' (%q) has invalid scheme (%q), require 'https'", opts.IssuerURL, url.Scheme)
 	}
 
-	if caFile != "" {
+	if opts.CAFile != "" {
-		roots, err = crypto.CertPoolFromFile(caFile)
+		roots, err = crypto.CertPoolFromFile(opts.CAFile)
 		if err != nil {
 			glog.Errorf("Failed to read the CA file: %v", err)
 		}
@@ -84,12 +98,21 @@ func New(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (*OIDCA
 	hc := &http.Client{}
 	hc.Transport = tr
 
+	maxRetries := opts.MaxRetries
+	if maxRetries < 0 {
+		maxRetries = DefaultRetries
+	}
+	retryBackoff := opts.RetryBackoff
+	if retryBackoff < 0 {
+		retryBackoff = DefaultBackoff
+	}
+
 	for i := 0; i <= maxRetries; i++ {
 		if i == maxRetries {
 			return nil, fmt.Errorf("failed to fetch provider config after %v retries", maxRetries)
 		}
 
-		cfg, err = oidc.FetchProviderConfig(hc, strings.TrimSuffix(issuerURL, "/"))
+		cfg, err = oidc.FetchProviderConfig(hc, strings.TrimSuffix(opts.IssuerURL, "/"))
 		if err == nil {
 			break
 		}
@@ -97,11 +120,11 @@ func New(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (*OIDCA
 		time.Sleep(retryBackoff)
 	}
 
-	glog.Infof("Fetched provider config from %s: %#v", issuerURL, cfg)
+	glog.Infof("Fetched provider config from %s: %#v", opts.IssuerURL, cfg)
 
 	ccfg := oidc.ClientConfig{
 		HTTPClient:     hc,
-		Credentials:    oidc.ClientCredentials{ID: clientID},
+		Credentials:    oidc.ClientCredentials{ID: opts.ClientID},
 		ProviderConfig: cfg,
 	}
 
@@ -113,9 +136,17 @@ func New(issuerURL, clientID, caFile, usernameClaim, groupsClaim string) (*OIDCA
 	// SyncProviderConfig will start a goroutine to periodically synchronize the provider config.
 	// The synchronization interval is set by the expiration length of the config, and has a mininum
 	// and maximum threshold.
-	stop := client.SyncProviderConfig(issuerURL)
+	stop := client.SyncProviderConfig(opts.IssuerURL)
 
-	return &OIDCAuthenticator{ccfg, client, usernameClaim, groupsClaim, stop}, nil
+	return &OIDCAuthenticator{
+		ccfg,
+		client,
+		opts.UsernameClaim,
+		opts.GroupsClaim,
+		stop,
+		maxRetries,
+		retryBackoff,
+	}, nil
 }
 
 // AuthenticateToken decodes and verifies a JWT using the OIDC client, if the verification succeeds,

plugin/pkg/auth/authenticator/token/oidc/oidc_test.go

@@ -200,11 +200,8 @@ func generateSelfSignedCert(t *testing.T, host, certPath, keyPath string) {
 }
 
 func TestOIDCDiscoveryTimeout(t *testing.T) {
-	maxRetries = 3
+	expectErr := fmt.Errorf("failed to fetch provider config after 1 retries")
-	retryBackoff = time.Second
+	_, err := New(OIDCOptions{"https://127.0.0.1:9999/bar", "client-foo", "", "sub", "", 1, 100 * time.Millisecond})
-	expectErr := fmt.Errorf("failed to fetch provider config after 3 retries")
-
-	_, err := New("https://foo/bar", "client-foo", "", "sub", "")
 	if !reflect.DeepEqual(err, expectErr) {
 		t.Errorf("Expecting %v, but got %v", expectErr, err)
 	}
@@ -212,7 +209,7 @@ func TestOIDCDiscoveryTimeout(t *testing.T) {
 
 func TestOIDCDiscoveryNoKeyEndpoint(t *testing.T) {
 	var err error
-	expectErr := fmt.Errorf("failed to fetch provider config after 3 retries")
+	expectErr := fmt.Errorf("failed to fetch provider config after 0 retries")
 
 	cert := path.Join(os.TempDir(), "oidc-cert")
 	key := path.Join(os.TempDir(), "oidc-key")
@@ -237,16 +234,13 @@ func TestOIDCDiscoveryNoKeyEndpoint(t *testing.T) {
 		Issuer: mustParseURL(t, srv.URL), // An invalid ProviderConfig. Keys endpoint is required.
 	}
 
-	_, err = New(srv.URL, "client-foo", cert, "sub", "")
+	_, err = New(OIDCOptions{srv.URL, "client-foo", cert, "sub", "", 0, 0})
 	if !reflect.DeepEqual(err, expectErr) {
 		t.Errorf("Expecting %v, but got %v", expectErr, err)
 	}
 }
 
 func TestOIDCDiscoverySecureConnection(t *testing.T) {
-	maxRetries = 3
-	retryBackoff = time.Second
-
 	// Verify that plain HTTP issuer URL is forbidden.
 	op := newOIDCProvider(t)
 	srv := httptest.NewServer(op.mux)
@@ -260,7 +254,7 @@ func TestOIDCDiscoverySecureConnection(t *testing.T) {
 
 	expectErr := fmt.Errorf("'oidc-issuer-url' (%q) has invalid scheme (%q), require 'https'", srv.URL, "http")
 
-	_, err := New(srv.URL, "client-foo", "", "sub", "")
+	_, err := New(OIDCOptions{srv.URL, "client-foo", "", "sub", "", 0, 0})
 	if !reflect.DeepEqual(err, expectErr) {
 		t.Errorf("Expecting %v, but got %v", expectErr, err)
 	}
@@ -296,7 +290,7 @@ func TestOIDCDiscoverySecureConnection(t *testing.T) {
 	}
 
 	// Create a client using cert2, should fail.
-	_, err = New(tlsSrv.URL, "client-foo", cert2, "sub", "")
+	_, err = New(OIDCOptions{tlsSrv.URL, "client-foo", cert2, "sub", "", 0, 0})
 	if err == nil {
 		t.Fatalf("Expecting error, but got nothing")
 	}
@@ -417,7 +411,7 @@ func TestOIDCAuthentication(t *testing.T) {
 	}
 
 	for i, tt := range tests {
-		client, err := New(srv.URL, "client-foo", cert, tt.userClaim, tt.groupsClaim)
+		client, err := New(OIDCOptions{srv.URL, "client-foo", cert, tt.userClaim, tt.groupsClaim, 1, 100 * time.Millisecond})
 		if err != nil {
 			t.Errorf("Unexpected error: %v", err)
 			continue