CNTRLPLANE-945: Add tests for ExternalOIDC and ExternalOIDCWithUIDAndExtraClaimMappings features #29917
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
everettraven
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request references CNTRLPLANE-945 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test all |
2 similar comments
|
/test all |
|
/test all |
|
Job Failure Risk Analysis for sha: 5d8b755
|
|
/test all |
|
Job Failure Risk Analysis for sha: 81377a4
|
openshift-merge-robot
added
the
needs-rebase
everettraven
force-pushed
the
feature/external-oidc-tests
branch
from
937d61b to
f70c43a
Compare
openshift-merge-robot
removed
the
needs-rebase
everettraven
force-pushed
the
feature/external-oidc-tests
branch
from
efa4dee to
effd33f
Compare
| } | ||
|
|
||
| func (c *CLI) UserConfig() *rest.Config { | ||
| if c.token != "" { |
There was a problem hiding this comment.
Note for reviewers - this change was added because whenever a token was set here, getting the user config would attempt to pull the config from an empty config path.
This resulted in a consistent error so I updated this to handle the case where a token is explicitly set.
There was a problem hiding this comment.
Do we have a job that exercise this path? This seems to the only change that might impact common path. So I want to double check.
There was a problem hiding this comment.
It looks like there are a few references in tests that I would expect to run in various jobs: https://github.com/search?q=repo%3Aopenshift/origin%20UserConfig&type=code (planning to search for an explicit job and update here)
If it feels too risky to make this change here, I could try to come up with some other approach.
There was a problem hiding this comment.
Looks like some oauthserver and etcd tests use the UserConfig() method.
Specifically,
origin/test/extended/etcd/etcd_test_runner.go
Lines 29 to 57 in e1ae86a
Because it calls
it should end up eventually triggering this code path because that call ends up using the same underlying call that I use in my test -GetClientConfigForUser(): origin/test/extended/util/client.go
Line 268 in e1ae86a
|
@everettraven: This pull request references CNTRLPLANE-945 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
everettraven
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Job Failure Risk Analysis for sha: effd33f
|
| This test suite runs tests to validate cluster behavior when cluster authentication is configured to use an external OIDC provider. | ||
| `), | ||
| Qualifiers: []string{ | ||
| `name.contains("[Suite:openshift/auth/external-oidc") && !name.contains("[Skipped]")`, |
There was a problem hiding this comment.
| `name.contains("[Suite:openshift/auth/external-oidc") && !name.contains("[Skipped]")`, | |
| `name.contains("[Suite:openshift/auth/external-oidc]") && !name.contains("[Skipped]")`, |
There was a problem hiding this comment.
I had intentionally left off the end bracket because it seems to be the pattern other test suites follow.
Presumably this is so you can run sub-suites of this as part of this suite (i.e if I did something like [Suite:openshift/auth/external-oidc/some-sub-thing] the test with this "tag" would still run as part of the openshift/auth/external-oidc test suite.
If we think this is unnecessary and that we should deviate from existing convention, I can add the closing bracket.
There was a problem hiding this comment.
Right, that's a good point -- I just thought this was a typo, but this makes sense 👍
test/extended/authentication/oidc.go
Outdated
|
|
||
| o.Expect(apiServerArgs["authentication-token-webhook-config-file"]).To(o.BeNil(), "authentication-token-webhook-config-file argument should not be specified when OIDC authentication is configured") | ||
| o.Expect(apiServerArgs["authentication-token-webhook-version"]).To(o.BeNil(), "authentication-token-webhook-version argument should not be specified when OIDC authentication is configured") | ||
| o.Expect(apiServerArgs["authConfig"]).To(o.BeNil(), "authConfig argument should not be specified when OIDC authentication is configured") |
There was a problem hiding this comment.
authConfig is not under apiServerArguments, it's on the same level.
There was a problem hiding this comment.
Good catch. I think what I meant here is authentication-config - will update
There was a problem hiding this comment.
I think authConfig is still a valid oauth related arg to validate missing -- it basically contains the path for the oauth metadata file. It's just in the same level as apiServerArguments instead of part of it.
test/extended/authentication/oidc.go
Outdated
| copiedOC := *oc | ||
| tokenOC := copiedOC.WithToken(keycloakCli.AccessToken()) | ||
|
|
||
| _, err = tokenOC.KubeClient().AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authzv1.SelfSubjectAccessReview{ |
There was a problem hiding this comment.
nit: Why not kill two birds with one stone with the next SSR test? Proves both token acceptance and cluster identity mapping. We could still split it into two tests that validate these two things.
Also, why not also use an SSR to prove the previous oauth token test as well?
There was a problem hiding this comment.
nit: Why not kill two birds with one stone with the next SSR test? Proves both token acceptance and cluster identity mapping. We could still split it into two tests that validate these two things.
Fair question. We can certainly perform a shared SSR.
Also, why not also use an SSR to prove the previous oauth token test as well?
Certainly can. I think I just went with the GET on Pods because it was a quick and easy way to say "we are now unauthorized to get something we were previously authorized to get".
I think an SSR is much more reliable, so I'll switch it to use that.
test/extended/authentication/oidc.go
Outdated
| copiedOC := *oc | ||
| tokenOC := copiedOC.WithToken(keycloakCli.AccessToken()) | ||
|
|
||
| _, err = tokenOC.KubeClient().AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authzv1.SelfSubjectAccessReview{ |
There was a problem hiding this comment.
nit: In the spirit of previous comments, maybe an SSR is sufficient?
test/extended/authentication/oidc.go
Outdated
|
|
||
| o.Expect(apiServerArgs["authentication-token-webhook-config-file"]).NotTo(o.BeNil(), "authentication-token-webhook-config-file argument should be specified when OIDC authentication is not configured") | ||
| o.Expect(apiServerArgs["authentication-token-webhook-version"]).NotTo(o.BeNil(), "authentication-token-webhook-version argument should be specified when OIDC authentication is not configured") | ||
| o.Expect(apiServerArgs["authConfig"]).NotTo(o.BeNil(), "authConfig argument should be specified when OIDC authentication is not configured") |
There was a problem hiding this comment.
Same as before, authConfig isn't part of apiServerArguments.
Is it likely that we use this beyond testing authentication? IMO we can extract this whenever we have a use case to re-use it. For now, I imagine we only care to use it for authentication related tests. |
everettraven
force-pushed
the
feature/external-oidc-tests
branch
from
effd33f to
68dad7b
Compare
|
/hold botched rebase |
openshift-ci
bot
added
the
do-not-merge/hold
Signed-off-by: Bryce Palmer <[email protected]>
everettraven
force-pushed
the
feature/external-oidc-tests
branch
from
0b3f84e to
e72069c
Compare
|
Build issues were due to a need to rebase and properly pickup changes from an o/api bump I missed. Should be fixed now. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
|
/approve |
openshift-ci
bot
added
the
approved
|
Job Failure Risk Analysis for sha: e72069c
|
|
Job Failure Risk Analysis for sha: e72069c
|
| group := map[string]interface{}{ | ||
| "name": name, | ||
| } |
There was a problem hiding this comment.
Nit:
Alternatives:
| group := map[string]interface{}{ | |
| "name": name, | |
| } | |
| group := struct{ | |
| Name string `json:"name"` | |
| }{ | |
| Name: name, | |
| } |
| group := map[string]interface{}{ | |
| "name": name, | |
| } | |
| group := map[string]any{ | |
| "name": name, | |
| } |
Applies to similar code in this file.
| data := url.Values{ | ||
| "username": []string{username}, | ||
| "password": []string{password}, | ||
| "grant_type": []string{"password"}, | ||
| "client_id": []string{clientID}, | ||
| "scope": []string{"openid"}, | ||
| } |
There was a problem hiding this comment.
Nit:
| data := url.Values{ | |
| "username": []string{username}, | |
| "password": []string{password}, | |
| "grant_type": []string{"password"}, | |
| "client_id": []string{clientID}, | |
| "scope": []string{"openid"}, | |
| } | |
| data := url.Values{} | |
| data.Set("username", username) | |
| data.Set("password", password) | |
| data.Set("grant_type", "password") | |
| data.Set("client_id", clientID) | |
| data.Set("scope", "openid") |
| respBodyData, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("reading response data: %w", err) | ||
| } | ||
|
|
||
| err = json.Unmarshal(respBodyData, &respBody) | ||
| if err != nil { | ||
| return fmt.Errorf("unmarshalling response body %s: %w", respBodyData, err) | ||
| } |
There was a problem hiding this comment.
Streams without buffering entire reponse:
| respBodyData, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return fmt.Errorf("reading response data: %w", err) | |
| } | |
| err = json.Unmarshal(respBodyData, &respBody) | |
| if err != nil { | |
| return fmt.Errorf("unmarshalling response body %s: %w", respBodyData, err) | |
| } | |
| if err := json.NewDecoder(resp.Body).Decode(&respBody); err != nil { | |
| return fmt.Errorf("unmarshalling response data: %w", err) | |
| } |
Applies to other occurrences as well. Only thing that I feel strongly about. But I would not block the PR as this is not production code.
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| respBody := map[string]interface{}{} |
There was a problem hiding this comment.
nit: I am a strong believer in anonymous structs for type-safety 😄
var tokenResponse struct {
AccessToken string `json:"access_token"`
IDToken string `json:"id_token"`
TokenType string `json:"token_type,omitempty"`
ExpiresIn int `json:"expires_in,omitempty"`
Scope string `json:"scope,omitempty"`
}| user := map[string]interface{}{ | ||
| "username": username, | ||
| "email": fmt.Sprintf("%[email protected]", username), | ||
| "enabled": true, | ||
| "emailVerified": true, | ||
| "groups": groups, | ||
| "credentials": []map[string]interface{}{ | ||
| { | ||
| "temporary": false, | ||
| "type": "password", | ||
| "value": password, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Nit:
user := struct {
Username string `json:"username"`
Email string `json:"email"`
Enabled bool `json:"enabled"`
EmailVerified bool `json:"emailVerified"`
Groups []string `json:"groups"`
Credentials []struct {
Temporary bool `json:"temporary"`
Type string `json:"type"`
Value string `json:"value"`
} `json:"credentials"`
}{
Username: username,
Email: fmt.Sprintf("%[email protected]", username),
Enabled: true,
EmailVerified: true,
Groups: groups,
Credentials: []struct {
Temporary bool `json:"temporary"`
Type string `json:"type"`
Value string `json:"value"`
}{
{
Temporary: false,
Type: "password",
Value: password,
},
},
}| tokenURL := *kc.adminURL | ||
| tokenURL.Path = fmt.Sprintf("/realms/%s/protocol/openid-connect/token", kc.realm) | ||
|
|
||
| resp, err := kc.DoRequest(http.MethodPost, tokenURL.String(), "application/x-www-form-urlencoded", false, bytes.NewBuffer([]byte(data.Encode()))) |
There was a problem hiding this comment.
There was a problem hiding this comment.
We only post form data for this one type of request. If we find ourselves repeating this logic quite frequently I could see value in a common method.
| } | ||
| cleanups = append(cleanups, cleanup) | ||
|
|
||
| return cleanups, waitForKeycloakAvailable(ctx, client, namespace) |
There was a problem hiding this comment.
hehe, let me talk you through the wonderful world of functional programming, the next time we drink a coffee together... 😄
Nit: a builder pattern would be cool (like we use it frequently in library-go) and a dedicated package (looks pretty interesting to import for kube-rbac-proxy and OIDC testing 😛)
There was a problem hiding this comment.
A builder pattern feels a bit overkill for now. If we have use cases for a common library here, lets take that as a follow-up action to create that and use it across all the places necessary?
test/extended/authentication/oidc.go
Outdated
| keycloakCli, err = keycloakClientFor(kcURL) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error creating a keycloak client") | ||
|
|
||
| // First authenticate as the admin keyloak user so we can add new groups and users |
There was a problem hiding this comment.
Nit:
| // First authenticate as the admin keyloak user so we can add new groups and users | |
| // First authenticate as the admin keycloak user so we can add new groups and users |
test/extended/authentication/oidc.go
Outdated
| } | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
There was a problem hiding this comment.
Nit:
errors.NewAggregate is very popular and widely used around our codebases. Otherwise, there seem to be some nice filtering capabilities as well:
errors.FilterOut(errors.NewAggregate(errs), apierrors.IsNotFound)
test/extended/authentication/oidc.go
Outdated
| func waitForRollout(ctx context.Context, client *exutil.CLI) { | ||
| kasCli := client.AdminOperatorClient().OperatorV1().KubeAPIServers() | ||
|
|
||
| // First wait for KAS to flip to progessing |
There was a problem hiding this comment.
Nit:
| // First wait for KAS to flip to progessing | |
| // First wait for KAS to flip to progressing |
Signed-off-by: Bryce Palmer <[email protected]>
|
Job Failure Risk Analysis for sha: bca8317
|
|
/retest |
|
Job Failure Risk Analysis for sha: bca8317
|
|
Job Failure Risk Analysis for sha: bca8317
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, ibihim, liouk, xueqzhan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
@everettraven: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: bca8317
|
openshift-merge-bot
bot
merged commit 5ef42f5
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
7 participants
This PR adds the tests necessary to promote the ExternalOIDC and ExternalOIDCWithUIDAndExtraClaimMappings feature-gates on OpenShift.
It adds a new test suite specific to tests that test the external OIDC provider authentication mode.
Some tests are intentionally marked as skipped as the functionality does not yet exist to test, but I wanted to still provide the skeleton for these tests so that we can easily make some updates when that functionality is implemented.