Skip to content

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Oct 11, 2023

This is an API to openshift/enhancements#1493

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 11, 2023

Hello @stlaz! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 11, 2023
@openshift-ci openshift-ci bot requested review from bparees and jwforres October 11, 2023 12:10
@stlaz stlaz force-pushed the direct_oidc_config branch from 53f216a to 475a752 Compare October 11, 2023 12:27
@stlaz stlaz force-pushed the direct_oidc_config branch 2 times, most recently from cdd7c02 to f17549a Compare October 12, 2023 14:41
KubeConfigKey = "kubeConfig"
)

type OIDCProvider struct {
Copy link
Contributor

@deads2k deads2k Oct 16, 2023

Choose a reason for hiding this comment

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

Consider for future, no need right now.

Is this the right scope for configuring a clientID and reference to a clientSecret for the console

Pros:

  1. keeps configuration close together from a user perspective
  2. will allow different clientIDs for different oidc providers for console

Cons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each client for each application should be different, or should be provided with means to request tokens with different audiences (e.g. the resource parameter). That means that should we choose to configure client parameters via this API, we need to make it configurable for a number of clients. That can get messy, but is still doable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yesterday, Seth argued the opposite: specifically name every clientID/secret pair. We should get on the same page.

@stlaz stlaz force-pushed the direct_oidc_config branch from 67c5422 to 4f09bdb Compare October 18, 2023 09:04
@stlaz stlaz force-pushed the direct_oidc_config branch 2 times, most recently from 2bb98ac to 1969932 Compare October 20, 2023 13:10
@stlaz stlaz force-pushed the direct_oidc_config branch from 1969932 to 90a73ec Compare October 24, 2023 11:17
@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 24, 2023
@stlaz stlaz force-pushed the direct_oidc_config branch from 0dac365 to d0f11a6 Compare October 24, 2023 12:39
// +kubebuilder:validation:Enum={"", "NoPrefix", "Prefix"}
PrefixPolicy UsernamePrefixPolicy `json:"prefixPolicy"`

Prefix *UsernamePrefix `json:"prefix"`
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still no CEL here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Test should be able to prove CEL is missing.

Copy link
Contributor Author

@stlaz stlaz Oct 25, 2023

Choose a reason for hiding this comment

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

Not sure which CEL validation is missing. I defined some rules on the object level.

I added tests that prove that the current CEL works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see my review mistake. Realized it in the shower this morning. Agreed.

@deads2k
Copy link
Contributor

deads2k commented Oct 25, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023
@deads2k
Copy link
Contributor

deads2k commented Oct 25, 2023

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2023
@deads2k
Copy link
Contributor

deads2k commented Oct 25, 2023

/refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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 merged commit c103851 into openshift:master Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants