- Notifications
You must be signed in to change notification settings - Fork1.1k
chore(coderd/coderdtest/oidctest): protect mutable fields with rwmutex#17151
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mafredri left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for working on this!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
johnstcn commentedMar 28, 2025
OK, I changed this around |
coderd/coderdtest/oidctest/idp.go Outdated
| hookMutateTokenfunc(tokenmap[string]interface{}) | ||
| fakeCoderdfunc(req*http.Request) (*http.Response,error) | ||
| hookOnRefreshfunc(emailstring)error | ||
| //fakeCoderd func(req *http.Request) (*http.Response, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
moved to protected
| // fakeCoderd func(req *http.Request) (*http.Response, error) |
mafredri left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few caveats, but probably don't need to re-review.
coderd/coderdtest/oidctest/idp.go Outdated
| providerProviderJSON | ||
| handler http.Handler | ||
| cfg*oauth2.Config | ||
| protfakeIDPProtected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: Maybe rename to priv/private, internal, etc? Prot makes me think of protoc/prototypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Renamed tolocked. 👍
Uh oh!
There was an error while loading.Please reload this page.
coderd/coderdtest/oidctest/idp.go Outdated
| ifcfg==nil { | ||
| cfg=&oauth2.Config{} | ||
| } | ||
| fn(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: This mutates the config which is a pointer, meaning the mutex only protectsf.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we take your suggestion above then we can just mutatef.cfg directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we're worried about races and re-entrant locking, couldn't we just do this?
f.mu.Lock()iff.cfg==nil {f.cfg=&oauth2.Config{}}cfg:=*f.cfgf.mu.Unlock()fn(cfg)f.mu.Lock()f.cfg=&cfgf.mu.Unlock()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When I make a shallow copy off.cfg it causesTestOIDCSkipIssuer to fail, most likely due to some missing unexported fields. We may sadly have to live with the possibility of re-entrant locks.
8cecc4f intomainUh oh!
There was an error while loading.Please reload this page.
Fixes a test flake seen here:https://github.com/coder/coder/actions/runs/14129446248/job/39585915325