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

Add GitHub IDP validations #1907

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Add GitHub IDP validations #1907

merged 1 commit into from
Apr 16, 2024

Conversation

joshuatcasey
Copy link
Member

Add GitHub IDP validations

@codecov Codecov
Copy link

codecov bot commented Apr 3, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 38.79%. Comparing base (84007cb) to head (9ef688a).
Report is 3 commits behind head on github_identity_provider.

Files Patch % Lines
test/testlib/client.go 0.00% 35 Missing ⚠️
internal/controller/utils.go 0.00% 27 Missing ⚠️
internal/upstreamgithub/upstreamgithub.go 0.00% 16 Missing ⚠️
...g/githubupstreamwatcher/github_upstream_watcher.go 95.00% 9 Missing and 4 partials ⚠️
Additional details and impacted files 
@@                     Coverage Diff                      @@
##           github_identity_provider    #1907      +/-   ##
============================================================
+ Coverage                     38.62%   38.79%   +0.16%     
============================================================
  Files                           356      355       -1     
  Lines                         44994    45287     +293     
============================================================
+ Hits                          17380    17570     +190     
- Misses                        27086    27190     +104     
+ Partials                        528      527       -1

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

benjaminapetersen
benjaminapetersen reviewed Apr 4, 2024
View reviewed changes
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go Outdated
case v1alpha1.GitHubUsernameID:
break
default:
return nil, fmt.Errorf("invalid spec.claims.username")
Copy link
Member

Choose a reason for hiding this comment

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

If it really can't happen, do we just want to log the error?
If an error here does happen, then it is a user configuration error, so we don't really want to return a Sync() error and trigger the loop again. A user has to step in and fix the config.

@joshuatcasey joshuatcasey force-pushed the jtc/add-github-idp-validations branch 11 times, most recently from 2afbe31 to d125f75 Compare April 10, 2024 10:56
@joshuatcasey joshuatcasey marked this pull request as ready for review April 10, 2024 12:09
@joshuatcasey joshuatcasey force-pushed the jtc/add-github-idp-validations branch from d125f75 to 36b5246 Compare April 10, 2024 12:10
benjaminapetersen
benjaminapetersen reviewed Apr 12, 2024
View reviewed changes
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go

// Should there be some sort of catch-all condition to capture this?
// This does not actually prevent a GitHub IDP from being added to the cache.
groupNameAttribute, usernameAttribute, userAndGroupErr := validateUserAndGroupAttributes(upstream)
Copy link
Member

Choose a reason for hiding this comment

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

We probably should have a condition to capture this.

benjaminapetersen
benjaminapetersen reviewed Apr 12, 2024
View reviewed changes
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
return conditions, hostWithHttps, c.httpClientBuilder(certPool), conn.Close()
}

func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) (v1alpha1.GitHubGroupNameAttribute, v1alpha1.GitHubUsernameAttribute, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we return a condition as well from this func?

@joshuatcasey joshuatcasey force-pushed the jtc/add-github-idp-validations branch 3 times, most recently from d028945 to 9ef688a Compare April 16, 2024 02:28
benjaminapetersen
benjaminapetersen reviewed Apr 16, 2024
View reviewed changes
internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go
buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"),
buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host),
buildLogForUpdatingPhase("minimal-idp-name", "Error"),
},
Copy link
Member

Choose a reason for hiding this comment

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

One more at the end, since update status is the last thing the controller does:

{
   name: "When update status request fails the controller will resync",
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how to fake out these K8s requests so that they return errors.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't too bad. See this example:

https://github.com/vmware-tanzu/pinniped/blob/main/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go#L1618

@joshuatcasey joshuatcasey force-pushed the jtc/add-github-idp-validations branch from 9ef688a to c9b61ef Compare April 16, 2024 19:34
benjaminapetersen
benjaminapetersen approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

Nice, lets move this forward!

@benjaminapetersen benjaminapetersen merged commit 0dc7dfc into github_identity_provider Apr 16, 2024
31 checks passed
@benjaminapetersen benjaminapetersen deleted the jtc/add-github-idp-validations branch April 16, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshuatcasey @benjaminapetersen @vmwclabot