appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
34.62k stars 3.73k forks source link

[Feature]: Allow customization of HTTP port in Helm chart #36761

Closed damienleger closed 2 weeks ago

damienleger commented 1 month ago

Is there an existing issue for this?

Summary

Right now tcp/80 is hardcoded https://github.com/appsmithorg/appsmith/blob/8463d02740f9e300c8d306c7012edec39067d2ff/deploy/helm/templates/statefulset.yaml#L73

The feature is about adding a config in values.yaml to customize this port.

I've drafter an implem, a PR will follow.

Why should this be worked on?

My K8S setup is based on the well-known and used community EKS terraform module. And I'm using Nginx ingress controller (community).

a) The terraform module doesn't allow node-to-node communication on privileged port (< 1025) https://github.com/terraform-aws-modules/terraform-aws-eks/blob/d2c671aca25165cc060e2459f5cafb49868a7be3/node_groups.tf#L134

b) Internally the Ingress configuration relies on Service, but Nginx ingress controller actually resolve the service endpoint and use directly the pod IP to fill-in its maintained nginx config file. If I remember correctly it's to allow for better load balancing than the Service loadbalancing abstraction offers.

So for Ingress to work, a nginx pod needs to contact directly (see b) appsmith's Caddy on tcp/80 pod IP, and because of a) it doesn't work.

I can fix by extending the node-to-node communication security group to privileged port but it's not advisable because

c) Opening a listening port on privileged port requires either root user or at least the CAP_NET_BIND_SERVICE capability. Both are security bad practice: secured environment must leverage SecurityContext to prevent root user and drop all capabilities (at least).

damienleger commented 1 month ago

Regarding implementation. The hardest part been done already: the envvar PORT is used to configured Caddy. Just need to add this envvar in K8S manifest and make the new part both on PORT envvar and containerPort definition https://github.com/appsmithorg/appsmith/blob/8463d02740f9e300c8d306c7012edec39067d2ff/deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs#L167

PR incoming

damienleger commented 1 month ago

^ PR above