Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

auth: Add Close() for OIDC authenticator. #18917

Merged
merged 1 commit into from Dec 23, 2015
Merged

auth: Add Close() for OIDC authenticator. #18917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 15 additions & 5 deletions plugin/pkg/auth/authenticator/token/oidc/oidc.go
View file
Open in desktop
Expand Up @@ -38,9 +38,10 @@ var (
)

type OIDCAuthenticator struct {
clientConfig oidc.ClientConfig
client *oidc.Client
usernameClaim string
clientConfig oidc.ClientConfig
client *oidc.Client
usernameClaim string
stopSyncProvider chan struct{}
}

// New creates a new OpenID Connect client with the given issuerURL and clientID.
Expand Down Expand Up @@ -113,9 +114,9 @@ func New(issuerURL, clientID, caFile, usernameClaim string) (*OIDCAuthenticator,
// 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.
client.SyncProviderConfig(issuerURL)
stop := client.SyncProviderConfig(issuerURL)

return &OIDCAuthenticator{ccfg, client, usernameClaim}, nil
return &OIDCAuthenticator{ccfg, client, usernameClaim, stop}, nil
}

// AuthenticateToken decodes and verifies a JWT using the OIDC client, if the verification succeeds,
Expand Down Expand Up @@ -156,3 +157,12 @@ func (a *OIDCAuthenticator) AuthenticateToken(value string) (user.Info, bool, er
// TODO(yifan): Add UID and Group, also populate the issuer to upper layer.
return &user.DefaultInfo{Name: username}, true, nil
}

// Close closes the OIDC authenticator, this will close the provider sync goroutine.
func (a *OIDCAuthenticator) Close() {
// This assumes the s.stopSyncProvider is an unbuffered channel.
// So instead of closing the channel, we send am empty struct here.
// This guarantees that when this function returns, there is no flying requests,
// because a send to an unbuffered channel happens after the receive from the channel.
a.stopSyncProvider <- struct{}{}
}
1 change: 1 addition & 0 deletions plugin/pkg/auth/authenticator/token/oidc/oidc_test.go
View file
Open in desktop
Expand Up @@ -391,5 +391,6 @@ func TestOIDCAuthentication(t *testing.T) {
if !reflect.DeepEqual(tt.userInfo, user) {
t.Errorf("#%d: Expecting: %v, but got: %v", i, tt.userInfo, user)
}
client.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this code, but IIUC we're not waiting for client to actually close here. By closing the channel we just notify it that it should, but I don't see how it would prevent the race.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmarek You are right. Let me replace the close with sending struct{}{}, as the channel is an unbuffered channel, sending to that channel happens after the channel is received, in which case it should guarantee there is not flying requests then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}
}