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

feat(authz): Authorino for Service Mesh #784

Merged
merged 37 commits into from
Feb 19, 2024
Merged

feat(authz): Authorino for Service Mesh #784

Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
134313a
feat(authz): Authorino for Service Mesh
israel-hdez Dec 11, 2023
2bd12d8
Fix linter issues
israel-hdez Jan 15, 2024
24a88c4
Resolve feedback: Bartosz
israel-hdez Jan 16, 2024
eb96386
fix: Remove port from the authorization policy
israel-hdez Jan 18, 2024
189d41a
Fix feedback: Bartosz
israel-hdez Jan 18, 2024
66f4dd8
More feedback: Bartosz
israel-hdez Jan 19, 2024
49a4df7
Fix feedback: Reto - Adjust AuthorizationPolicy
israel-hdez Jan 22, 2024
3a9324c
Fix more feedback: Bartosz
israel-hdez Jan 22, 2024
10f902e
chore: adds sec. prefix to authorino label selector
bartoszmajsak Jan 23, 2024
fe7a481
fix: adds base dir to manifest sources
bartoszmajsak Jan 23, 2024
ba0b12d
chore: uses security instead of sec as a prefix in authorino label
bartoszmajsak Jan 24, 2024
cc7db0e
Merge branch 'incubation' into authorino-setup
bartoszmajsak Jan 25, 2024
d17532c
fix: /healthz is called by _something_, skipp
aslakknutsen Jan 25, 2024
7f2ce38
fix: adopt ODH-ADR-0006 for clean up label
aslakknutsen Jan 25, 2024
2dff875
fix: uses correct CRD name for authconfigs
bartoszmajsak Jan 26, 2024
003b585
Merge branch 'incubation' into authorino-setup
bartoszmajsak Jan 26, 2024
9974aea
Merge branch 'incubation' into authorino-setup
israel-hdez Jan 29, 2024
44b0077
Remove left-over file
israel-hdez Jan 29, 2024
5408983
Feedback: remove auth-refs ConfigMap
israel-hdez Jan 31, 2024
e94e78f
Add missing role.yaml changes
israel-hdez Jan 31, 2024
07c2e70
Go back to installing Authorino on its own namespace
israel-hdez Feb 1, 2024
a7a17e7
Merge branch 'incubation' into authorino-setup
israel-hdez Feb 1, 2024
dfe2f17
Merge branch 'incubation' into authorino-setup
bartoszmajsak Feb 2, 2024
99dd030
Feedback: Add clean-up for KServe/OSSM-auth
israel-hdez Feb 5, 2024
773cef9
Feedback: Simplify namings
israel-hdez Feb 5, 2024
d3ec6a1
Merge branch 'incubation' into authorino-setup
aslakknutsen Feb 6, 2024
faede78
fix: add auth-refs cm
aslakknutsen Feb 5, 2024
a06b1fd
Feedback: adjust labels and a log message
israel-hdez Feb 14, 2024
fae0994
Bugfix: Extension provider terminating with error when SMCP is gone
israel-hdez Feb 14, 2024
b5fc2b2
Fix: add missing RBAC for ConfigMaps func
israel-hdez Feb 14, 2024
2d64844
Fix: Run `make bundle` and commit resulting changes
israel-hdez Feb 16, 2024
95163ef
Feedback: Wen - Better feature namings
israel-hdez Feb 16, 2024
bdeca2c
Feedback: Bartosz
israel-hdez Feb 16, 2024
e28da29
Feedback: Wen - revert image placeholder was replaced
israel-hdez Feb 16, 2024
1e5d734
Merge remote-tracking branch 'odh/incubation' into authorino-setup
israel-hdez Feb 16, 2024
c60ea4b
Merge branch 'incubation' into authorino-setup
israel-hdez Feb 19, 2024
abe7daa
Merge branch 'incubation' into authorino-rebase-bup
israel-hdez Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apis/dscinitialization/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 20 additions & 20 deletions bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -105,12 +105,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -149,12 +149,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -195,12 +195,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -298,12 +298,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -342,12 +342,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -385,12 +385,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -428,12 +428,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -471,12 +471,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -514,12 +514,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down
22 changes: 22 additions & 0 deletions bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,28 @@ spec:
user experience; e.g. it provides unified authentication giving
a Single Sign On experience.
properties:
auth:
description: Auth holds configuration of authentication and authorization
services used by Service Mesh in Opendatahub.
properties:
audiences:
default:
- https://kubernetes.default.svc
description: Audiences is a list of the identifiers that the
resource server presented with the token identifies as.
Audience-aware token authenticators will verify that the
token was intended for at least one of the audiences in
this list. If no audiences are provided, the audience will
default to the audience of the Kubernetes apiserver (kubernetes.default.svc).
items:
type: string
type: array
namespace:
description: Namespace where it is deployed. If not provided,
the default is to use '-auth-provider' suffix on the ApplicationsNamespace
of the DSCI.
type: string
type: object
controlPlane:
description: ControlPlane holds configuration of Service Mesh
used by Opendatahub.
Expand Down
27 changes: 26 additions & 1 deletion bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,12 @@ spec:
- tokenreviews
verbs:
- create
- apiGroups:
- authorino.kuadrant.io
resources:
- authconfigs
verbs:
- '*'
- apiGroups:
- authorization.k8s.io
resources:
Expand Down Expand Up @@ -538,6 +544,7 @@ spec:
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
Expand Down Expand Up @@ -1170,6 +1177,12 @@ spec:
- deletecollection
- get
- patch
- apiGroups:
- networking.istio.io
resources:
- envoyfilters
verbs:
- '*'
- apiGroups:
- networking.istio.io
resources:
Expand Down Expand Up @@ -1250,6 +1263,12 @@ spec:
- patch
- update
- watch
- apiGroups:
- operator.authorino.kuadrant.io
resources:
- authorinos
verbs:
- '*'
- apiGroups:
- operator.knative.dev
resources:
Expand Down Expand Up @@ -1390,6 +1409,12 @@ spec:
- patch
- update
- watch
- apiGroups:
- security.istio.io
resources:
- authorizationpolicies
verbs:
- '*'
- apiGroups:
- security.openshift.io
resources:
Expand Down Expand Up @@ -1752,7 +1777,7 @@ spec:
env:
- name: DISABLE_DSC_CONFIG
value: "true"
image: REPLACE_IMAGE:latest
image: quay.io/edgarhz/odh-operator:latest
zdtsw marked this conversation as resolved.
Show resolved Hide resolved
imagePullPolicy: Always
livenessProbe:
httpGet:
Expand Down
9 changes: 7 additions & 2 deletions components/kserve/kserve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,16 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client, resC
return err
}
}
return nil

