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.

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

fix(authz): Fix broken external auth configuration #892

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

israel-hdez
Copy link
Contributor

Description

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).

Fixes https://issues.redhat.com/browse/RHOAIENG-4192

How Has This Been Tested?

Both doing a clean install, and simulating an upgrade.

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

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 <23639005+israel-hdez@users.noreply.github.com>
@israel-hdez
Copy link
Contributor Author

@VaishnaviHire This would fix the broken installation that Eyal reported.

@bartoszmajsak
Copy link
Contributor

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.

@israel-hdez I think this is a good workaround for now, but there's a config map (auth-refs) we can use to share the final name across controllers. /cc @aslakknutsen

Copy link
Contributor

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

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

LGTM

I am wondering however if we have any automated test somewhere smoke testing this setup?

@zdtsw
Copy link
Member

zdtsw commented Mar 6, 2024

you want to keep file name "pkg/feature/templates/servicemesh/kserve/z-fix-jira-4192/kserve-predictor-authorizationpolicy.patch.tmpl" ?

@zdtsw zdtsw removed the request for review from ajaypratap003 March 6, 2024 13:06
@zdtsw zdtsw added the odh-2.9 label Mar 6, 2024
@israel-hdez
Copy link
Contributor Author

you want to keep file name "pkg/feature/templates/servicemesh/kserve/z-fix-jira-4192/kserve-predictor-authorizationpolicy.patch.tmpl" ?

Yes, manifests are applied in filename alphabetical order. So, I wanted to make sure the patch is applied at last.

@israel-hdez
Copy link
Contributor Author

israel-hdez commented Mar 6, 2024

I am wondering however if we have any automated test somewhere smoke testing this setup?

I guess E2E tests on KServe repo would have catched this, but they are not using the operator to setup the stack. So, I think in serving side we should improve our E2E setup to catch earlier this.

Copy link
Contributor

@VedantMahabaleshwarkar VedantMahabaleshwarkar left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Mar 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartoszmajsak, VedantMahabaleshwarkar, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Mar 6, 2024
@zdtsw zdtsw merged commit c04130f into opendatahub-io:incubation Mar 6, 2024
6 of 7 checks passed
@israel-hdez israel-hdez deleted the jira-4192-fix branch March 6, 2024 17:27
@zdtsw
Copy link
Member

zdtsw commented Mar 8, 2024

i am confused again :D
https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/pkg/feature/servicemesh/cleanup.go#L15C2-L15C79
should this be updated too, if the name is patched to {{ .AppNamespace }}-auth-provider already

@bartoszmajsak
Copy link
Contributor

i am confused again :D incubation/pkg/feature/servicemesh/cleanup.go#L15C2-L15C79 should this be updated too, if the name is patched to {{ .AppNamespace }}-auth-provider already

Yeah, we missed it :( @zdtsw go ahead and open PR with the fix.

@zdtsw
Copy link
Member

zdtsw commented Mar 8, 2024

i am confused again :D incubation/pkg/feature/servicemesh/cleanup.go#L15C2-L15C79 should this be updated too, if the name is patched to {{ .AppNamespace }}-auth-provider already

Yeah, we missed it :( @zdtsw go ahead and open PR with the fix.

:rage4:

@bartoszmajsak
Copy link
Contributor

I added fix and ported missing tests in #905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants