add helm chart for MAS deployments #2653

phk-nord · 2024-04-25T08:42:51Z

This PR intends to add a helm chart.
Although this does not automate the setup and configuration of the authentication service it may be something people might want to have as basis

sandhose

Thanks for this! I've really quickly skimmed through, this isn't a full review.

I'm not yet sure whether this should live in this repository or somewhere else, as I'm not sure I have enough time to maintain this and make sure it doesn't break. Maybe we could do something like synapse does and have it in a contrib/ directory, to make it explicit as it being an external contribution, but not necessarily well tested?

sandhose · 2024-04-30T08:09:28Z

helm/Chart.yaml

+description: A Helm chart for matrix-authentication-service
+type: application
+version: 0.1.0
+appVersion: "1.16.0"


That doesn't feel right, latest version is 0.9.0

yes, thats a remain from helm create

sandhose · 2024-04-30T08:17:08Z

helm/configs/config.yaml

+      resources: {{- range .Values.config.listeners.resources }}
+      - name: {{ .name }}
+      {{- if .path }}
+        path: {{ .path -}}
+      {{- end }}
+      {{- if .playground }}
+        playground: {{ .playground }}
+      {{- end }}
+      {{- end }}


If anyway the chart exposes everything on a single port (which is fine!), I wouldn't expose the various resources in the values, but rather flags like graphqlPlayground, metrics, etc. and change that section accordingly

which listener make sense to expose by default and which should always be optional besides graphqlPlayground and metrics ?

sandhose · 2024-04-30T08:18:23Z

helm/configs/config.yaml

+      {{- if .Values.config.listeners.binds.type.address.enabled }}
+        # First option: listen to the given address
+        - address: {{ .Values.config.listeners.binds.address | quote }}
+      {{- end }}
+      {{- if .Values.config.listeners.binds.type.hp.enabled }}
+        # Second option: listen on the given host and port combination
+        - host: {{ .Values.config.listeners.binds.host.name }}
+          port: {{ .Values.config.listeners.binds.host.port }}
+      {{- end }}
+      {{- if .Values.config.listeners.binds.type.socket.enabled }}
+        # Third option: listen on the given UNIX socket
+        - socket: {{ .Values.config.listeners.binds.socket }}
+      {{- end }}


Is it useful to bind on other host/ports? Why would you want to use unix sockets here?

Is it useful to bind on other host/ports?
almost never, no. There are obscure one node use cases, however you're right, this should be something that covers a regular use case instead

sandhose · 2024-04-30T08:19:35Z

helm/configs/config.yaml

+    {{- if .Values.telemetry.useJaeger.enabled }}
+    protocol: {{ .Values.telemetry.useJaeger.protocol }}
+    endpoint: {{ .Values.telemetry.useJaeger.endpoint }}
+    username: {{ .Values.telemetry.useJaeger.username }}
+    password: {{ .Values.telemetry.useJaeger.password }}
+    agent_host: {{ .Values.telemetry.useJaeger.agent_host }}
+    agent_port: {{ .Values.telemetry.useJaeger.agent_port }}
+    {{- end }}
+    {{- if .Values.telemetry.useZipkin.enabled }}
+    collector_endpoint: {{ .Values.telemetry.useZipkin.collector_endpoint }}
+    {{- end }}


The jaeger and zipkin exporters were removed in 0.9.0

sandhose · 2024-04-30T08:20:18Z

helm/configs/config.yaml

+  # Settings related to upstream OAuth 2.0/OIDC providers. This section must be synced to the database using the config sync command.
+  # https://matrix-org.github.io/matrix-authentication-service/usage/cli/config.html#config-sync---prune---dry-run


This is not the case anymore, as they get synced automatically on startup now

sandhose · 2024-04-30T08:23:10Z

helm/values.yaml

+wellKnown:
+  issuer: https://auth.example.com/
+  authEnd: https://auth.example.com/authorize
+  tokenEnd: https://auth.example.com/oauth2/token
+  jwks: https://auth.example.com/oauth2/keys.json
+  registerEnd: https://auth.example.com/oauth2/registration
+  introEnd: https://auth.example.com/oauth2/introspect
+  revokeEnd: https://auth.example.com/oauth2/revoke


Why would an end user need to configure this?

sandhose · 2024-04-30T08:23:52Z

helm/values.yaml

+      type:
+        address: 
+          enabled: false
+        hp:
+          enabled: true
+        socket: 
+          enabled: false 


why would you need to change this? also hp feels unnecessarily obscure

sandhose · 2024-04-30T08:24:35Z

helm/values.yaml

+secrets:
+  # Encryption secret (used for encrypting cookies and database fields)
+  # This must be a 32-byte long hex-encoded key
+  encrypt: 


Can't we have helm automatically generate something here and put that in a secret?

sandhose · 2024-04-30T08:26:13Z

helm/values.yaml

+  keys:
+    - kid: keyid#1
+      key: |-
+
+    - kid: keyid#2
+      key: |-
+
+    - kid: keyid#3
+      key: |
+
+    - kid: keyid#4
+      key: |


Same here: you could generate a RSA key and a ECDSA P-256 key with Helm and that would be enough

this is now done as well as for secrets.encryption

