First draft of a GitHubIdentityProvider CRD

[joshuatcasey]

See #1859

[cfryanr]

Is the naming sort of awkward that you can set a field called "AllowSomething" to have the value "Deny"? The field's name says it is about allowing something, not denying something.

Maybe name it something like AllowedUsers, to be symmetrical with AllowedOrganizations? Possible values would be something like:

• OnlyUsersBelongingToAllowedOrganizations (the default), which also makes it clear that it relates back to the AllowedOrganizations field
• AllUsers

[joshuatcasey]

It's definitely a little awkward.

How about GitHubOrganizationLoginPolicy with AllowedOrganizationsOnly as the default and AllOrganizationsAllowed as the other option?

[cfryanr]

I find it confusing that those names have substrings AllowedOrganizations and OrganizationsAllowed which makes them sound as if they are the same thing at first glance.

[joshuatcasey]

Hmmm that makes sense. Let's chat tomorrow and capture the decision here.

[cfryanr]

How about AllowedOrganizationsPolicy? That name ties it back to the AllowedOrganizations field.

We want the non-default value to emphasize the effect that it allows all GitHub users to authenticate regardless of org membership, even if they belong to no orgs. So how about enum values like OnlyUsersBelongingToSpecifiedOrgs and AllGitHubUsers.

So the settings in practice would look like:

spec:
  allowedOrganizations: [vmware-tanzu, broadcom]
  allowedOrganizationsPolicy: OnlyUsersBelongingToSpecifiedOrgs

or

spec:
  allowedOrganizations: []
  allowedOrganizationsPolicy: AllGitHubUsers

and because of the default values of the two settings, these would also be valid:

spec:
  allowedOrganizations: [vmware-tanzu, broadcom]

and

spec:
  allowedOrganizationsPolicy: AllGitHubUsers

The only configurations that allow all GitHub users must explicitly say AllGitHubUsers because it is the non-default value. Kinda verbose but hopefully very clear.

[joshuatcasey]

I like it

[joshuatcasey]

Let's do this instead:

---
spec:
  client:
    secretName: foo
  allowAuthentication:
    organizations:
      policy: OnlyUsersFromAllowedOrganizations
      allowed:
      - foo
      - vmware-tanzu
      - Broadcom
    personalAccessTokens:
      fineGrained: true
---
spec:
  client:
    secretName: foo
  allowAuthentication:
    organizations:
      policy: AllGitHubUsers
    personalAccessTokens:
      classic: true

[codecov]

Codecov Report

Attention: Patch coverage is 0.51948% with 383 lines in your changes are missing coverage. Please review.

Project coverage is 38.24%. Comparing base (146e61e) to head (42dd8d1).
Report is 2 commits behind head on github_identity_provider.

Files	Patch %	Lines
...s/supervisor/idp/v1alpha1/zz_generated.deepcopy.go	0.00%	153 Missing ⚠️
...ioned/typed/idp/v1alpha1/githubidentityprovider.go	0.00%	107 Missing ⚠️
...d/idp/v1alpha1/fake/fake_githubidentityprovider.go	0.00%	71 Missing ⚠️
...sor/listers/idp/v1alpha1/githubidentityprovider.go	0.00%	23 Missing ⚠️
...nalversions/idp/v1alpha1/githubidentityprovider.go	0.00%	21 Missing ⚠️
...rsioned/typed/idp/v1alpha1/fake/fake_idp_client.go	0.00%	2 Missing ⚠️
...ientset/versioned/typed/idp/v1alpha1/idp_client.go	0.00%	2 Missing ⚠️
...t/supervisor/informers/externalversions/generic.go	0.00%	2 Missing ⚠️
...formers/externalversions/idp/v1alpha1/interface.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           github_identity_provider    #1900      +/-   ##
============================================================
- Coverage                     38.56%   38.24%   -0.33%     
============================================================
  Files                           350      354       +4     
  Lines                         44514    44899     +385     
============================================================
+ Hits                          17169    17173       +4     
- Misses                        26831    27212     +381     
  Partials                        514      514              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[benjaminapetersen]

Noting we can now rebase this on main.

[cfryanr]

Maybe try to avoid using the term "downstream" in all the comments?

[benjaminapetersen]

LGTM, I tend to agree with dropping the simple CEL validations since we have to do more sophisticated in the controller anyway. 👍