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.

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

Introduce CheckEndpointConnector, implement for OpenShift connector. #2512

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dhaus67
Copy link

@dhaus67 dhaus67 commented May 9, 2022

Overview

Add the ability to check the endpoints used by the connectors. The goal is to give clients the possibility to check whether a connector's endpoints are reachable.

What this PR does / why we need it

The ultimate goal behind is to avoid potential misconfiguration when creating a new connector. Right now, it wouldn't be possible to detect this at the configuration point, but rather only when using the connector to login if you i.e. use dex within your application.

Ideally, we'd like to check this during creation.
Since this would be a major, undesirable change to introduce this during the Open method, I've opted to add a new interface which connectors may implement, if they want to offer the functionality to provide such a method.

Alternative to the interface choice, we could also decide to make this method exclusive to the openshift connector, similar to the OpenWithHTTPClient, to only expose it for library usage.

Special notes for your reviewer

Does this PR introduce a user-facing change?

Added CheckEndpointConnector, provide the ability to test

@dhaus67 dhaus67 force-pushed the oauth2-endpoints-availability branch from 29b843c to d5e9bbd Compare May 9, 2022 22:47
@dhaus67
Copy link
Author

dhaus67 commented May 9, 2022

This might be interesting to you @nabokihms , as it is somewhat similar to #2430 .

@dhaus67 dhaus67 force-pushed the oauth2-endpoints-availability branch from d5e9bbd to 3e961e8 Compare May 9, 2022 23:01
@nabokihms
Copy link
Member

Thank you for the PR. It makes sense to me. Generic OIDC connector also works the same way. I will take a look through the code and give detailed feedback next week.

@nabokihms nabokihms self-requested a review May 13, 2022 14:18
@nabokihms
Copy link
Member

I've looked through this PR, and, in my opinion, it is not right to add more public interfaces only to use Dex as a library. The best for me is to add optional endpoints health check to the Open method. It just makes more sense.

@sagikazarmark, could you also please have a look at this code?

@nabokihms nabokihms requested a review from sagikazarmark May 25, 2022 17:18
@dhaus67 dhaus67 force-pushed the oauth2-endpoints-availability branch from 913d306 to 3c09a4c Compare May 30, 2022 01:34
@dhaus67 dhaus67 force-pushed the oauth2-endpoints-availability branch from 3c09a4c to eea20e1 Compare May 30, 2022 01:37
@dhaus67
Copy link
Author

dhaus67 commented May 30, 2022

@nabokihms thanks for the feedback.
I can see why we don't want to expose a public interface for that as well. I've gone the way you mentioned with regards to the optional endpoints health check via options that can be passed to OpenWithOptions, allowing for customization within library usage.

Let me know what you think about it.

@nabokihms
Copy link
Member

Thank you for the quick fix, @dhaus67. I'm still waiting for the second opinion here. We need to figure out what to do with this (and probably with further) Openshift Connector improvements.

@nabokihms nabokihms added this to the v2.33.0 milestone May 30, 2022
@nabokihms nabokihms added the release-note/enhancement Release note: Enhancements label May 30, 2022
@dhaus67
Copy link
Author

dhaus67 commented Jun 13, 2022

@nabokihms @sagikazarmark any update on your end regarding this?

@nabokihms nabokihms modified the milestones: v2.33.0, v2.34.0 Jul 26, 2022
@nabokihms nabokihms modified the milestones: v2.34.0, v2.35.0 Sep 16, 2022
@sagikazarmark sagikazarmark modified the milestones: v2.35.0, v2.36.0 Sep 29, 2022
@nabokihms nabokihms modified the milestones: v2.36.0, v2.37.0 Mar 6, 2023
@sagikazarmark sagikazarmark modified the milestones: v2.37.0, v2.38.0 May 12, 2023
@nabokihms nabokihms removed this from the v2.38.0 milestone Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/enhancement Release note: Enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants