- Notifications
You must be signed in to change notification settings - Fork928
chore: support multi-org group sync with runtime configuration#14578
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
Conversation
07fa2b1 toc343f86Comparede150b1 to2163eb7Compareb22d62d toae5315fCompare| }, nil | ||
| } | ||
| func (s AGPLIDPSync) SyncGroups(ctx context.Context, db database.Store, user database.User, params GroupParams) 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.
Enterprise license code just handles theParseGroupClaims. So AGPL will always be disabled.
I put the actualSyncGroups 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to3263d99Compare1995c13 to0df7f28CompareThere 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.
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?
Uh oh!
There was an error while loading.Please reload this page.
@f0ssel yea, I rebased it correctly, but forgot to change the base branch to |
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.
still trying to catch up to speed but all lgtm!
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm very curious what this is necessary for
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.
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.
6a846cd intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 testshad 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 syncin 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.