Skip to content

[fix] Add default multiplayer ingress rule#205

Merged
jjlgao merged 5 commits intomainfrom
jason/multiplayer-ingress
Feb 18, 2025
Merged

[fix] Add default multiplayer ingress rule#205
jjlgao merged 5 commits intomainfrom
jason/multiplayer-ingress

Conversation

@jjlgao
Copy link
Contributor

@jjlgao jjlgao commented Jan 21, 2025

Addresses #199, aims to automatically create an ingress rule, if multiplayer is enabled, to point requests for /api/multiplayer to the multiplayer pod. To avoid conflicts with folks who already have this ingress rule set, this automatic rule can be disabled using multiplayer.ingress.enabled=false.

Tested by applying to our test cluster, which created a new (inaccessible) ingress, which looks like this (redacted hostnames just in case, but they shouldn't be sensitive):
Screenshot 2025-01-22 at 8 15 26 AM
Screenshot 2025-01-22 at 8 15 34 AM

I also added these values to the values.yaml file:

ingress:
  enabled: true
  labels: {}
  annotations: {}
  # kubernetes.io/ingress.class: nginx
  # kubernetes.io/tls-acme: "true"
  hosts:
  - host: retool-dev.[hostname]
    paths:
      - path: /
  tls:
  - secretName: retool-dev-tls
    hosts:
      - retool-dev.[hostname]
  pathType: ImplementationSpecific
multiplayer:
  enabled: true

@jjlgao jjlgao marked this pull request as ready for review January 22, 2025 16:09
Copy link

@lukefoster11 lukefoster11 left a comment

Choose a reason for hiding this comment

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

Looks good, just had one question before merge

# Paths to route to multiplayer pods; defaults to /api/multiplayer. Can specify both path and port.
ingress:
# This conditional is dependent on multiplayer.enabled.
enabled: true

Choose a reason for hiding this comment

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

I know this can be set to false but by default could this be a breaking change for anyone who has already set up this ingress rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be, which I considered and decided to keep this way. I think there's no real way to make this default-true moving forward, without breaking folks who have the ingress rule already. I believe that what will happen is that people will run into an error on helm upgrade due to the duplicate routes, so when I tag the release for this PR I'll make a note in the release to expect the breaking change.

Choose a reason for hiding this comment

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

Gotcha, yeah don't know a smooth alternative so this makes sense

enabled: true
paths:
- path: /api/multiplayer
port: 80

Choose a reason for hiding this comment

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

The multiplayer service uses 3009 as the default port, should that be set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this points at the entry port for the service, which is 80: https://github.com/tryretool/retool-helm/blob/main/charts/retool/templates/deployment_multiplayer_ws.yaml#L13

The destination port for the multiplayer deployment is hardcoded to 3001 here (probably should have been left at 3009 rather than introducing potential port conflicts, but that isn't relevant for this change), with another hardcoded env var pointing the container to port 3001: https://github.com/tryretool/retool-helm/blob/main/charts/retool/templates/deployment_multiplayer_ws.yaml#L78

@alex-stabile alex-stabile self-requested a review January 25, 2025 21:50
@jjlgao jjlgao force-pushed the jason/multiplayer-ingress branch from d837016 to 7a5bc22 Compare February 18, 2025 00:36
@jjlgao jjlgao merged commit 9a3c6ff into main Feb 18, 2025
12 checks passed
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.

3 participants