sandhose · 2024-04-30T08:27:48Z

helm/values.yaml

The configuration keys feel quite inconsistent. Sometimes you use camelCase, sometimes snake_case ; sometimes it's a 1-1 mapping of what is in the MAS config, sometimes it's another term used…

I did try to use the original MAS config, as I am a fan of referring to the regular documentation rather than just doing something entirely else. Sometimes however my mind gets off track. the hp for host+port was indeed quite odd

- remove deprecated monitoring options - remove separate deployment of well-known endpoints - adjust values key pairs to be more in line with the regular configuration keys - use built in /health endpoint - use remove options that dont make sense in kubernetes context

phk-nord · 2024-05-06T12:44:19Z

I'm not yet sure whether this should live in this repository or somewhere else, as I'm not sure I have enough time to maintain this and make sure it doesn't break. Maybe we could do something like synapse does and have it in a contrib/ directory, to make it explicit as it being an external contribution, but not necessarily well tested?

I'd be for a contrib/ directory as well because of that very disclaimer that it's not official. The helm chart of matrix-hookshot for example is entirely broken and drives me crazy. Although it is apparently not contributed or at least not inside their contrib folder

jessebot · 2024-05-25T13:38:52Z

Thanks for this! This would be awesome to move forward, because then I can archive a chart I recently stood up for the same thing here:
https://github.com/small-hack/matrix-authentication-service-chart/tree/main/charts/matrix-authentication-service

Please feel free to take anything from my chart above to help your cause too :)

gabreal · 2024-06-06T12:07:13Z

helm/configs/config.yaml

+      {{- end }}
+      - name: health
+        path: /health
+      {{- if .Values.telemetry.metrics.enabled }}


what is the idea behind putting some config values below .Values.config and others like this not?

gabreal · 2024-06-06T12:11:22Z

helm/configs/config.yaml

+  # Confidential client
+  - client_id: {{ .Values.clients.client_id }}
+    client_auth_method: {{ .Values.clients.client_auth_method }}
+    client_secret: {{ .Values.clients.client_secret }}


i understand that secrets handling mostly depends on custom integrations, but having secrets in a configmap is not so typical. anance's synapse chart for instance puts them into separate config files from secrets. it has pros and cons.

gabreal · 2024-06-06T12:13:19Z

helm/README.md

+After initialising the pod you need to sync MAS with it's config
+
+```bash
+kubectl exec -it -n mas deployments/mas -- mas-cli config sync


the sync could be run from an init container. since the migrations and basic sync are now performed automatically on start up we added:

- name: mas-sync-clients image: ghcr.io/matrix-org/matrix-authentication-service:0.9.0 command: ["/usr/local/bin/mas-cli", "config", "sync", "--config=/config/config.yaml", "--prune"] volumeMounts: - mountPath: /config name: config

gabreal · 2024-06-06T12:15:43Z

helm/values.yaml

+  repository: ghcr.io/matrix-org/matrix-authentication-service
+  pullPolicy: IfNotPresent
+  # Overrides the image tag whose default is the chart appVersion.
+  tag: "0.9.0"


why not use appVersion from the chart as default (ref)?

add helm chart

4ee5d79

sandhose reviewed Apr 30, 2024

View reviewed changes

phk-nord added 3 commits May 2, 2024 15:47

remove deprecated options

3be73c8

generate keys and secrets using helm

bae9fdf

various changes

8f0a720

- remove deprecated monitoring options - remove separate deployment of well-known endpoints - adjust values key pairs to be more in line with the regular configuration keys - use built in /health endpoint - use remove options that dont make sense in kubernetes context

gabreal reviewed Jun 6, 2024

View reviewed changes

matrixbot mentioned this pull request Sep 10, 2024

add helm chart for MAS deployments element-hq/matrix-authentication-service#2653

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add helm chart for MAS deployments #2653

add helm chart for MAS deployments #2653

phk-nord commented Apr 25, 2024

sandhose left a comment

sandhose Apr 30, 2024

phk-nord May 6, 2024

sandhose Apr 30, 2024

phk-nord May 6, 2024

sandhose Apr 30, 2024

phk-nord May 6, 2024

sandhose Apr 30, 2024

sandhose Apr 30, 2024

sandhose Apr 30, 2024

sandhose Apr 30, 2024

sandhose Apr 30, 2024

sandhose Apr 30, 2024

phk-nord May 6, 2024

sandhose Apr 30, 2024

phk-nord May 6, 2024

phk-nord commented May 6, 2024

jessebot commented May 25, 2024

gabreal Jun 6, 2024

gabreal Jun 6, 2024

gabreal Jun 6, 2024

gabreal Jun 6, 2024

		# Settings related to upstream OAuth 2.0/OIDC providers. This section must be synced to the database using the config sync command.
		# https://matrix-org.github.io/matrix-authentication-service/usage/cli/config.html#config-sync---prune---dry-run

add helm chart for MAS deployments #2653

Are you sure you want to change the base?

add helm chart for MAS deployments #2653

Conversation

phk-nord commented Apr 25, 2024

sandhose left a comment

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

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

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

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

phk-nord commented May 6, 2024

jessebot commented May 25, 2024

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