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.

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 ftp passive ports #60

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

nrx-ops
Copy link

@nrx-ops nrx-ops commented Apr 16, 2021

No description provided.

@sagikazarmark
Copy link
Owner

Hi @nrx-ops !

Thanks for the contribution! I have a couple reservations/questions about the implementation though:

  • Configuration does not necessarily come from the config value, it can be mounted as a secret or configmap. Therefore I think it would be better to make this a separate config in the values file which makes it somewhat redundant, but makes the solution more universal for other config sourcing options.
  • You added support for passive ports in the main service, but the additional services will also need to be changed.
  • How should we handle node ports? If someone wishes to use node ports, the assigned ports will be random.

I also wonder how much of an edge case is this. Is this something this chart should support, or could it be a section in the documentation (ie. this is how you can create a service for passive ports). I'm not saying no, but I have more questions than answers at the moment.

What do you think?

@nrx-ops
Copy link
Author

nrx-ops commented Apr 16, 2021

Hi,
Thanks for the review. You made an awesome work with this chart. I not very familiar with GH process, this P/R is in early stage.

  • I really wonder about the way to specify pasv port range from .Value or from config. I had some issues on other charts with duplicate config. I though using .Values.config was a better idea, but I missed secret/cm config. I can change it with a new default value under ftpd. This will avoid some dirty lines.

  • OK, I will add it

  • I have no good idea, for this case. Maybe we could use a startPort and increase for each pasv-port, I will see that and propose you a solution, the other solution is to make incompatible nodePorts and pasv-ports.

I think pasv port should be supported by this chart. sftpgo supports FTP and ftp need pasv-ports to work.

@sagikazarmark
Copy link
Owner

I really wonder about the way to specify pasv port range from .Value or from config. I had some issues on other charts with duplicate config.

The alternative is to make the port range part of the values file and configure it via environment variables. See deployment.yaml for examples.

@vincenthodicq
Copy link

Hi @nrx-ops

did you make it work with FTP with your changes? I tried but I still got some errors on the configuration part of sftpgo.

Regards

@nrx-ops
Copy link
Author

nrx-ops commented Jul 19, 2021

Hi @vincenthodicq,
I make it work on ovh cloud and GCP.
What kind of error do you have? What cloud provider do you use?

@vincenthodicq
Copy link

I deployed it on Azure AKS. I had errors when trying to connect with FTP, i got errors with Passive connection and unable to access any of the passive port.

What type of service did you use to install sftpgo? did you use ingress?

@nrx-ops
Copy link
Author

nrx-ops commented Jul 19, 2021

OK,
I did not use Ingress, only the loadbalancer service.
On gcp I had to fix firewall rules to make it works.

@vincenthodicq
Copy link

@nrx-ops can you share an example of the values your override for this configuration? (without anything sensitive or protected)

@vincenthodicq
Copy link

I am looking in particular to see if you used force_passive_ip for ftpd configuration. If yes, did you use load balancerIP?

And did you use specific externalTrafficPolicy for your service?

Thanks in advance

@nrx-ops
Copy link
Author

nrx-ops commented Jul 19, 2021

replicaCount: 1
podSecurityContext:
  fsGroup: 1000
sftpd:
  enabled: true
ftpd:
  enabled: true
webdavd:
  enabled: false
config:
  defender:
    enabled: true
  sftpd:
    host_keys:
      - /var/lib/sftpgo/fingerprints/id_rsa
      - /var/lib/sftpgo/fingerprints/id_ecdsa
      - /var/lib/sftpgo/fingerprints/id_ed25519
    kex_algorithms:
      - diffie-hellman-group1-sha1
      - diffie-hellman-group14-sha1
      - ecdh-sha2-nistp256
      - ecdh-sha2-nistp384
      - ecdh-sha2-nistp521
      - [email protected]
    ciphers:
      - aes128-ctr,
      - aes192-ctr
      - aes256-ctr
      - [email protected]
      - chacha20Poly1305ID
      - arcfour256
      - arcfour128
      - arcfour
      - aes128cbcID
      - tripledescbcID
  ftpd:
    bindings:
      port: 2021
    passive_port_range:
      start: 20000
      end: 20100
    enable_site: false
  data_provider:
    driver: postgresql
    name: sftpgo
    host: postgresql-sftp
    port: 5432
    username: postgres
    password: [REDACTED]
    prefer_database_credentials: true


service:
  type: LoadBalancer
  externalTrafficPolicy: Local
  loadBalancerIP: [REDACTED]
  annotations: {}
  ports:
    ftpPasv:
      ports:
        start: 20000
        end: 20100
      nodeports:
        start: 30000

volumes:
  - name: cm-fingerprint-sftp
    configMap:
      name: cm-fingerprint-sftp
volumeMounts:
  - mountPath: /var/lib/sftpgo/fingerprints
    name: cm-fingerprint-sftp

@vincenthodicq
Copy link

Thanks. It helps and i am now able to connect with FTP. Still having an issue with FTPS (default port being 990 and not opened / configured in the chart)

@nrx-ops
Copy link
Author

nrx-ops commented Jul 20, 2021

In this case, I think that .Values.service.ports.ftp.port must be set on 990.
Or you should use additionnal service

@OlavH96
Copy link

OlavH96 commented Nov 9, 2023

Any update on this PR? Without exposing passive ports the ftp portion of this helm chart is unusable

@dbt-lucka
Copy link

dbt-lucka commented Nov 14, 2023

It's funny that someone re-rerequested this PR 5 days ago, because I am basically blocked too.
Is there anything I could support in getting this PR merged?
The FTP service is basically unusable or I have to create another Kubernetes service which would probably get its own loadbalancer created 🤔

Edit: You can use Kustomize with helm post-renderers to get the additional port exposures in.
In case you're using Flux it has OOTB support for post-renderer execution.
This works:
https://fluxcd.io/flux/components/helm/helmreleases/#post-renderers

@gioppoluca
Copy link

+1 to this

@BabisK
Copy link

BabisK commented Jan 28, 2024

I agree, consider merging this

@eduardodbr
Copy link

+1 to this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants