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.

Sign up for GitHub

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

Add auth config for scalar manager #280

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add auth config for scalar manager #280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd2476b
Add auth config for scalar manager
thongdk8 Dec 27, 2024
78607d9
Refactor config
thongdk8 Jan 10, 2025
7b54ac3
fix trailing spcace
thongdk8 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions charts/scalar-manager/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,17 @@ Current chart version is `3.0.0-SNAPSHOT`
| securityContext.allowPrivilegeEscalation | bool | `false` | |
| securityContext.capabilities.drop[0] | string | `"ALL"` | |
| securityContext.runAsNonRoot | bool | `true` | |
| service.port | int | `80` | |
| service.type | string | `"ClusterIP"` | |
| service.api.port | int | `8080` | |
| service.api.type | string | `"ClusterIP"` | |
| service.web.port | int | `80` | |
| service.web.type | string | `"ClusterIP"` | |
| serviceAccount.automountServiceAccountToken | bool | `true` | |
| serviceAccount.serviceAccountName | string | `""` | |
| tolerations | list | `[]` | |
| web.authorization.baseUrl | string | `"http://localhost:8080"` | |
| web.authorization.enabled | bool | `false` | |
| web.image.pullPolicy | string | `"IfNotPresent"` | |
| web.image.repository | string | `"ghcr.io/scalar-labs/scalar-manager-web"` | |
| web.image.tag | string | `""` | |
| web.operation.baseUrl | string | `"http://localhost:8080"` | |
| web.resources | object | `{}` | |
7 changes: 7 additions & 0 deletions charts/scalar-manager/templates/scalar-manager/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ spec:
image: "{{ .Values.web.image.repository }}:{{ .Values.web.image.tag | default .Chart.AppVersion }}"
resources:
{{- toYaml .Values.web.resources | nindent 12 }}
env:
- name: NEXT_PUBLIC_AUTH_ENABLED
value: {{ .Values.web.authorization.enabled | quote }}
- name: NEXT_PUBLIC_PERSISTENCE_API_BASE_URL
value: {{ .Values.web.authorization.baseUrl | quote }}
- name: NEXT_PUBLIC_OPERATION_API_BASE_URL
value: {{ .Values.web.operation.baseUrl | quote }}
Comment on lines +47 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these environment variables mandatory?

Listing a lot of environment variables here might cause increasing maintenance costs for Helm Chart. This is my concern.

So, I think it would be better to set configurations via application.properties instead of environment variables. In such a case, we might not need to update the Helm Chart even if you add/change some configurations on the Scalar Manager side. It can reduce the maintenance costs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These environment variables are mandatory, but they are for the web container. For the api container we still keep the config in the application.properties as it was.
The config for the web container would not be many I think, for now we will have these new three vars.

ports:
- containerPort: 3000
imagePullPolicy: {{ .Values.web.image.pullPolicy }}
Expand Down
23 changes: 20 additions & 3 deletions charts/scalar-manager/templates/scalar-manager/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,33 @@
apiVersion: v1
kind: Service
metadata:
name: {{ include "scalar-manager.fullname" . }}
name: {{ include "scalar-manager.fullname" . }}-web
namespace: {{ .Release.Namespace }}
labels:
{{- include "scalar-manager.labels" . | nindent 4 }}
spec:
type: {{ .Values.service.type }}
type: {{ .Values.service.web.type }}
ports:
- protocol: TCP
name: web
port: {{ .Values.service.port }}
port: {{ .Values.service.web.port }}
targetPort: 3000
selector:
{{- include "scalar-manager.selectorLabels" . | nindent 4 }}
---
apiVersion: v1
kind: Service
Comment on lines +18 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a service for API in case we want to expose the API for cross Scalar Manager authentication

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I don't know the details of cross Scalar Manager authentication. If I remember correctly, we planned to add the simple authentication feature as a first step on the Scalar Manager Web side only to avoid anonymous login.

So, could you please elaborate on the details of cross Scalar Manager authentication?