return k.configureServiceMesh(dscispec)
}

func (k *Kserve) Cleanup(_ client.Client, instance *dsciv1.DSCInitializationSpec) error {
return k.removeServerlessFeatures(instance)
if removeServerlessErr := k.removeServerlessFeatures(instance); removeServerlessErr != nil {
return removeServerlessErr
}

return k.removeServiceMeshConfigurations(instance)
}

func (k *Kserve) configureServerless(instance *dsciv1.DSCInitializationSpec) error {
Expand Down
46 changes: 46 additions & 0 deletions components/kserve/servicemesh_setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package kserve

import (
"path"

operatorv1 "github.com/openshift/api/operator/v1"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/servicemesh"
)

func (k *Kserve) configureServiceMesh(dscispec *dsciv1.DSCInitializationSpec) error {
bartoszmajsak marked this conversation as resolved.
Show resolved Hide resolved
if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed {
serviceMeshInitializer := feature.ComponentFeaturesHandler(k, dscispec, k.defineServiceMeshFeatures())
return serviceMeshInitializer.Apply()
}
if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed {
return nil
}
Comment on lines +18 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be an unsupported state worth reporting somehow?

Copy link
Contributor Author

@israel-hdez israel-hdez Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My rationale was for the case when the user started with a Managed Mesh. Then, the user wanted to flip from Managed to Unmanaged so that odh-operator steps down and the user takes over on the setup.

Although, thinking more, probably this should also be applicable for the case when the DSCI is created with an unmanaged mesh from the beginning (i.e. they bring their own mesh). If this is the case, I think the user would be responsible for doing the setup of Mesh+Authorino+the filters involved here.


return k.removeServiceMeshConfigurations(dscispec)
}

func (k *Kserve) removeServiceMeshConfigurations(dscispec *dsciv1.DSCInitializationSpec) error {
Copy link
Member

Choose a reason for hiding this comment

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

i did a test: to set servicemesh to Removed, but the FT "configure-kserve-for-external-authz" did not get deleted. is this correct?
till i set kserve to Removed, FT are gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is correct nor incorrect, but the opposite to that 😄

i did a test: to set servicemesh to Removed

If there is no mesh, my expectation would be that the FT and its associated resources must be removed. I think we've been OK with dealing with the DSCI and the DSC separately. However, this is introducing some cohesion and, now, changes to the DSCI should trigger a reconcile to the DSC.

This is declared in the setup of the manager. I haven't reviewed that code.

You may do the following test: set ServiceMesh to Removed and, instead of setting KServe to Removed, simply add a dummy annotation to the DSC (to force its reconciliation). Most likely, you'll see that the FT will vanish.

Analyzing deeper, I think we shouldn't let the user to freely flip settings. E.g. setting the Mesh to Removed while setting Serverless to Managed should not be a valid state. With Vedant's PR #864 coming, the number of invalid states (and needed guards) increases.

I think we should be more proactive on helping the user and start thinking on adding a validating webhook, so that the user gets feedback on invalid configs when doing kubectl apply, rather than waiting for a reconciliation to happen and looking at the events/status of the DSCI/DSC. If you think this is worth to do, I can volunteer on doing the coding, since most (or all?) the things that require validation are on serving side.

Copy link
Contributor

Choose a reason for hiding this comment

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

With Vedant's PR #864 coming, the number of invalid states (and needed guards) increases.

We really need to bring validating webhook to handle such cases there instead. We could have a set of rules there and not even allow to save invalid DSC/DSCI on the cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

With Vedant's PR #864 coming, the number of invalid states (and needed guards) increases.

We really need to bring validating webhook to handle such cases there instead. We could have a set of rules there and not even allow to save invalid DSC/DSCI on the cluster.

webhook with one simple check has been in review for a while and you even looked in it (#711)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

webhook with one simple check has been in review for a while and you even looked in it (#711)

Ah, this is good.
So, once it is merged, I just need to add serving-specific validations.

Copy link
Contributor

Choose a reason for hiding this comment

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

webhook with one simple check has been in review for a while and you even looked in it (#711)

Ah, this is good. So, once it is merged, I just need to add serving-specific validations.

Join the review ;)

Copy link
Member

Choose a reason for hiding this comment

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

#711 is now merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, too late to join the review 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

With Vedant's PR #864 coming, the number of invalid states (and needed guards) increases.

We really need to bring validating webhook to handle such cases there instead. We could have a set of rules there and not even allow to save invalid DSC/DSCI on the cluster.

webhook with one simple check has been in review for a while and you even looked in it (#711)

I know I did :) it's my subtle attempt to make it shipped to incubation faster. It seems it worked.

serviceMeshInitializer := feature.ComponentFeaturesHandler(k, dscispec, k.defineServiceMeshFeatures())
return serviceMeshInitializer.Delete()
}

func (k *Kserve) defineServiceMeshFeatures() feature.FeaturesProvider {
return func(handler *feature.FeaturesHandler) error {
kserveExtAuthzErr := feature.CreateFeature("configure-kserve-for-external-authz").
zdtsw marked this conversation as resolved.
Show resolved Hide resolved
For(handler).
Manifests(
path.Join(feature.KServeDir),
).
WithData(servicemesh.ClusterDetails).
Load()

if kserveExtAuthzErr != nil {
return kserveExtAuthzErr
}

return nil
}
}
22 changes: 22 additions & 0 deletions config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,28 @@ spec:
user experience; e.g. it provides unified authentication giving
a Single Sign On experience.
properties:
auth:
zdtsw marked this conversation as resolved.
Show resolved Hide resolved
description: Auth holds configuration of authentication and authorization
services used by Service Mesh in Opendatahub.
properties:
audiences:
default:
- https://kubernetes.default.svc
description: Audiences is a list of the identifiers that the
resource server presented with the token identifies as.
Audience-aware token authenticators will verify that the
token was intended for at least one of the audiences in
this list. If no audiences are provided, the audience will
default to the audience of the Kubernetes apiserver (kubernetes.default.svc).
items:
type: string
type: array
namespace:
description: Namespace where it is deployed. If not provided,
the default is to use '-auth-provider' suffix on the ApplicationsNamespace
of the DSCI.
type: string
type: object
controlPlane:
description: ControlPlane holds configuration of Service Mesh
used by Opendatahub.
Expand Down
Loading