feat: open port 2222 on amalthea session service - #1230
SalimKayal wants to merge 2 commits into
Conversation
998dbca to
5ccb042
Compare
| const secondProxyPort int32 = 65533 | ||
| const RemoteSessionControllerPort int32 = 65532 | ||
| const TunnelPort int32 = 65531 | ||
| const SSHPort int32 = 2222 |
There was a problem hiding this comment.
2222 is not reserved, so it could be in conflict with the session.
| Port: TunnelPort, | ||
| TargetPort: intstr.FromString(tunnelServiceName), | ||
| }) | ||
| } else if cr.Labels[frontendVariantLabel] == sshFrontendVariant { |
There was a problem hiding this comment.
This is bad design: this is completely coupled to having renku.io/frontend-variant = ssh on the AmaltheaSession CR. It is non-extensible and non-flexible both on the Renku side and the Amalthea side.
Solution:
- Use a matching logic de-coupled from the "frontend variant", for example:
renku.io/ssh_enabled = true, renku.io/ssh_port = 2222 - Much, much more clean: add a
sshobject into the session spec.if cr.Spec.Session.SSH.Enabled { // Add svc port to cr.Spec.Session.SSH.Port // Also add pod annotation (or label if must) for netpols }
There was a problem hiding this comment.
I'm wondering, if we maybe should make it even more flexible by not tying it to SSH. If I understand correctly, we pass the port via Spec.Session.SSH.Port. But we could provide a means to name any additional ports that should be added to the spec. We already have a means to pass labels and annotations from the session down to child resources. So no SSH.Enabled necessary, which wouldn't really enable ssh anyways.
There was a problem hiding this comment.
In that case, you can have:
ExtraPorts:
- Port: 2222 (int)
Type: TCP (TCP or UDP)
Service:
Enabled: true
Type: ClusterIP
By default the list is empty, so no extra ports are open, and any number of extra ports can be added.
I don't think we want to expose extra HTTP frontends, which would also need extra ingress configs.
There was a problem hiding this comment.
But this will not solve for network policies, so I think that having a dedicated SSH section is acceptable.
There was a problem hiding this comment.
wrt netpols: wouldn't we need to pass specific labels/annotations controlling the netpols from the dataservices? Or are they all decided in amalthea itself (could be I suppose)? Currently, I think we are adding the netpols with "static" labels (or whatever) given in the yaml files. Or I missed it in the code.
There was a problem hiding this comment.
AFAIK, all the network policies are managed in the renku helm chart.
Currently most are "hard coded" to specific pods names rather than dedicated labels (e.g. database-access, solr-access, etc).
I am working to improve that.
|
This PR has been superseded by serving the ssh server on RENKU_SESSION_PORT and wiring that in data-services+netpols |
4 participants
Summary
Open port 2222 on the session Service only for sessions labeled
renku.io/frontend-variant=ssh.Motivation and context
data-services labels the session CR with the launcher's frontend. Only ssh sessions are reached by the proxy, so non-ssh sessions should not expose the port at all.
Changes
api/v1alpha1/amaltheasession_children.go: theamalthea-ssh2222 port is added to local sessions only whenrenku.io/frontend-variant == ssh; remote sessions are unchanged.api/v1alpha1/amaltheasession_children_test.go: cover local ssh, local non-ssh, local without the label, and remote ssh.