chore: support multi-org group sync with runtime configuration #14578
Conversation
07fa2b1 to
c343f86
Compare
de150b1 to
2163eb7
Compare
0f7deba to
9f546d0
Compare
b22d62d to
ae5315f
Compare
| }, nil | ||
| } | ||
|
|
||
| func (s AGPLIDPSync) SyncGroups(ctx context.Context, db database.Store, user database.User, params GroupParams) error { |
There was a problem hiding this comment.
Enterprise license code just handles the ParseGroupClaims. So AGPL will always be disabled.
I put the actual SyncGroups code in AGPL so that I can unit test and maintain the sync code all in 1 package. It is a convenience, while still making sure AGPL code cannot leverage this feature.
| // Only care about the default org for deployment settings if the | ||
| // legacy deployment settings exist. | ||
| defaultOrgID := uuid.Nil | ||
| // Default organization is configured via legacy deployment values | ||
| if s.DeploymentSyncSettings.Legacy.GroupField != "" { | ||
| defaultOrganization, err := db.GetDefaultOrganization(ctx) | ||
| if err != nil { | ||
| return xerrors.Errorf("get default organization: %w", err) | ||
| } | ||
| defaultOrgID = defaultOrganization.ID | ||
| } |
There was a problem hiding this comment.
Startup group sync config has to be pulled for the default org 😢. Backwards compatibility.
| orgResolver := s.Manager.OrganizationResolver(tx, orgID) | ||
| settings, err := s.SyncSettings.Group.Resolve(ctx, orgResolver) |
There was a problem hiding this comment.
Currently this is a DB call.
#14586 will make it a cache lookup
So 100 org deployments would be a bit slow to login until we handle caching. Problem for another day
| GroupField string | ||
| // GroupAllowList (if set) will restrict authentication to only users who | ||
| // have at least one group in this list. | ||
| // A map representation is used for easier lookup. | ||
| GroupAllowList map[string]struct{} |
There was a problem hiding this comment.
At some point we should disconnect these from the default org. Might need to move the existing config env and flags, and deprecate the old.
a5b64dd to
3263d99
Compare
1995c13 to
0df7f28
Compare
There was a problem hiding this comment.
Overall looks good, glad the runtime config only impacts this feature. Unfortunate about the sql tests being disabled, maybe we can add some sort of simple cache or debounce for just tests? Could be a followup.
Also it seems like all the frontend changes should be in a separate PR? Or maybe the branch just needs rebasing?
|
@f0ssel yea, I rebased it correctly, but forgot to change the base branch to |
coderd/database/queries/groups.sql
Outdated
| AND CASE WHEN array_length(@group_names :: text[], 1) > 0 THEN | ||
| groups.name = ANY(@group_names) | ||
| ELSE true | ||
| END |
There was a problem hiding this comment.
| AND CASE WHEN array_length(@group_names :: text[], 1) > 0 THEN | |
| groups.name = ANY(@group_names) | |
| ELSE true | |
| END | |
| AND CASE WHEN array_length(@group_names :: text[], 1) > 0 THEN | |
| groups.name = ANY(@group_names) | |
| ELSE true | |
| END |
nit; indentation is a little weird
coderd/database/dbmem/dbmem.go
Outdated
| func (q *FakeQuerier) getGroupByNameNoLock(arg database.NameOrganizationPair) (database.Group, error) { | ||
| for _, group := range q.groups { | ||
| if group.OrganizationID == arg.OrganizationID && | ||
| group.Name == arg.Name { | ||
| return group, nil | ||
| } | ||
| } | ||
|
|
||
| return database.Group{}, sql.ErrNoRows | ||
| } |
There was a problem hiding this comment.
I'm very curious what this is necessary for
There was a problem hiding this comment.
Oh, I originally was using it in another query. Wanted to reuse the same code. Now it's not necessary to extract to it's own function 🤷♂️. I'll put it back.
Closes #14202
What is this
Group sync implemented to support
multi-organizationruntime configuration (CRUD does not yet exist).All previous logic existed in
oidcGroupsandoauthLogin. I disliked this approach, as the components were completely untestable. So tests had to be integration tests.The goal of this package is to completely test the sync logic independent of the oidc or broader coderd code.
What is does
Enables group sync to work in multiple organizations, not just the default. There is no way to configure other organizations in this PR, the API and CRUD is not done yet.
Unit tests show how these values can be set.
Testing notes
This is a large refactor to a section of code that had very little testing of it's parts. It did however have good integration style tests for group sync in the default org. In the package tests, multi-org sync is tested, but only with
dbmemas it inserts a lot of data. When CRUD api is added, more tests can be added similar to those below for multi-org.