-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: create project level authorino authconfig #64
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,9 @@ type reconcileFunc func(ctx context.Context, namespace *v1.Namespace) error | |
func (r *OpenshiftServiceMeshReconciler) reconcileAuthConfig(ctx context.Context, namespace *v1.Namespace) error { | ||
log := r.Log.WithValues("feature", "authorino", "namespace", namespace.Name) | ||
|
||
desiredAuthConfig, err := r.createAuthConfig(namespace, namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost]) | ||
desiredAuthConfig, err := r.createAuthConfig(namespace, | ||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternExternal], | ||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternInternal]) | ||
if err != nil { | ||
log.Error(err, "Failed creating AuthConfig object") | ||
|
||
|
@@ -103,16 +105,7 @@ func (r *OpenshiftServiceMeshReconciler) createAuthConfig(namespace *v1.Namespac | |
|
||
authConfig.Labels[keyValue[0]] = keyValue[1] | ||
authConfig.Spec.Hosts = authHosts | ||
|
||
// Reflects oauth-proxy SAR settings | ||
authConfig.Spec.Authorization[0].KubernetesAuthz.ResourceAttributes = &authorino.Authorization_KubernetesAuthz_ResourceAttributes{ //nolint:nosnakecase //reason external library | ||
Namespace: authorino.StaticOrDynamicValue{Value: namespace.Name}, | ||
Group: authorino.StaticOrDynamicValue{Value: "kubeflow.org"}, | ||
Resource: authorino.StaticOrDynamicValue{Value: "notebooks"}, | ||
Verb: authorino.StaticOrDynamicValue{Value: "get"}, | ||
} | ||
|
||
authConfig.Spec.Identity[0].KubernetesAuth.Audiences = getAuthAudience() | ||
authConfig.Spec.Identity[0].KubernetesAuth.Audiences = []string{namespace.Name + "-api"} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would it work for e.g. project with only workbench enabled? At this point it expects the user to be able to "get" notebooks in this namespace. It's a similar approach in other components IIRC. Do we foresee some granularity of the roles? Can you help me understand how is this supposed to replace it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is all Host based. Workbenchs + Notebooks are bound to the "*.opendatahub.openshift.ai" domain which is the openatahub level authconfig with oauth/single sign on configured. Which namespace they belong to doesn't matter. Models are bound to a various of different domains, aka ".namespace.srv.cluster.local", ".namespace.apps.openshift.ai". These are configured in the project level authconfig. In the current impl the ProjectLevel authconfig would still exist in the Project namespace if only workbenches are used, but the workbench is reached on different URL pattern so it won't be active. |
||
|
||
return authConfig, nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"reflect" | ||
"strconv" | ||
|
||
configv1 "github.com/openshift/api/config/v1" | ||
routev1 "github.com/openshift/api/route/v1" | ||
"github.com/pkg/errors" | ||
v1 "k8s.io/api/core/v1" | ||
|
@@ -120,8 +121,6 @@ func (r *OpenshiftServiceMeshReconciler) findIstioIngress(ctx context.Context) ( | |
LabelSelector: labels.SelectorFromSet(labels.Set{"app": "odh-dashboard"}), | ||
Namespace: meshNamespace, | ||
}); err != nil { | ||
r.Log.Error(err, "Unable to find matching gateway") | ||
|
||
return routev1.RouteList{}, errors.Wrap(err, "unable to find matching gateway") | ||
} | ||
|
||
|
@@ -136,3 +135,18 @@ func (r *OpenshiftServiceMeshReconciler) findIstioIngress(ctx context.Context) ( | |
|
||
return routes, nil | ||
} | ||
|
||
func (r *OpenshiftServiceMeshReconciler) findAppDomain(ctx context.Context) (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is fetching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not the only way to find it, but it's the controlling value location. |
||
ingress := configv1.Ingress{} | ||
|
||
err := r.Client.Get(ctx, types.NamespacedName{Name: "cluster"}, &ingress) | ||
if err != nil { | ||
return "", errors.Wrap(err, "unable to find matching ingress config cluster") | ||
} | ||
|
||
if ingress.Spec.AppsDomain != "" { | ||
return ingress.Spec.AppsDomain, nil | ||
} | ||
|
||
return ingress.Spec.Domain, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,17 +33,6 @@ func getAuthorinoLabel() ([]string, error) { | |
return keyValue, nil | ||
} | ||
|
||
func getAuthAudience() []string { | ||
aud := getEnvOr(AuthAudience, "https://kubernetes.default.svc") | ||
audiences := strings.Split(aud, ",") | ||
|
||
for i := range audiences { | ||
audiences[i] = strings.TrimSpace(audiences[i]) | ||
} | ||
|
||
return audiences | ||
} | ||
|
||
Comment on lines
-36
to
-46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this was introduced specifically to address the case when OpenDataHub runs e.g. on ROSA environment and we need to use third-party token issuers in Istio (so also needs this config change in the cluster: https://access.redhat.com/documentation/en-us/openshift_container_platform/4.8/html-single/service_mesh/index#ossm-install-rosa_ossm-create-smcp) Won't we need it with the new approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be up to the user if needed. Depends on the external server they are integrating this with. It's certainly not a install wide option anymore. |
||
func getEnvOr(key, defaultValue string) string { | ||
if env, defined := os.LookupEnv(key); defined { | ||
return env | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
package controllers | ||
|
||
const ( | ||
AnnotationServiceMesh = "opendatahub.io/service-mesh" | ||
AnnotationPublicGatewayName = "service-mesh.opendatahub.io/public-gateway-name" | ||
AnnotationPublicGatewayExternalHost = "service-mesh.opendatahub.io/public-gateway-host-external" | ||
AnnotationPublicGatewayInternalHost = "service-mesh.opendatahub.io/public-gateway-host-internal" | ||
LabelMaistraGatewayName = "maistra.io/gateway-name" | ||
LabelMaistraGatewayNamespace = "maistra.io/gateway-namespace" | ||
AnnotationServiceMesh = "opendatahub.io/service-mesh" | ||
AnnotationPublicGatewayName = "service-mesh.opendatahub.io/public-gateway-name" | ||
AnnotationPublicGatewayExternalHost = "service-mesh.opendatahub.io/public-gateway-host-external" | ||
AnnotationPublicGatewayInternalHost = "service-mesh.opendatahub.io/public-gateway-host-internal" | ||
AnnotationProjectModelGatewayHostPatternExternal = "service-mesh.opendatahub.io/model-gateway-hostpattern-external" | ||
AnnotationProjectModelGatewayHostPatternInternal = "service-mesh.opendatahub.io/model-gateway-hostpattern-internal" | ||
LabelMaistraGatewayName = "maistra.io/gateway-name" | ||
LabelMaistraGatewayNamespace = "maistra.io/gateway-namespace" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,9 @@ import ( | |
func (r *OpenshiftServiceMeshReconciler) addGatewayAnnotations(ctx context.Context, namespace *v1.Namespace) error { | ||
if namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] != "" && | ||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] != "" && | ||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayName] != "" { | ||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayName] != "" && | ||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternExternal] != "" && | ||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternInternal] != "" { | ||
Comment on lines
13
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's getting a bit hard to read now. Maybe we can have a func such as
which will improve readability? |
||
// If annotation is present we have nothing to do | ||
return nil | ||
} | ||
|
@@ -24,9 +26,19 @@ func (r *OpenshiftServiceMeshReconciler) addGatewayAnnotations(ctx context.Conte | |
return err | ||
} | ||
|
||
appDomain, err := r.findAppDomain(ctx) | ||
if err != nil { | ||
r.Log.Error(err, "unable to find app domain") | ||
|
||
return err | ||
} | ||
|
||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayExternalHost] = ExtractHostName(routes.Items[0].Spec.Host) | ||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayInternalHost] = fmt.Sprintf("%s.%s.svc.cluster.local", routes.Items[0].Spec.To.Name, getMeshNamespace()) | ||
|
||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternExternal] = fmt.Sprintf("*.%s.%s", namespace.Name, appDomain) | ||
namespace.ObjectMeta.Annotations[AnnotationProjectModelGatewayHostPatternInternal] = fmt.Sprintf("*.%s.svc.cluster.local", namespace.Name) | ||
|
||
gateway := extractGateway(routes.Items[0].ObjectMeta) | ||
if gateway != "" { | ||
namespace.ObjectMeta.Annotations[AnnotationPublicGatewayName] = gateway | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,74 @@ | ||
apiVersion: authorino.kuadrant.io/v1beta1 | ||
kind: AuthConfig | ||
metadata: | ||
name: odh-dashboard-protection | ||
name: $ODH_NS-protection | ||
namespace: $ODH_NS | ||
labels: | ||
authorino/topic: odh | ||
spec: | ||
hosts: | ||
- "${ODH_ROUTE}" | ||
- "${ODH_ROUTE}" | ||
identity: | ||
- name: kubernetes-users | ||
kubernetes: | ||
audiences: | ||
- "https://kubernetes.default.svc" | ||
- name: authorized-service-accounts | ||
kubernetes: | ||
audiences: | ||
- $ODH_NS-api | ||
- name: anonymous-grpc | ||
priority: 1 | ||
anonymous: {} | ||
when: | ||
- selector: context.request.http.headers.mm-vmodel-id | ||
operator: matches | ||
value: .+ | ||
extendedProperties: | ||
- name: modelid | ||
valueFrom: | ||
authJSON: context.request.http.headers.mm-vmodel-id | ||
- name: anonymous-http | ||
priority: 1 | ||
anonymous: {} | ||
when: | ||
- selector: context.request.http.headers.mm-vmodel-id | ||
operator: matches | ||
value: ^$ | ||
extendedProperties: | ||
- name: modelid | ||
valueFrom: | ||
authJSON: context.request.http.path.@extract:{"sep":"/","pos":2} | ||
metadata: | ||
- name: access | ||
http: | ||
endpoint: http://odh-model-controller-metrics-service.opendatahub.svc.cluster.local:8080/model/?ns={context.request.http.host.@extract:{"sep":".","pos":0}}&modelid={auth.identity.modelid} | ||
#cache: | ||
# key: | ||
# valueFrom: | ||
# authJSON: {context.request.http.host.@extract:{"sep":".","pos":0}}-{auth.identity.modelid} | ||
# ttl: 300 | ||
when: | ||
- selector: auth.identity.anonymous | ||
operator: eq | ||
value: "true" | ||
authorization: | ||
- name: k8s-rbac | ||
kubernetes: | ||
user: | ||
valueFrom: { authJSON: auth.identity.username } | ||
response: | ||
- name: x-auth-data | ||
- name: anonymous | ||
when: | ||
- selector: auth.identity.anonymous | ||
operator: eq | ||
value: "true" | ||
json: | ||
properties: | ||
- name: username | ||
valueFrom: { authJSON: auth.identity.username } | ||
rules: | ||
- selector: auth.metadata.access.anonymous | ||
operator: eq | ||
value: "true" | ||
denyWith: | ||
unauthenticated: | ||
message: | ||
value: "Access denied" | ||
code: 302 | ||
headers: | ||
- name: Location | ||
valueFrom: | ||
authJSON: http://some-service?redirect_to={context.request.http.path} | ||
unauthorized: | ||
message: | ||
value: "Unauthorized" | ||
code: 403 | ||
headers: | ||
- name: Location | ||
valueFrom: | ||
authJSON: http://some-service?redirect_to={context.request.http.path} | ||
Comment on lines
+68
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we planning to change those There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit of an open question. I think in the current form these are more "examples" then real values as they are dummy and the chosen auth form does not support a kind of "start flow" type thing. This is now a User controlled AuthConfig. If they use some external auth server, they could choose to auto redirect there if the request is not auth to e.g. to start the oauth flow. But maybe these should be commented out or moved to docs. |
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.