authentication: direct external OIDC provider #1632
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/retest |
1 similar comment
|
/retest |
liouk
force-pushed
the
direct-external-oidc-provider
branch
6 times, most recently
from
4c0e494 to
d130e8c
Compare
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
d130e8c to
e871efd
Compare
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
e871efd to
914f4c0
Compare
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
914f4c0 to
98f31c3
Compare
liouk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
|
||
| ## Graduation Criteria | ||
|
|
||
| The goal for this enhancement is to go directly to GA, because the feature is already available and in GA in HCP. |
There was a problem hiding this comment.
it will continue through techpreview, but I'm hopeful we do it quickly.
| Overall, for this feature there must be e2e tests that cover the following: | ||
| - configuring an external OIDC provider on a cluster that uses the built-in OAuth stack (good/bad configurations should be tested) | ||
| - on a cluster that uses an external OIDC provider, test reverting configuration back to the built-in OAuth stack (good/bad configurations should be tested) | ||
| - on a cluster that uses an external OIDC provider, test monitoring and cluster-authentication-operator status when the provider becomes unavailable |
There was a problem hiding this comment.
need to be sure that the user mapping capabilities work as well.
|
|
||
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| #### cluster-kube-apiserver-operator |
There was a problem hiding this comment.
should these trigger kube-apiserver revisions or simply trigger an all-at-once instant rollout using live file reloads for the kube-apiserver?
There was a problem hiding this comment.
We've agreed that this decision must be documented in this enhancement. Putting a hold until this is resolved.
/hold
|
looks like a good overall direction. I'll approve, but leave the lgtm with the team /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
818c4bb to
aa34cb5
Compare
|
Changing to WIP in order to clarify the open questions. /retitle [WIP] authentication: direct external OIDC provider |
openshift-ci
bot
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| 2. this means that the `spec.oauthMetadata` field will be set to empty; however `status.integratedOAuthMetadata` will still have a value | ||
| 3. KAS-o config observer picks up the auth type change, configures KAS pods for OIDC, triggers a rollout | ||
| 4. once rollout is successful, CAO picks up the change to OIDC, removes oauth-specific resources, including `status.integratedOAuthMetadata` | ||
| 5. KAS-o oauth-metadata config observer picks up the change in `status.integratedOAuthMetadata` and since `spec.oauthMetadata` is also unset, removes the oauth-metadata configmap; this results in a new config, which triggers a new rollout |
There was a problem hiding this comment.
mhm, I think that if you configure the external OIDC and remove the destination CM in the same step, no additional rollout will be required after cleaning up the status field, right?
There was a problem hiding this comment.
Actually, thinking more about it, I believe we cannot guarantee that these two operations will be performed atomically.
There was a problem hiding this comment.
What happens after configuring the external OIDC server? Does kas use the new one, or both?
There was a problem hiding this comment.
if you configure the external OIDC and remove the destination CM in the same step, no additional rollout will be required after cleaning up the status field, right?
Right -- that's what I've tried to achieve with the alternative proposal.
we cannot guarantee that these two operations will be performed atomically
If I understand this correctly, we could achieve atomicity if we use the same config observer to configure either the webhook or OIDC depending on the auth type, in a mutually exclusive way, i.e. configuring the one will remove configuration of the other. Indeed we cannot guarantee this will happen atomically if we use different observers.
Does kas use the new one, or both?
If we keep the webhook in the configuration and configure OIDC as well, both methods will be part of the authentication chain. If the first one in the chain fails to validate a token, then the next one will be used until it gets validated or the chain is finished. So both can be active at the same time.
One benefit of doing that, would be that we can keep the webhook up until the KAS-o has validated that OIDC works as expected; then it can mark its auth status as available, and the CAO can then clean up the oauth metadata and webhook (thus triggering a second rollout). If there's any problem with OIDC, the KAS-o won't mark OIDC as available, the CAO won't take down the webhook, and thus the webhook will keep working until the user fixes the OIDC config.
There was a problem hiding this comment.
Coming back to this for some further details I learned.
If we keep the webhook in the configuration and configure OIDC as well, both methods will be part of the authentication chain.
While in theory this could be done, in practice that's not really possible due to our authentication CRD validation. The webhook can be defined only if type is not OIDC, which means that if we switch configuration to OIDC in the auth CR then we have to get rid of the webhook.
liouk
force-pushed
the
direct-external-oidc-provider
branch
7 times, most recently
from
2c6b6f3 to
f7772dd
Compare
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
f7772dd to
13a83f2
Compare
liouk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
liouk
force-pushed
the
direct-external-oidc-provider
branch
3 times, most recently
from
7a41c9f to
8d170a5
Compare
liouk
force-pushed
the
direct-external-oidc-provider
branch
from
8d170a5 to
03439ca
Compare
|
Fixups have been squashed, and open questions addressed; cancelling hold. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@liouk: all tests passed! 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. |
|
|
||
| #### Authentication Resource | ||
|
|
||
| The cluster's Authentication CR (`authentication.config/cluster`) must be modified and updated with the configuration of the external OIDC provider in the `OIDCProviders` field. |
There was a problem hiding this comment.
| The cluster's Authentication CR (`authentication.config/cluster`) must be modified and updated with the configuration of the external OIDC provider in the `OIDCProviders` field. | |
| The cluster's Authentication CR (`authentication.config/cluster`) must be modified and updated with the configuration of the external OIDC provider in the `OIDCProviders` field. The `WebhookTokenAuthenticator` field should be removed (by setting it `null`). |
I add this explicitly because readers who are not aware of this may just set oidcProviders and ignore webhookTokenAuthenticator. This makes readers who attempt to run oc patch or oc edit on authentication.config/cluster the first time get:
The Authentication "cluster" is invalid: spec.webhookTokenAuthenticator: Invalid value: v1.WebhookTokenAuthenticator{KubeConfig:v1.SecretNameReference{Name:"webhook-authentication-integrated-oauth"}}: this field cannot be set with the "OIDC" .spec.type
This PR adds an enhancement for the implementation of using a direct external OIDC provider instead of the built-in oauth stack in OCP.
See also https://issues.redhat.com/browse/OCPSTRAT-306.