fix(authz): ensures extauthz provider is removed from control plane during cleanup #905
Conversation
This file contains 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
bartoszmajsak
requested review from
zdtsw and
israel-hdez
and removed request for
lucferbux and
atheo89
3 tasks
bartoszmajsak
changed the title
zdtsw
approved these changes
Mar 8, 2024
The reason for this is to have a simple naming convention instead of suggesting to store migration patches in dedicated folders named after tickets. Additionally feature explictly orders files instead of assuming that underlying fsys implementation fulfills such a contract.
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
```
This is short term fix for existing codebase. Long term 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 it's own thing instead, as it represents the concept in the infrastructure/authz setup.
bartoszmajsak
force-pushed
the
fix-ext-authz-provider
branch
from
533cfe8
to
01b6082
Compare
bartoszmajsak
commented
Mar 8, 2024
| err := fixtures.CreateSubscription(fixtures.OssmSubscription, "openshift-operators", envTestClient) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| BeforeEach(func() { | ||
| err := fixtures.CreateSubscription(fixtures.OssmSubscription, "openshift-operators", envTestClient) |
There was a problem hiding this comment.
As tests run in sequence there is no need now to remove subscription, but I will clean this up in the follow-up PR.
israel-hdez
approved these changes
Mar 8, 2024
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: israel-hdez, zdtsw 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 |
|
New changes are detected. LGTM label has been removed. |
|
/test opendatahub-operator-e2e |
|
/retest |
bartoszmajsak
changed the title
bartoszmajsak
merged commit b990de2
into
opendatahub-io:incubation
6 of 7 checks passed
zdtsw
pushed a commit
to VaishnaviHire/opendatahub-operator
that referenced
this pull request
Mar 12, 2024
…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.
zdtsw
added a commit
to red-hat-data-services/rhods-operator
that referenced
this pull request
Mar 12, 2024
* Update bundle * feat(authz): Authorino for Service Mesh (opendatahub-io#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 (opendatahub-io#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 (opendatahub-io#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 (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 #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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 existing codebase. 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 it's own thing instead, as it represents the concept in the infrastructure/authz setup.