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

[FIXED] Always update account issuer in accountsz #5886

Merged
merged 4 commits into from
Sep 13, 2024
Merged

[FIXED] Always update account issuer in accountsz #5886

merged 4 commits into from
Sep 13, 2024

Conversation

aricart
Copy link
Member

@aricart aricart commented Sep 13, 2024

changed to update the account issuer for an account to always update otherwise the information shown in accountz will not be consistent with the decoded JWT

@aricart
changed to update the account issuer for an account to always update …
dfa6bd1 
…otherwise the information shown in accountz will not be consistent with the decoded JWT
@aricart aricart requested a review from a team as a code owner September 13, 2024 15:44
@neilalexander
Copy link
Member

Has seemingly highlighted a race condition:

==================
WARNING: DATA RACE
Write at 0x00c001cc90f0 by goroutine 57238:
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountWithClaimJWT()
      /home/travis/build/nats-io/nats-server/server/server.go:2045 +0x29c
  github.com/nats-io/nats-server/v2/server.(*Server).fetchAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2130 +0xbd
  github.com/nats-io/nats-server/v2/server.(*Server).lookupAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2000 +0x226
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountClaimsWithRefresh()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3461 +0x3097
  github.com/nats-io/nats-server/v2/server.(*Server).UpdateAccountClaims()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3174 +0x1d3
  github.com/nats-io/nats-server/v2/server.(*Server).buildInternalAccount()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3813 +0x1ba
  github.com/nats-io/nats-server/v2/server.(*Server).fetchAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2124 +0x84
  github.com/nats-io/nats-server/v2/server.(*Server).lookupAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2000 +0x226
  github.com/nats-io/nats-server/v2/server.(*Server).updateAccountClaimsWithRefresh()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3461 +0x3097
  github.com/nats-io/nats-server/v2/server.(*Server).UpdateAccountClaims()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3174 +0x1d3
  github.com/nats-io/nats-server/v2/server.(*Server).buildInternalAccount()
      /home/travis/build/nats-io/nats-server/server/accounts.go:3813 +0x1ba
  github.com/nats-io/nats-server/v2/server.(*Server).fetchAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2124 +0x84
  github.com/nats-io/nats-server/v2/server.(*Server).lookupAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2000 +0x226
  github.com/nats-io/nats-server/v2/server.(*Server).LookupAccount()
      /home/travis/build/nats-io/nats-server/server/server.go:2006 +0x226c
  github.com/nats-io/nats-server/v2/server.(*Server).processClientOrLeafAuthentication()
      /home/travis/build/nats-io/nats-server/server/auth.go:897 +0x226d
  github.com/nats-io/nats-server/v2/server.(*Server).isClientAuthorized()
      /home/travis/build/nats-io/nats-server/server/auth.go:389 +0xb9
  github.com/nats-io/nats-server/v2/server.(*Server).checkAuthentication()
      /home/travis/build/nats-io/nats-server/server/auth.go:365 +0x7a
  github.com/nats-io/nats-server/v2/server.(*client).processConnect()
      /home/travis/build/nats-io/nats-server/server/client.go:2114 +0xf84
  github.com/nats-io/nats-server/v2/server.(*client).parse()
      /home/travis/build/nats-io/nats-server/server/parser.go:942 +0x1437
  github.com/nats-io/nats-server/v2/server.(*client).readLoop()
      /home/travis/build/nats-io/nats-server/server/client.go:1391 +0x1b18
  github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
      /home/travis/build/nats-io/nats-server/server/server.go:3348 +0x54
  github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
      /home/travis/build/nats-io/nats-server/server/server.go:3834 +0x59
Previous read at 0x00c001cc90f0 by goroutine 57686:
  github.com/nats-io/nats-server/v2/server.(*Server).processClientOrLeafAuthentication()
      /home/travis/build/nats-io/nats-server/server/auth.go:901 +0x2291
  github.com/nats-io/nats-server/v2/server.(*Server).isClientAuthorized()
      /home/travis/build/nats-io/nats-server/server/auth.go:389 +0xb9
  github.com/nats-io/nats-server/v2/server.(*Server).checkAuthentication()
      /home/travis/build/nats-io/nats-server/server/auth.go:365 +0x7a
  github.com/nats-io/nats-server/v2/server.(*client).processConnect()
      /home/travis/build/nats-io/nats-server/server/client.go:2114 +0xf84
  github.com/nats-io/nats-server/v2/server.(*client).parse()
      /home/travis/build/nats-io/nats-server/server/parser.go:942 +0x1437
  github.com/nats-io/nats-server/v2/server.(*client).readLoop()
      /home/travis/build/nats-io/nats-server/server/client.go:1391 +0x1b18
  github.com/nats-io/nats-server/v2/server.(*Server).createClientEx.func1()
      /home/travis/build/nats-io/nats-server/server/server.go:3348 +0x54
  github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine.func1()
      /home/travis/build/nats-io/nats-server/server/server.go:3834 +0x59

Looks like processClientOrLeafAuthentication doesn't take the account lock to read the issuer.

derekcollison
derekcollison reviewed Sep 13, 2024
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Let's fix race but LGTM.

derekcollison
derekcollison approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

derekcollison
derekcollison approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks, was just about to report that one.

@derekcollison derekcollison merged commit 66dafcf into main Sep 13, 2024
5 checks passed
@derekcollison derekcollison deleted the update-account-issuer branch September 13, 2024 17:49
wallyqs pushed a commit that referenced this pull request Sep 17, 2024
@aricart @wallyqs
update account issuer always (#5886)
e9e77cf 
changed to update the account issuer for an account to always update
otherwise the information shown in accountz will not be consistent with
the decoded JWT
@wallyqs wallyqs mentioned this pull request Sep 17, 2024
Merged
@wallyqs wallyqs changed the title update account issuer always [FIXED] Update account issuer in accountz Sep 17, 2024
@wallyqs wallyqs changed the title [FIXED] Update account issuer in accountz [FIXED] Always update account issuer in accountsz Sep 17, 2024
wallyqs pushed a commit that referenced this pull request Sep 17, 2024
@aricart @wallyqs
update account issuer always (#5886)
7e748fa 
changed to update the account issuer for an account to always update
otherwise the information shown in accountz will not be consistent with
the decoded JWT
wallyqs added a commit that referenced this pull request Sep 17, 2024
@wallyqs
Cherry picks for v2.10.21 RC1 (#5898)
032ac9c 
Includes:

- #5854
- #5863
- #5869
- #5886
- #5893
- #5896
- #5852
- #5781
- #5887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aricart @neilalexander @derekcollison