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

feat: add authorization service to Renku Helm chart #3340

Merged
merged 14 commits into from
Apr 22, 2024

Conversation

olevski
Copy link
Member

@olevski olevski commented Oct 31, 2023

This adds Authzed's SpiceDB as an authorization service.

/deploy

@olevski olevski requested review from a team as code owners October 31, 2023 21:42
@olevski olevski marked this pull request as draft October 31, 2023 21:42
@olevski olevski temporarily deployed to ci-renku-3340 October 31, 2023 21:42 — with GitHub Actions Inactive
@olevski olevski temporarily deployed to ci-renku-3340 October 31, 2023 23:52 — with GitHub Actions Inactive
@RenkuBot
Copy link
Collaborator

You can access the deployment of this PR at https://ci-renku-3340.dev.renku.ch

@olevski olevski temporarily deployed to ci-renku-3340 October 31, 2023 23:57 — with GitHub Actions Inactive
@olevski olevski force-pushed the build-authorization branch from b8627c7 to 0e7f9d2 Compare April 10, 2024 09:57
@olevski olevski marked this pull request as ready for review April 11, 2024 08:50
@olevski olevski changed the base branch from build-authorization to release-0.52.x April 11, 2024 08:52
helm-chart/renku/templates/authz/deployment.yaml Outdated Show resolved Hide resolved
Comment on lines +12 to +20
ports:
- port: 8443
targetPort: http
protocol: TCP
name: http
- port: 50051
targetPort: grpc
protocol: TCP
name: grpc
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a port for the metrics too, as well as the prometheus annotations

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the idea was that you will add all the stuff around logging and monitoring. So I leave this up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok after the discussion in the weekly sync meeting it is clear to me that I will be doing this. So I will do this in a separate followup PR.

Comment on lines +1 to +15
---
{{- $db_password := randAlphaNum 64 }}
{{- $db_password_enc := $db_password | b64enc | quote }}
{{- $db_conn_uri := (printf "postgres://authz:%s@%s:5432/authz" $db_password (include "postgresql.fullname" .)) | b64enc | quote }}
{{- $api_key := randAlphaNum 64 | b64enc | quote }}

{{- $renkuFullname := include "renku.fullname" . -}}

{{- $secretName := cat $renkuFullname "-authz" | nospace }}
{{- $secret := (lookup "v1" "Secret" .Release.Namespace $secretName) }}
{{- if $secret }}
{{- $db_password_enc = index $secret.data "SPICEDB_POSTGRES_PASSWORD" }}
{{- $db_conn_uri = index $secret.data "SPICEDB_DATASTORE_CONN_URI" }}
{{- $api_key = index $secret.data "SPICEDB_GRPC_PRESHARED_KEY" }}
{{- end -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to allow for existing postgres credentials to be provided in the values file

Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to the database name. I do not want to allow people to change this. What we accept is a single postgres admin password that can be passed through the values file. Then using this password renku makes different postgres accounts with their own passwords and gives them access to specific hardcoded databases. If I accept this in the values file I also have to add logic and jobs to compare it with a previous value and to then if needed update the database so that everything works.

helm-chart/renku/templates/network-policies.yaml Outdated Show resolved Hide resolved
Comment on lines +55 to +58
- name: AUTHZ_DB_USERNAME
value: authz
- name: AUTHZ_DB_NAME
value: authz
Copy link
Contributor

Choose a reason for hiding this comment

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

We should allow for these values to be set in the values instead of hard coding them. See setup-job-gitlab.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont want anyone to change this. The deployment/app manages this. We do not allow people to change any of the database names we create for the data service as well.

If this is in the values file and it is changed then the deployment will break because no one will be able to access anything since there will be a new blank database without any permissions.

If you look at the setup-job-renku-dbs.yaml file you will see that the DB names for the KG and the data services (we call the db renku) are also all hardcoded.

setup-job-gitlab.yaml is only used for the internal gitlab only. So it is actually never used and it will be removed soon.

autoscaling:
enabled: false
minReplicas: 1
maxReplicas: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems quite high. Has this been necessary during testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not been able to do any load testing. It is still too early for this because the authorization service is not hooked up to anything in the backend yet. So I cannot do any meaningful load tests. I changed this to 3 and we can revise it further.

But this is also very dependent on each deployment. If the deployment serves 10 people it will need less replicas than something like renkulab.io.

Either way I can do some tests when I implement stuff on the backend and revise this. The autoscaling is also off by default.

@olevski olevski requested a review from wesjdj April 12, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants