-
Notifications
You must be signed in to change notification settings - Fork 100
WIP: AUTH-543: OIDC/OAuth resource configuration #740
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
Open
liouk
wants to merge
23
commits into
openshift:master
Choose a base branch
from
liouk:oidc-oauth-cleanup
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,083
−328
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
2be467c
common: add helper to determine whether OIDC is enabled on KAS pods
liouk 6892957
controllers: add helper that deletes controller conditions from opera…
liouk 564611d
operator: configure static resources that should be removed for OIDC
liouk d673191
operator: enable or disable API services depending on whether OIDC is…
liouk 8d249b2
deployment: bypass precondition checks if OIDC is available
liouk 726e408
endpointaccessible: disable controller checks upon specific conditions
liouk a645adb
proxyconfig: disable proxy checks when external OIDC config is available
liouk fa2a646
ingressnodesavailable: disable checks when external OIDC config is av…
liouk 9dd17d8
ingressstate: disable checks when external OIDC config is available
liouk a399cba
readiness: disable checks when external OIDC config is available
liouk a3fbe42
metadata: remove operands if external OIDC config is available
liouk 98a2d89
routercerts: remove operands if external OIDC config is available
liouk c621dd4
serviceca: remove operands if external OIDC config is available
liouk e6acb56
payload: remove operands if external OIDC config is available
liouk c1c28bc
customroute: remove operands and clear custom route ingress status if…
liouk 2cdda07
controllers: add switchable informer controller
liouk aeeaa03
oauthclientscontroller: remove operands when external OIDC config is …
liouk 4933044
trustdistribution: remove operands if external OIDC config is available
liouk 3b7fb52
webhookauthenticator: remove operands if external OIDC config is avai…
liouk 5783e47
test/e2e-oidc: validate oauth resources missing when OIDC gets rolled…
liouk dc6ad1e
drop! pull in lib-go workload changes
liouk e72ead9
drop! use new lib-go to delete workloads
liouk 1a9d90f
drop! add helper envs in e2e test
liouk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
package common | ||
|
||
import ( | ||
"fmt" | ||
"strings" | ||
|
||
configv1 "github.com/openshift/api/config/v1" | ||
configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1" | ||
configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||
operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" | ||
operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" | ||
corev1informers "k8s.io/client-go/informers/core/v1" | ||
|
||
"k8s.io/apimachinery/pkg/api/errors" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
corelistersv1 "k8s.io/client-go/listers/core/v1" | ||
) | ||
|
||
type AuthConfigChecker struct { | ||
authentications configv1informers.AuthenticationInformer | ||
authLister configv1listers.AuthenticationLister | ||
kubeAPIServers operatorv1informers.KubeAPIServerInformer | ||
kasLister operatorv1listers.KubeAPIServerLister | ||
kasNamespaceConfigMaps corev1informers.ConfigMapInformer | ||
kasConfigMapLister corelistersv1.ConfigMapLister | ||
} | ||
|
||
func NewAuthConfigChecker(authentications configv1informers.AuthenticationInformer, kubeapiservers operatorv1informers.KubeAPIServerInformer, configmaps corev1informers.ConfigMapInformer) AuthConfigChecker { | ||
return AuthConfigChecker{ | ||
authentications: authentications, | ||
kubeAPIServers: kubeapiservers, | ||
kasNamespaceConfigMaps: configmaps, | ||
authLister: authentications.Lister(), | ||
kasLister: kubeapiservers.Lister(), | ||
kasConfigMapLister: configmaps.Lister(), | ||
} | ||
} | ||
|
||
func (c *AuthConfigChecker) AuthConfig() (*configv1.Authentication, error) { | ||
return c.authLister.Get("cluster") | ||
} | ||
|
||
func (c *AuthConfigChecker) Authentications() configv1informers.AuthenticationInformer { | ||
return c.authentications | ||
} | ||
|
||
func (c *AuthConfigChecker) KubeAPIServers() operatorv1informers.KubeAPIServerInformer { | ||
return c.kubeAPIServers | ||
} | ||
|
||
func (c *AuthConfigChecker) KubeAPIServerNamespaceConfigMaps() corev1informers.ConfigMapInformer { | ||
return c.kasNamespaceConfigMaps | ||
} | ||
|
||
// OIDCAvailable checks the kubeapiservers/cluster resource for KAS pod | ||
// rollout status; it returns true if auth type is OIDC, all KAS pods are currently on a revision | ||
// that includes the structured auth-config ConfigMap, and the KAS args include the respective | ||
// arg that enables usage of the structured auth-config. It returns false otherwise. | ||
func (c *AuthConfigChecker) OIDCAvailable() (bool, error) { | ||
if auth, err := c.authLister.Get("cluster"); err != nil { | ||
return false, err | ||
} else if auth.Spec.Type != configv1.AuthenticationTypeOIDC { | ||
return false, nil | ||
} | ||
|
||
kas, err := c.kasLister.Get("cluster") | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
observedRevisions := sets.New[int32]() | ||
for _, nodeStatus := range kas.Status.NodeStatuses { | ||
observedRevisions.Insert(nodeStatus.CurrentRevision) | ||
} | ||
|
||
if observedRevisions.Len() == 0 { | ||
return false, nil | ||
} | ||
|
||
for _, revision := range observedRevisions.UnsortedList() { | ||
// ensure every observed revision includes an auth-config revisioned configmap | ||
_, err := c.kasConfigMapLister.ConfigMaps("openshift-kube-apiserver").Get(fmt.Sprintf("auth-config-%d", revision)) | ||
if errors.IsNotFound(err) { | ||
return false, nil | ||
} else if err != nil { | ||
return false, err | ||
} | ||
|
||
// every observed revision includes a copy of the KAS config configmap | ||
cm, err := c.kasConfigMapLister.ConfigMaps("openshift-kube-apiserver").Get(fmt.Sprintf("config-%d", revision)) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
// ensure the KAS config of every observed revision contains the appropriate CLI arg for OIDC | ||
// but not the respective ones for OAuth | ||
if !strings.Contains(cm.Data["config.yaml"], `"oauthMetadataFile":""`) || | ||
strings.Contains(cm.Data["config.yaml"], `"authentication-token-webhook-config-file":`) || | ||
!strings.Contains(cm.Data["config.yaml"], `"authentication-config":["/etc/kubernetes/static-pod-resources/configmaps/auth-config/auth-config.json"]`) { | ||
return false, nil | ||
} | ||
} | ||
|
||
return true, nil | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we actually need to keep a hold of the
*Informer
types here?I don't see anywhere they are used within the
AuthConfigChecker
or being accessed through the accessor methods on theAuthConfigChecker
. Did I miss something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AuthenticationInformer and KubeAPIServerInformer are accessed via the accessor methods in controllers that need to use them to trigger syncs. But indeed, the ConfigMapInformer one isn't used anywhere, this was probably a left over during development; I'll drop that one.