I am not able to judge whether this service resource is necessary or not and configuration is proper or not without the detailed information of cross Scalar Manager authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, let me explain it a bit more based on the current spec we have.
The cross authentication is in the case: We have two instances of the SM (Scalar Manager) in different clusters for example, one is deployed with a configured database and it has the authentication endpoints, once login the user will have the JWT tokens to access other protected endpoints from that instance (e.g pause, schedule pause). The other instance is deployed without database config but it is configured with the JWT public key sharing the same as instance 1 so this instance is able to verify requests with the JWT token in the header issued by instance 1, basically client (web UI) can login to SM 1 and access to both SM 1 and SM 2, of course the SM 2 API need to be viable to the SM 1.
Please correct me if I miss something @ypeckstadt
I made a PR for the docs update that contains a bit more info about deployment configuration here https://github.com/scalar-labs/docs-internal-orchestration/pull/86

metadata:
name: {{ include "scalar-manager.fullname" . }}-api
namespace: {{ .Release.Namespace }}
labels:
{{- include "scalar-manager.labels" . | nindent 4 }}
spec:
type: {{ .Values.service.api.type }}
ports:
- protocol: TCP
name: api
port: {{ .Values.service.api.port }}
targetPort: 8080
selector:
{{- include "scalar-manager.selectorLabels" . | nindent 4 }}
43 changes: 39 additions & 4 deletions charts/scalar-manager/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,27 @@
"service": {
"type": "object",
"properties": {
"port": {
"type": "integer"
"api": {
"type": "object",
"properties": {
"port": {
"type": "integer"
},
"type": {
"type": "string"
}
}
},
"type": {
"type": "string"
"web": {
"type": "object",
"properties": {
"port": {
"type": "integer"
},
"type": {
"type": "string"
}
}
}
}
},
Expand All @@ -119,6 +135,17 @@
"web": {
"type": "object",
"properties": {
"authorization": {
"type": "object",
"properties": {
"baseUrl": {
"type": "string"
},
"enabled": {
"type": "boolean"
}
}
},
"image": {
"type": "object",
"properties": {
Expand All @@ -133,6 +160,14 @@
}
}
},
"operation": {
"type": "object",
"properties": {
"baseUrl": {
"type": "string"
}
}
},
"resources": {
"type": "object"
}
Expand Down
61 changes: 59 additions & 2 deletions charts/scalar-manager/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ nodeSelector: {}
tolerations: []

service:
type: ClusterIP
port: 80
Comment on lines -30 to -31
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this update breaks backward compatibility. This is because the existing configuration service.type and service.port will not work after this update.

Is my understanding correct?

Also, if this backward incompatible update is intended, do you expect to release this new update as a new major version release? I want to confirm it, just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will break the backward compatibility as the config will be moved under the web and I think we will make a new major version release. One other way if we want to keep the backward compatibility is keeping the service at the top level as it was for the web and add only a service config for the API under the api config, but that makes the config looks not be in the same style. WDYT?

web:
type: ClusterIP
port: 80
api:
type: ClusterIP
port: 8080
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be more understandable for users to use web.service.*** and api.service.*** instead of service.web.*** and service.api.***.

This is because Scalar Manager Web and Scalar Manager API are deployed as different containers respectively. (I know they are deployed in one pod, but the components are separated in the pod.)

Also, I think it would be better to combine Scalar Manager Web configurations under the web: key and combine Scalar Manager API configurations under the api: key. In this pattern, users can know These configurations are for Web and These configurations are for API clearly.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, that makes sense. Let me refactor them.


serviceAccount:
serviceAccountName: ""
Expand Down Expand Up @@ -80,12 +84,65 @@ api:
paused-state-retention.storage=${PAUSED_STATE_RETENTION_STORAGE:configmap}
paused-state-retention.max-number=${PAUSED_STATE_RETENTION_MAX_NUMBER:100}

# JWT configuration
# P-256 (secp256k1) private key in PKCS8 format, using for signing JWT tokens, required when persistence endpoints are enabled
authentication.providers.static-jwt.private-key=${AUTHENTICATION_PROVIDERS_STATIC_JWT_PRIVATE_KEY:}
# Public key in X.509/SPKI format using for verifying JWT tokens, when authentication is enabled, this key is used for verifying JWT tokens
authentication.providers.static-jwt.public-key=${AUTHENTICATION_PROVIDERS_STATIC_JWT_PUBLIC_KEY:}
authentication.providers.static-jwt.issuer-uri=${AUTHENTICATION_PROVIDERS_STATIC_JWT_ISSUER_URI:https://scalar-manager.scalar-labs.com}
authentication.providers.static-jwt.access-token-expiration-time=${AUTHENTICATION_PROVIDERS_STATIC_JWT_ACCESS_TOKEN_EXPIRATION_TIME:15m}
authentication.providers.static-jwt.refresh-token-expiration-time=${AUTHENTICATION_PROVIDERS_STATIC_JWT_REFRESH_TOKEN_EXPIRATION_TIME:3d}

# OpenAPI configuration
springdoc.swagger-ui.enabled=${SPRINGDOC_SWAGGER_UI_ENABLED:false}
springdoc.swagger-ui.path=${SPRINGDOC_SWAGGER_UI_PATH:/swagger-ui.html}

# Whether to enable persistence endpoints or not (auth, user management)
app.persistence-endpoints.enabled=${APP_PERSISTENCE_ENDPOINTS_ENABLED:false}

# Whether to enable authorization or not for the operational endpoints
app.authorization.enabled=${APP_AUTHORIZATION_ENABLED:false}

# CORS configuration
app.cors.allowed-origins=${APP_CORS_ALLOWED_ORIGINS:*}
app.cors.allowed-methods=${APP_CORS_ALLOWED_METHODS:*}
app.cors.allowed-headers=${APP_CORS_ALLOWED_HEADERS:*}

# # Initial admin configuration, only need these configurations if persistence endpoints is enabled
# app.initial-admin-user.enabled=${APP_INITIAL_ADMIN_USER_ENABLED:false}
# app.initial-admin-user.email=${APP_INITIAL_ADMIN_USER_EMAIL:[email protected]}
# app.initial-admin-user.name=${APP_INITIAL_ADMIN_USER_NAME:Administrator}
# app.initial-admin-user.password=${APP_INITIAL_ADMIN_USER_PASSWORD:Password@123!}

# # JPA configuration, only need these configurations if persistence endpoints is enabled
# spring.jpa.hibernate.ddl-auto=${SPRING_JPA_HIBERNATE_DDL_AUTO:update}
# spring.jpa.show-sql=${SPRING_JPA_SHOW_SQL:false}
# spring.jpa.properties.hibernate.format_sql=${SPRING_JPA_PROPERTIES_HIBERNATE_FORMAT_SQL:false}

# # Database configuration, only need these configurations if persistence endpoints is enabled
# spring.datasource.url=jdbc:postgresql://${DATABASE_HOST:scalar-manager-postgres-postgresql}:${DATABASE_PORT:5432}/${DATABASE_NAME:scalar-manager}
# spring.datasource.username=${DATABASE_USERNAME:scalar-manager}
# spring.datasource.password=${DATABASE_PASSWORD:scalar-manager}
# spring.datasource.driver-class-name=org.postgresql.Driver


web:
image:
repository: ghcr.io/scalar-labs/scalar-manager-web
pullPolicy: IfNotPresent
# Overrides the image tag whose default is the chart appVersion.
tag: ""

authorization:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the application.properties, it seems that you use the term authentication as a property name. However, in the values.yaml file, you use authorization.

Is there any reason why you use the term authorization here?

Sorry if I missed something, but I think it would be better to use the same term authentication to avoid unnecessary confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually in the application.properties I also used the authorization in app.authorization.enabled config. The authentication word is used in the prefix for the JWT config only authentication.providers.static-jwt.xxx.
The main reason that authorization makes more sense in our case that the API can be deployed without authentication endpoints but it is still able to verify the request via the JWT in the Authorization header from the requests.

# Whether to enable authorization or not for the web application, if enabled the login, user management page will be available
enabled: false
# The base URL of the authorization service, default is same as the scalar-manager-api service
baseUrl: http://localhost:8080
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of the API service running in the same pod with Scalar Manager web doesn't have the persistent endpoints enabled then we can config this baseUrl point to another instance of the Scalar Manager API (in other clusters) to do authentication. Note that the JWT public key need to be configured the same in the API instances


operation:
# The base URL of the operation service, default is same as the scalar-manager-api service
baseUrl: http://localhost:8080

resources:
{}
# We usually recommend not to specify default resources and to leave this as a conscious
Expand Down
Loading