chore(coderd/coderdtest/oidctest): protect mutable fields with rwmutex #17151
Conversation
|
OK, I changed this around |
coderd/coderdtest/oidctest/idp.go
Outdated
| @@ -110,8 +203,8 @@ type FakeIDP struct { | |||
| // some claims. | |||
| defaultIDClaims jwt.MapClaims | |||
| hookMutateToken func(token map[string]interface{}) | |||
| fakeCoderd func(req *http.Request) (*http.Response, error) | |||
| hookOnRefresh func(email string) error | |||
| // fakeCoderd func(req *http.Request) (*http.Response, error) | |||
There was a problem hiding this comment.
moved to protected
| // fakeCoderd func(req *http.Request) (*http.Response, error) |
coderd/coderdtest/oidctest/idp.go
Outdated
| provider ProviderJSON | ||
| handler http.Handler | ||
| cfg *oauth2.Config | ||
| prot fakeIDPProtected |
There was a problem hiding this comment.
Suggestion: Maybe rename to priv/private, internal, etc? Prot makes me think of protoc/prototypes.
coderd/coderdtest/oidctest/idp.go
Outdated
| if cfg == nil { | ||
| cfg = &oauth2.Config{} | ||
| } | ||
| fn(cfg) |
There was a problem hiding this comment.
Nit: This mutates the config which is a pointer, meaning the mutex only protects f.cfg but not the actual &oauth2.Config{} which could be modified and read in a racy way? I'd suggest at least making a copy of it, but did not verify if that's sufficient.
There was a problem hiding this comment.
If we take your suggestion above then we can just mutate f.cfg directly.
There was a problem hiding this comment.
If we're worried about races and re-entrant locking, couldn't we just do this?
f.mu.Lock()
if f.cfg == nil {
f.cfg = &oauth2.Config{}
}
cfg := *f.cfg
f.mu.Unlock()
fn(cfg)
f.mu.Lock()
f.cfg = &cfg
f.mu.Unlock()There was a problem hiding this comment.
When I make a shallow copy of f.cfg it causes TestOIDCSkipIssuer to fail, most likely due to some missing unexported fields. We may sadly have to live with the possibility of re-entrant locks.
Fixes a test flake seen here: https://github.com/coder/coder/actions/runs/14129446248/job/39585915325