feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

bartoszmajsak · 2023-10-05T17:31:30Z

⚠️ Important

Code walk-through https://www.youtube.com/watch?v=n7vqYYVfaTg

This code is a port of feat: introduces OSSM plugin for KfDef #515. Core functionalities around templates and packages are unchanged.
See the extended checklist of Merge Criteria at the bottom of this PR description.

Description

Enables service mesh for dashboard and workbench components (part of #496).

It addresses several limitations we encountered when using the initial kustomize/manifest and init-job solution, specifically:

Autoconfiguration: Simplifies the process of obtaining cluster-specific data for auto-configuring authorization settings.
Reversible Changes: Enables flexible and easy reversion of applied configurations.
- Utilizes owner references wherever applicable.
- Enables custom cleanup logic hooks implemented in Golang, such as undoing patches.
Ease of Integration: Allows a straightforward addition to Component model

Key features

configures Authorino as an external authorization provider for Service Mesh
supports OAuth flow through EnvoyFilters for Dashboard and Workbenches
sets up required Istio resources for each component
deploys odh-project-controller which coordinates Service Mesh setup for Data Science Projects

Additional improvements

Finalizers to DSC and DSCI (based on Add finalizer for DSCI and DSC #301 which can be closed)
- there's default no-op Cleanup implementation for each component
- dashboard.go has it's own logic which will be invoked by reconciler on deletion
secret became it's own package with few enhancements as it's need for service mesh.
Service Mesh integration tests
- added EnvTest helpers and unified with existing tests
- instead of comparing platform == "" there's now a meaningful constant platform == deploy.Unknown

Enabling it

See docs/SERVICE-MESH.md

How Has This Been Tested?

make test will run all tests, including OSSM features such as patching resources or cleaning up changes on deletion.

We did extensive manual testing as well.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

Additional merge criteria/dependencies:

feat(ossm): adds OSSM annotations to the relevant cluster resources odh-dashboard#1088
feat: disables features not needed for Istio deployments kubeflow#172
ensure we are using official manifests for Kubeflow - change reference in get_all_manifests.sh file.

openshift-ci · 2023-10-05T17:31:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign etirelli for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bartoszmajsak · 2023-10-05T17:36:35Z

⚠️ Known issue

While testing those changes back and forth we discovered a regression affecting notebook pods created in the Application Namespace (so the same as controllers). It has a permission issue resulting in CrashLoopBackOff of the Jupyter container (which has istio-proxy sidecar injected):

PermissionError: [Errno 13] Permission denied: '/opt/app-root/src/.local'

It does work however when the same Jupyter workbench is created in a new namespace (Data Science Project).

The investigation is ongoing.

Here are the pod details of both scenarios:

workbench in its own ns: https://gist.github.com/bartoszmajsak/5b12ffd4301d7326fd2a704c9481381a
workbench in app namespace (opendatahub): https://gist.github.com/bartoszmajsak/a29a39b19a4b5690c029c90262d25a82

WORKAROUND

Use SMCP explicitly set to v2.3 to get this working.

Related issue: #616

cam-garrison · 2023-10-05T18:12:39Z

⚠️ Known issue

After deploying the data science cluster, if the user tries to log in to the dashboard quickly after the dashboard pod(s) are ready, they may hit an error page after login saying:

Danger alert:General loading error
Cannot read properties of undefined (reading 'spec')

Logging out and logging back in may solve the issue.

Backend logs:

{"level":50,"time":1696518565791,"pid":17,"hostname":"odh-dashboard-958bc566b-2qstq","msg":"Error writing log. Cannot read properties of undefined (reading 'spec')"}
{"level":50,"time":1696518565792,"pid":17,"hostname":"odh-dashboard-958bc566b-2qstq","reqId":"req-u","req":{"method":"GET","url":"/api/builds","hostname":"opendatahub.apps-crc.testing","remoteAddress":"127.0.0.6","remotePort":53401},"res":{"statusCode":500},"err":{"type":"TypeError","message":"Cannot read properties of undefined (reading 'spec')","stack":"TypeError: Cannot read properties of undefined (reading 'spec')\n    at getNamespaces (/usr/src/app/backend/dist/utils/notebookUtils.js:30:44)\n    at /usr/src/app/backend/dist/utils/route-security.js:22:70\n    at Generator.next (<anonymous>)\n    at fulfilled (/usr/src/app/backend/dist/utils/route-security.js:5:58)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"},"msg":"Cannot read properties of undefined (reading 'spec')"}

Simply closing the tab, waiting 30 seconds (not a scientific value, YMMV) and reopening the route to the dashboard through the mesh should resolve the issue.

Will have to figure out if this is already a known issue with dashboard experts.

zdtsw · 2023-10-05T18:13:52Z

docs/SERVICE-MESH.md

+apiVersion: maistra.io/v2
+kind: ServiceMeshControlPlane
+metadata:
+  name: minimal


should be "basic"? or changed default value

This is just an example, and the name is more accurate in my eyes. Open for discussion.

A more descriptive name, perhaps identifying this SMCP belonging to this product would be a good differentiator given that there could be multiple SMCPs deployed on a single cluster.

zdtsw · 2023-10-05T18:19:44Z

apis/dscinitialization/v1/servicemesh_types.go

+	Label string `json:"label,omitempty"`
+	// Image allows to define a custom container image to be used when deploying Authorino's instance.
+	// +kubebuilder:default="quay.io/kuadrant/authorino:v0.13.0"
+	Image string `json:"image,omitempty"`


not really sure what this image is here for.

Perhaps more a DevFlag thing in the retrospect. Up for discussion. This new part of the spec is a slimmed version of #515

components/workbenches/workbenches.go

get_all_manifests.sh

bartoszmajsak · 2023-10-06T06:33:09Z

There's one limitation we need to fix #617

mattmahoneyrh · 2023-10-06T14:58:29Z

docs/SERVICE-MESH.md

+  name: $name
+  source: $source
+  sourceNamespace: openshift-marketplace
+EOF"    


Is the subscription created in openshift-marketplace, or should it be openshift-opeators?

Thanks for raising this concern. This just an example for devs on how to test it, not an official user guide. Perhaps it should be emphasized in the opening section.

We should make sure we refer to the right distribution channels when providing a final end user guide @cam-garrison

as for now:

for upstream, our operator is called OpenDataHub from community-operators on fast channel

for downstream, the operator is called Red Hat OpenShift Data Science from redhat-operators on alpha channel

mattmahoneyrh · 2023-10-06T15:48:00Z

docs/SERVICE-MESH.md

+      name: kiali
+      enabled: false
+EOF
+```


I sanity tested the spec, with the result being that the SMCP deployed fine with the above spec.

Note: "name: kiali" is not needed, the SMCP deployed fine without the name.

Extracts the fluent interface for Features from PR opendatahub-io#605. This allows other components to configure cluster resources using this interface before the original PR gets merged. No changes to the reconcile logic have been introduced.

* initial add tracker spec * update tests, update crd * add omitempty to origin struct * undo accidental tag change * re add empty line * move pointer operator * add testing * lint * re-lint changes * add ownertype, move newOrigin() to shared util * Update apis/features/v1/features_types.go Co-authored-by: Bartosz Majsak <[email protected]> * remove origin from featureinitializer * modify kserve sm step to match dashboard's * make dsci servicemesh setup like dashboard's * fix merge issues, lint --------- Co-authored-by: Bartosz Majsak <[email protected]>

openshift-merge-robot · 2024-01-12T18:17:04Z

PR needs rebase.

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/test-infra repository.

@bartoszmajsak

This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]>

@bartoszmajsak

This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]>

guicassolato · 2024-01-19T09:12:46Z

pkg/feature/templates/servicemesh/authorino/base/authconfig.tmpl

+    success:
+      headers:
+        x-auth-data:
+          json:


This is fine, but only so you know. Authorino also has plain now, in case you prefer just the username as the value in the header, rather than a stringified JSON.

Thanks @guicassolato

@bartoszmajsak

This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]>

@bartoszmajsak

This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]>

walkErr is propagated as part of recursive call when traversing directory tree. This adds a guard check and not proceed with the error happened in the call chain.

@bartoszmajsak

* feat(authz): Authorino for Service Mesh This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request #605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]> * Fix linter issues Signed-off-by: Edgar Hernández <[email protected]> * Resolve feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * fix: Remove port from the authorization policy Also, add `/metrics` to the ignored paths for auth. Signed-off-by: Edgar Hernández <[email protected]> * Fix feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * More feedback: Bartosz Co-authored-by: Bartosz Majsak <[email protected]> * Fix feedback: Reto - Adjust AuthorizationPolicy Signed-off-by: Edgar Hernández <[email protected]> * Fix more feedback: Bartosz - Remove Authorino namespace field from DSCI. - Move around some code in kserve.go to servicemesh_setup.go Signed-off-by: Edgar Hernández <[email protected]> * chore: adds sec. prefix to authorino label selector * fix: adds base dir to manifest sources * chore: uses security instead of sec as a prefix in authorino label * fix: /healthz is called by _something_, skipp * fix: adopt ODH-ADR-0006 for clean up label * fix: uses correct CRD name for authconfigs Co-authored-by: Cameron Garrison <[email protected]> * Remove left-over file Signed-off-by: Edgar Hernández <[email protected]> * Feedback: remove auth-refs ConfigMap Signed-off-by: Edgar Hernández <[email protected]> * Add missing role.yaml changes Signed-off-by: Edgar Hernández <[email protected]> * Go back to installing Authorino on its own namespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Add clean-up for KServe/OSSM-auth Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Simplify namings Signed-off-by: Edgar Hernández <[email protected]> * fix: add auth-refs cm * Feedback: adjust labels and a log message Signed-off-by: Edgar Hernández <[email protected]> * Bugfix: Extension provider terminating with error when SMCP is gone Signed-off-by: Edgar Hernández <[email protected]> * Fix: add missing RBAC for ConfigMaps func Signed-off-by: Edgar Hernández <[email protected]> * Fix: Run `make bundle` and commit resulting changes Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - Better feature namings Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Bartosz * Use feature logger * Don't trim -applications suffix on ResolveAuthNamespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - revert image placeholder was replaced Signed-off-by: Edgar Hernández <[email protected]> --------- Signed-off-by: Edgar Hernández <[email protected]> Co-authored-by: Bartosz Majsak <[email protected]> Co-authored-by: Aslak Knutsen <[email protected]> Co-authored-by: Cameron Garrison <[email protected]>

This test ensures additional of extension provider for external authorization and that it is removed from the control plane properly using custom cleanup function. NOTE: it is now failing to demonstrate existing bug ``` [FAILED] Timed out after 5.000s. Expected <[]interface {} | len:1, cap:1>: [ <map[string]interface {} | len:2>{ "envoyExtAuthzGrpc": <map[string]interface {} | len:2>{ "port": <int64>50051, "service": <string>"authorino-authorino-authorization.auth-provider.svc.cluster.local", }, "name": <string>"test-ns-auth-provider", }, ] to be empty ```

…uring cleanup (#905) ### Renames migration folder The reason for this is to have a simple naming convention instead of suggesting storing migration patches in dedicated folders named after tickets. Additionally, the feature explicitly orders files instead of assuming that the underlying fsys implementation fulfills such a contract. ### Ports #605 test for extension provider This test ensures the addition of an extension provider for external authorization and that it is removed from the control plane properly using a custom cleanup function. We have missed it in the original work. ### Fix: aligns provider name between template and cleanup logic This is short-term fix for the existing codebase. In the long term (which is actively worked on) we need to improve the way of how we are storing config information to limit cases where we rely on pre/suffixes. Cases like this should be kept as its own thing instead, as it represents the concept in the infrastructure/authz setup.

@bartoszmajsak

* feat(authz): Authorino for Service Mesh This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]> * Fix linter issues Signed-off-by: Edgar Hernández <[email protected]> * Resolve feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * fix: Remove port from the authorization policy Also, add `/metrics` to the ignored paths for auth. Signed-off-by: Edgar Hernández <[email protected]> * Fix feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * More feedback: Bartosz Co-authored-by: Bartosz Majsak <[email protected]> * Fix feedback: Reto - Adjust AuthorizationPolicy Signed-off-by: Edgar Hernández <[email protected]> * Fix more feedback: Bartosz - Remove Authorino namespace field from DSCI. - Move around some code in kserve.go to servicemesh_setup.go Signed-off-by: Edgar Hernández <[email protected]> * chore: adds sec. prefix to authorino label selector * fix: adds base dir to manifest sources * chore: uses security instead of sec as a prefix in authorino label * fix: /healthz is called by _something_, skipp * fix: adopt ODH-ADR-0006 for clean up label * fix: uses correct CRD name for authconfigs Co-authored-by: Cameron Garrison <[email protected]> * Remove left-over file Signed-off-by: Edgar Hernández <[email protected]> * Feedback: remove auth-refs ConfigMap Signed-off-by: Edgar Hernández <[email protected]> * Add missing role.yaml changes Signed-off-by: Edgar Hernández <[email protected]> * Go back to installing Authorino on its own namespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Add clean-up for KServe/OSSM-auth Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Simplify namings Signed-off-by: Edgar Hernández <[email protected]> * fix: add auth-refs cm * Feedback: adjust labels and a log message Signed-off-by: Edgar Hernández <[email protected]> * Bugfix: Extension provider terminating with error when SMCP is gone Signed-off-by: Edgar Hernández <[email protected]> * Fix: add missing RBAC for ConfigMaps func Signed-off-by: Edgar Hernández <[email protected]> * Fix: Run `make bundle` and commit resulting changes Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - Better feature namings Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Bartosz * Use feature logger * Don't trim -applications suffix on ResolveAuthNamespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - revert image placeholder was replaced Signed-off-by: Edgar Hernández <[email protected]> --------- Signed-off-by: Edgar Hernández <[email protected]> Co-authored-by: Bartosz Majsak <[email protected]> Co-authored-by: Aslak Knutsen <[email protected]> Co-authored-by: Cameron Garrison <[email protected]> (cherry picked from commit e32a7c2)

…uring cleanup (opendatahub-io#905) ### Renames migration folder The reason for this is to have a simple naming convention instead of suggesting storing migration patches in dedicated folders named after tickets. Additionally, the feature explicitly orders files instead of assuming that the underlying fsys implementation fulfills such a contract. ### Ports opendatahub-io#605 test for extension provider This test ensures the addition of an extension provider for external authorization and that it is removed from the control plane properly using a custom cleanup function. We have missed it in the original work. ### Fix: aligns provider name between template and cleanup logic This is short-term fix for the existing codebase. In the long term (which is actively worked on) we need to improve the way of how we are storing config information to limit cases where we rely on pre/suffixes. Cases like this should be kept as its own thing instead, as it represents the concept in the infrastructure/authz setup.

@bartoszmajsak

* Update bundle * feat(authz): Authorino for Service Mesh (#784) * feat(authz): Authorino for Service Mesh This first iteration is to cover authentication needs for KServe * Add templates to install Authorino * Add templates to configure Service Mesh to use Authorino to delegate Authorization * Add KServe-specific templates add ability to secure KServe Inference Services * Add relevant fields to DSCInitialization resource * Code for proper cleanup, in case of uninstalling Most (if not all) of this code comes from pull request opendatahub-io#605. Attribution to original authors: @bartoszmajsak, @aslakknutsen, @cam-garrison, et. al. Related opendatahub-io/kserve#128 Signed-off-by: Edgar Hernández <[email protected]> * Fix linter issues Signed-off-by: Edgar Hernández <[email protected]> * Resolve feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * fix: Remove port from the authorization policy Also, add `/metrics` to the ignored paths for auth. Signed-off-by: Edgar Hernández <[email protected]> * Fix feedback: Bartosz Signed-off-by: Edgar Hernández <[email protected]> * More feedback: Bartosz Co-authored-by: Bartosz Majsak <[email protected]> * Fix feedback: Reto - Adjust AuthorizationPolicy Signed-off-by: Edgar Hernández <[email protected]> * Fix more feedback: Bartosz - Remove Authorino namespace field from DSCI. - Move around some code in kserve.go to servicemesh_setup.go Signed-off-by: Edgar Hernández <[email protected]> * chore: adds sec. prefix to authorino label selector * fix: adds base dir to manifest sources * chore: uses security instead of sec as a prefix in authorino label * fix: /healthz is called by _something_, skipp * fix: adopt ODH-ADR-0006 for clean up label * fix: uses correct CRD name for authconfigs Co-authored-by: Cameron Garrison <[email protected]> * Remove left-over file Signed-off-by: Edgar Hernández <[email protected]> * Feedback: remove auth-refs ConfigMap Signed-off-by: Edgar Hernández <[email protected]> * Add missing role.yaml changes Signed-off-by: Edgar Hernández <[email protected]> * Go back to installing Authorino on its own namespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Add clean-up for KServe/OSSM-auth Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Simplify namings Signed-off-by: Edgar Hernández <[email protected]> * fix: add auth-refs cm * Feedback: adjust labels and a log message Signed-off-by: Edgar Hernández <[email protected]> * Bugfix: Extension provider terminating with error when SMCP is gone Signed-off-by: Edgar Hernández <[email protected]> * Fix: add missing RBAC for ConfigMaps func Signed-off-by: Edgar Hernández <[email protected]> * Fix: Run `make bundle` and commit resulting changes Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - Better feature namings Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Bartosz * Use feature logger * Don't trim -applications suffix on ResolveAuthNamespace Signed-off-by: Edgar Hernández <[email protected]> * Feedback: Wen - revert image placeholder was replaced Signed-off-by: Edgar Hernández <[email protected]> --------- Signed-off-by: Edgar Hernández <[email protected]> Co-authored-by: Bartosz Majsak <[email protected]> Co-authored-by: Aslak Knutsen <[email protected]> Co-authored-by: Cameron Garrison <[email protected]> (cherry picked from commit e32a7c2) * fix(authz): Fix broken external auth configuration There are two misconfigurations being fixed: * In the SMCP, the service hostname of Authorino was coded with `-authorization` suffix, but the right suffix is `-authorino-authorization`. * In the `kserve-predictor` AuthorizationPolicy, the hardcoded `opendatahub-odh-auth-provider` provider name was used, but it should have been the template `{{ .AppNamespace }}-auth-provider`. In `pkg/feature/feature.go` the patch manifests (i.e. the ones containing `.patch` in the filename) are always applied. Thus, the first bullet is solved by fixing the patch file that adds the `extensionProvider` to the SMCP. For the second bullet, the faulty AuthorizationPolicy is created with a regular manifest template which is only applied if the resource does not exist. Thus, a patch manifest is added to properly fix the faulty policy (including operator upgrades). Signed-off-by: Edgar Hernández <[email protected]> (cherry picked from commit e4252a0) * fix: Rework operator precondition checks (#899) * init commit * tmp: switch to subsciption * tmp * fix up testing * linter on import * minor self nits * add bracket, make * use found,err for checking subscription Co-authored-by: Bartosz Majsak <[email protected]> * fix import + test error expected outputs * directly return errs rather than log and ret Co-authored-by: Bartosz Majsak <[email protected]> * remove unused log var from condiitons * move const fixtures to separate package * move creating op subscription to function * rename noop features in testing * remove redundant comments Co-authored-by: Bartosz Majsak <[email protected]> * move CreateSubscription to fixtures --------- Co-authored-by: Bartosz Majsak <[email protected]> (cherry picked from commit f44528e) * chore: follow up review comments from previous PR (#858) * update: follow up comments - cleanup commented out code - rename function - cleanup unnecessary sleep Signed-off-by: Wen Zhou <[email protected]> * update: add check on return err + remove apierrs.IsNotFound check Signed-off-by: Wen Zhou <[email protected]> * Update pkg/deploy/deploy.go Co-authored-by: Bartosz Majsak <[email protected]> * update(review): create new function DeleteSubscription Signed-off-by: Wen Zhou <[email protected]> * update: return for get and delete subscription - get: return 'sub, nil' or 'nil, err' here error can be real one or notfound Signed-off-by: Wen Zhou <[email protected]> * Update pkg/deploy/deploy.go Co-authored-by: Bartosz Majsak <[email protected]> * fix(linter) Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> Co-authored-by: Bartosz Majsak <[email protected]> (cherry picked from commit a81a3da) * fix(authz): ensures extauthz provider is removed from control plane during cleanup (#905) ### Renames migration folder The reason for this is to have a simple naming convention instead of suggesting storing migration patches in dedicated folders named after tickets. Additionally, the feature explicitly orders files instead of assuming that the underlying fsys implementation fulfills such a contract. ### Ports #605 test for extension provider This test ensures the addition of an extension provider for external authorization and that it is removed from the control plane properly using a custom cleanup function. We have missed it in the original work. ### Fix: aligns provider name between template and cleanup logic This is short-term fix for the existing codebase. In the long term (which is actively worked on) we need to improve the way of how we are storing config information to limit cases where we rely on pre/suffixes. Cases like this should be kept as its own thing instead, as it represents the concept in the infrastructure/authz setup. * chore: indentation Signed-off-by: Wen Zhou <[email protected]> * fix: use old package path till we cherry-pick refactor commit Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> Co-authored-by: Edgar Hernández <[email protected]> Co-authored-by: Edgar Hernández <[email protected]> Co-authored-by: Cameron Garrison <[email protected]> Co-authored-by: Wen Zhou <[email protected]> Co-authored-by: Bartosz Majsak <[email protected]>

openshift-ci · 2024-04-24T16:46:44Z

@bartoszmajsak: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-index	`98049d8`	link	true	`/test ci-index`
ci/prow/opendatahub-operator-pr-image-mirror	`98049d8`	link	true	`/test opendatahub-operator-pr-image-mirror`
ci/prow/images	`98049d8`	link	true	`/test images`
ci/prow/opendatahub-operator-e2e	`98049d8`	link	true	`/test opendatahub-operator-e2e`

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/test-infra repository. I understand the commands that are listed here.

ReplaceChar is not used anywhere in the templates, so there is no point in keeping it around. Most likely spill over from infamous opendatahub-io#605.

ReplaceChar is not used anywhere in the templates, so there is no point in keeping it around. Most likely spill over from infamous #605.

ReplaceChar is not used anywhere in the templates, so there is no point in keeping it around. Most likely spill over from infamous opendatahub-io#605. (cherry picked from commit 0c363cf)

openshift-ci bot requested review from etirelli and LaVLaS October 5, 2023 17:31

bartoszmajsak requested review from zdtsw and VaishnaviHire October 5, 2023 17:32

zdtsw reviewed Oct 5, 2023

View reviewed changes

This was referenced Oct 5, 2023

Service Mesh SSO(Auth) Integration opendatahub-io/opendatahub-community#97

Open

Enable Service Mesh Auth(z) settings for KServe #614

Closed

Jupyter application fails to start when Service Mesh is enabled #616

Closed

bartoszmajsak force-pushed the service-mesh-integration branch from 5632608 to 7ed09d5 Compare October 6, 2023 06:13

bartoszmajsak mentioned this pull request Oct 6, 2023

Application namespace hardcoded for Kubeflow #617

Closed

openshift-merge-robot added the needs-rebase label Oct 6, 2023

mattmahoneyrh reviewed Oct 6, 2023

View reviewed changes

zdtsw mentioned this pull request Oct 9, 2023

Add finalizer for DSCI and DSC #301

Closed

3 tasks

This was referenced Oct 9, 2023

[Service Mesh]: handle istio proxy events opendatahub-io/odh-dashboard#1937

Merged

[Service Mesh] include SM notebook patches in one function opendatahub-io/odh-dashboard#1955

Merged

VaishnaviHire added the incubating Added to issues that are currently incubating in ODH label Oct 20, 2023

bartoszmajsak force-pushed the service-mesh-integration branch from 290f1b1 to e0f68b7 Compare October 24, 2023 12:03

openshift-merge-robot removed the needs-rebase label Oct 24, 2023

bartoszmajsak mentioned this pull request Oct 24, 2023

feat: adds finalizers to DSC and DSCI #661

Merged

3 tasks

israel-hdez mentioned this pull request Oct 24, 2023

New Manifests for required CRs installation opendatahub-io/kserve#105

Closed

bartoszmajsak force-pushed the service-mesh-integration branch 3 times, most recently from 876e587 to 454edb4 Compare October 25, 2023 17:04

bartoszmajsak and others added 2 commits January 11, 2024 16:18

fix(lint): adjusts code based on latest linter settings

1ed1206

openshift-merge-robot added the needs-rebase label Jan 12, 2024

guicassolato reviewed Jan 19, 2024

View reviewed changes

aslakknutsen mentioned this pull request Jan 26, 2024

feat: update project level auth config with kserve hosts opendatahub-io/odh-model-controller#130

Merged

3 tasks

bartoszmajsak added 5 commits February 5, 2024 09:28

Merge branch 'incubation' into service-mesh-integration

cb1f013

fix(manifests): return walkErr immediately

9b89a0c

walkErr is propagated as part of recursive call when traversing directory tree. This adds a guard check and not proceed with the error happened in the call chain.

fix: uses proper template location

7920590

Merge branch 'incubation' into service-mesh-integration

76a0313

fix(templ): corrects metrics folder location

98049d8

bartoszmajsak mentioned this pull request Mar 8, 2024

fix(authz): ensures extauthz provider is removed from control plane during cleanup #905

Merged

bartoszmajsak mentioned this pull request Jul 9, 2024

chore(feature): removes unused tmpl func #1105

Merged

5 tasks

openshift-merge-bot bot pushed a commit that referenced this pull request Jul 9, 2024

chore(feature): removes unused tmpl func (#1105)

0c363cf

ReplaceChar is not used anywhere in the templates, so there is no point in keeping it around. Most likely spill over from infamous #605.

bartoszmajsak closed this Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

bartoszmajsak commented Oct 5, 2023 •

edited by cam-garrison

Loading

openshift-ci bot commented Oct 5, 2023

bartoszmajsak commented Oct 5, 2023 •

edited

Loading

cam-garrison commented Oct 5, 2023 •

edited

Loading

zdtsw Oct 5, 2023

bartoszmajsak Oct 5, 2023

mattmahoneyrh Oct 6, 2023

zdtsw Oct 5, 2023

bartoszmajsak Oct 5, 2023 •

edited

Loading

bartoszmajsak commented Oct 6, 2023

mattmahoneyrh Oct 6, 2023

bartoszmajsak Oct 6, 2023

zdtsw Oct 9, 2023

mattmahoneyrh Oct 6, 2023

openshift-merge-robot commented Jan 12, 2024

guicassolato Jan 19, 2024

bartoszmajsak Jan 19, 2024

openshift-ci bot commented Apr 24, 2024

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches #605

Conversation

bartoszmajsak commented Oct 5, 2023 • edited by cam-garrison Loading

⚠️ Important

Description

Key features

Additional improvements

Enabling it

How Has This Been Tested?

Merge criteria:

Additional merge criteria/dependencies:

openshift-ci bot commented Oct 5, 2023

bartoszmajsak commented Oct 5, 2023 • edited Loading

WORKAROUND

cam-garrison commented Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

bartoszmajsak commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 24, 2024

bartoszmajsak commented Oct 5, 2023 •

edited by cam-garrison

Loading

bartoszmajsak commented Oct 5, 2023 •

edited

Loading

cam-garrison commented Oct 5, 2023 •

edited

Loading

bartoszmajsak Oct 5, 2023 •

edited

Loading