KAAP-847: New connector for keystone SSO login.#53
Conversation
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #1fde5a
Actionable Suggestions - 7
-
connector/keystone/keystone.go - 7
- Response body not properly closed on error · Line 294-298
- Response body not properly closed on error · Line 324-328
- Incorrect error handling for HTTP status codes · Line 392-395
- Inconsistent error handling in getAllGroupsForUser function · Line 583-583
- Inconsistent error handling in projects fetching · Line 582-583
- Inconsistent error handling in hostname retrieval · Line 582-583
- Missing validation for hostname format edge cases · Line 680-686
Additional Suggestions - 4
-
connector/keystone/federation.go - 2
-
HTTP response body not drained on error · Line 199-203The HTTP response body is not being read and discarded before closing when an error occurs. This can prevent connection reuse in the HTTP client pool.
Code suggestion
@@ -198,8 +198,13 @@ resp, err := clientNoRedirect.Do(req) if err != nil { c.logger.Error("failed to execute federation auth request", "error", err) return "", err } - defer resp.Body.Close() + defer func() { + // Drain the body to allow connection reuse + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + }() -
Redundant error logging before returning error · Line 104-109The error from `getKeystoneTokenFromFederation` is logged and then immediately returned, causing redundant error logging. Consider removing the error logging here since the error will be handled by the caller.
Code suggestion
@@ -104,7 +104,6 @@ ksToken, err = c.getKeystoneTokenFromFederation(r) if err != nil { - c.logger.Error("failed to get token from federation cookies", "error", err) return connector.Identity{}, err } c.logger.Info("successfully obtained token from federation cookies")
-
-
connector/keystone/keystone.go - 2
-
Inconsistent error formatting in refactored function · Line 222-265The `getAdminTokenUnscoped` function has been moved from a method on `conn` to a standalone function, but the error handling in the federation code still expects the old error format. This could cause inconsistent error messages.
Code suggestion
@@ -255,7 +255,7 @@ if err != nil { - return "", fmt.Errorf("keystone: error %v", err) + return "", err } if resp.StatusCode/100 != 2 { return "", fmt.Errorf("keystone login: error %v", resp.StatusCode) -
Redundant variable assignment without modification · Line 675-675The variable `keystoneUrl` is redundant as it's immediately assigned from `baseURL` and never modified before being used. This creates unnecessary code that doesn't add any value.
Code suggestion
@@ -675,8 +675,7 @@ func getHostname(baseURL string) (string, error) { - keystoneUrl := baseURL - parsedURL, err := url.Parse(keystoneUrl) + parsedURL, err := url.Parse(baseURL) if err != nil { return "", err }
-
Review Details
-
Files reviewed - 4 · Commit Range:
6d95787..6d95787- connector/keystone/federation.go
- connector/keystone/keystone.go
- connector/keystone/types.go
- server/server.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
There was a problem hiding this comment.
Code Review Agent Run #76b882
Actionable Suggestions - 5
-
connector/keystone/keystone.go - 5
- URL normalization functionality lost in refactoring · Line 273-277
- Missing query parameter separator in URL construction · Line 344-348
- Missing keystone prefix in URL construction · Line 383-387
- Incorrect API path in URL construction · Line 419-424
- API endpoint path structure changed unexpectedly · Line 457-457
Additional Suggestions - 1
-
connector/keystone/federation.go - 1
-
URL normalization logic duplicated across codebase · Line 78-84The `normalizeKeystoneURL` function is being removed but its functionality is reimplemented inline. Consider keeping the function or extracting the URL normalization logic to maintain code clarity.
Code suggestion
@@ -78,9 +78,13 @@ func (c *FederationConnector) LoginURL(scopes connector.Scopes, callbackURL, sta - // remove trailing slash from c.cfg.Host - baseURL := strings.TrimSuffix(c.cfg.Host, "/") - // remove leading slash from c.cfg.ShibbolethLoginPath - ssoLoginPath := strings.TrimPrefix(c.cfg.ShibbolethLoginPath, "/") - - u, err := url.Parse(fmt.Sprintf("%s/%s", baseURL, ssoLoginPath)) + baseURL := normalizeKeystoneURL(c.cfg.Host) + ssoLoginPath := normalizePath(c.cfg.ShibbolethLoginPath) + + u, err := url.Parse(fmt.Sprintf("%s/%s", baseURL, ssoLoginPath)) +} + +func normalizeKeystoneURL(urlStr string) string { + return strings.TrimSuffix(urlStr, "/") +} + +func normalizePath(path string) string { + return strings.TrimPrefix(path, "/")
-
Review Details
-
Files reviewed - 2 · Commit Range:
6d95787..e07336b- connector/keystone/keystone.go
- connector/keystone/federation.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
Code Review Agent Run #083bd9Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Code Review Agent Run #188829
Actionable Suggestions - 1
-
connector/keystone/federation.go - 1
- Unconditional removal of URL path suffix · Line 79-80
Review Details
-
Files reviewed - 2 · Commit Range:
e848d28..5c01ff2- connector/keystone/federation.go
- connector/keystone/federation_test.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
cruizen
left a comment
cruizen
left a comment
There was a problem hiding this comment.
Thank you for the detailed testing notes and all green checks!
Code Review Agent Run #85e510Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Code Review Agent Run #4e34be
Actionable Suggestions - 2
-
connector/keystone/keystone.go - 2
- Incorrect parameter type in function call · Line 81-81
- Incorrect parameter type passed to function · Line 151-151
Review Details
-
Files reviewed - 1 · Commit Range:
c2226eb..888aa6d- connector/keystone/keystone.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
3 participants
Overview
Added New Connector
keystone federationWhat this PR does / why we need it
This PR adds new connector for keystone federation which is required to login with SSO configured in keystone (via shibboleth)
WIKI Reference -
Refer this sequence diagram to understand how Dex-keystone-shibboleth-IDP connects each other in the new connector.
https://platform9.atlassian.net/wiki/spaces/~61ccb5f6bce5e00069e66647/pages/5091819551/KAAPI+SSO+login+for+kubectl#Proposed-design---IMPLEMENTED
Does this PR introduce a user-facing change?
Yes, with this change user will see a new option in dex login page to select SSO login -
Change Summary
connector/keystone now contains 2 connectors -
keystone.gofederation.gofederation.gounder connector/keystone - This contains all the new connector related fns like connector details and callback handling.types.go-registered new connector in
server.goTESTING
a) Tested that
kubectl get nodescommand works -- It redirects to dex login page in browser
- we get 2 options Local vs SSO
- after choosing SSO, it redirects to Okta (configured IDP in shibboleth)
- after user authenticates with okta we get the final dex token.
- Dex token for the SSO user -
Summary by Bito
This PR implements and refactors the Keystone SSO login connector with federation support, improving URL construction and normalization logic. The changes modify group retrieval logic to use the correct domain attribute and introduce a new federation-based connector to facilitate SSO via shibboleth and Okta. Log levels were adjusted from Info to Debug for more detailed operational tracking, enhancing reliability and maintainability with better error handling.