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

AUTH-356: Add openshift-cli-client OAuth Client #606

Merged
merged 2 commits into from Jul 3, 2023
Merged

AUTH-356: Add openshift-cli-client OAuth Client #606

merged 2 commits into from Jul 3, 2023

Conversation

liouk
Copy link
Member

@liouk liouk commented Apr 11, 2023

This PR introduces a new OAuthClient at bootstrap which is required for the OAuth2 Authorization Code Grant Flow for oc.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 11, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 11, 2023 •
edited by openshift-ci bot

@liouk: This pull request references AUTH-356 which is a valid jira issue.

In response to this:

This PR introduces a new OAuthClient at bootstrap which is required for the OAuth2 Authorization Code Grant Flow for oc.

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/test-infra repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023
@openshift-ci openshift-ci bot requested review from deads2k and stlaz April 11, 2023 13:42
@liouk liouk changed the title [WIP]: AUTH-356: Add openshift-cli-client OAuth Client AUTH-356: Add openshift-cli-client OAuth Client Apr 11, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023
@liouk liouk mentioned this pull request Apr 11, 2023
Merged
@liouk
Copy link
Member Author

liouk commented Apr 12, 2023

/retest-required

5 similar comments
@liouk
Copy link
Member Author

liouk commented Apr 12, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented Apr 13, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented Apr 13, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented Apr 19, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented Apr 20, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented Apr 25, 2023

@stlaz does the new oauth client need to be added to the docs?

@liouk
Copy link
Member Author

liouk commented Apr 26, 2023

/retest-required

2 similar comments
@liouk
Copy link
Member Author

liouk commented Apr 26, 2023

/retest-required

@liouk
Copy link
Member Author

liouk commented May 2, 2023

/retest-required

@liouk liouk requested a review from stlaz May 24, 2023 12:42
@s-urbaniak
Copy link
Contributor

@liouk since we're here, would you mind adding a unit test for this file? This code has no unit test coverage, we only rely on e2e tests. For mocking the client you can get inspired i.e. from here

cluster-authentication-operator/pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller_test.go

Lines 119 to 126 in 9355b59

ingressControllerIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
for _, ingressController := range tt.ingressControllers {
ingressControllerIndexer.Add(ingressController)
}
nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
for _, node := range tt.nodes {
nodeIndexer.Add(node)
}
but using oauth clients rather than nodes.

@liouk
Copy link
Member Author

liouk commented Jun 26, 2023

@s-urbaniak added unit tests for oauthclientscontroller in 5d3b63d

@liouk
Copy link
Member Author

liouk commented Jun 27, 2023

/retest-required

@liouk liouk force-pushed the pkce-oauth-client branch 2 times, most recently from acd7e24 to c505c00 Compare June 27, 2023 15:22
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2023
@arjunrn @liouk
oauthclientscontroller: add openshift-cli-client OAuth Client to boot…
ab847d3 
…strap

The operator creates an `OAuthClient` without secrets and a redirect
URIs set to the loopback address. This will be used by the CLI to obtain
tokens through [PKCE](https://datatracker.ietf.org/doc/html/rfc7636).
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@liouk
Copy link
Member Author

liouk commented Jun 29, 2023

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2023

@liouk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-single-node e623f2f link false /test e2e-aws-single-node

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/test-infra repository. I understand the commands that are listed here.

@stlaz
Copy link
Member

stlaz commented Jul 3, 2023

/approve
next time please split the vendored from non-generated changes

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liouk, s-urbaniak, stlaz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2023
@openshift-merge-robot openshift-merge-robot merged commit 39a1e26 into openshift:master Jul 3, 2023
10 of 11 checks passed
@xueqzhan xueqzhan mentioned this pull request Jul 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@deads2k deads2k Awaiting requested review from deads2k

@stlaz stlaz Awaiting requested review from stlaz

Assignees

@s-urbaniak s-urbaniak

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@liouk @openshift-ci-robot @s-urbaniak @stlaz @openshift-merge-robot @arjunrn