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

DNM/WIP dont try to listen on the advertise address #98

Closed
wants to merge 3 commits into from

Conversation

dcd
Copy link
Contributor

@dcd dcd commented Sep 29, 2020

  • sometimes docker might not be in use, but you still want to tell clients a different IP than you bind to inside the container

@tgorgdotcom this is here for your consideration, i am not really a python guy and haven't put a ton of thought into this yet. however, the current is_docker function is also breaking for k8s (even when actually using docker), and there is no guarantee that docker will be the CRI.

dcd added 3 commits September 28, 2020 22:47
* sometimes docker might not be in use, but you still want to tell clients a different IP than you bind to
* this doesnt work on k8s

* its a container, always listen on 0.0.0.0
@dcd
Copy link
Contributor Author

dcd commented Sep 29, 2020

so the first push didnt actually work , i made a cheap workaround to get service back, but i doubt its how you want it in the end.

the is_docker check does not work on k8s, and docker may not be the container runtime in use anyways.

i think the preferred solution will probably be to introduce a new parameter for listen_address, but i also dont understand the use case where a user would not desire 0.0.0.0

@dcd dcd changed the title DNM/WIP use a param for advertise ip instead of detecting docker DNM/WIP dont try to listen on the advertise address Sep 29, 2020
@deathbybandaid
Copy link
Contributor

#92 has some stuff built in to address this

@tgorgdotcom
Copy link
Owner

Hello dcd. I think I'll incorporate this manually as I'm working on some other issues. One thought I wanted to run by you -- What would you think of the following logic:

  • add bind_ip and bind_port to main config
  • change is_docker to rely on a file created by the Dockerfile (RUN touch is_docker or the like)
  • when we need to figure out what port to bind to, rely on the following rules:
    • If bind_ip and bind_port are set, use these settings
    • If bind_ip/bind_port is not set use our default way to figure out ports:
      • If using docker, rely on 0.0.0.0
      • If not using docker, rely on the plex_accessible_ip/plex_accessible_port

I want to change plex_accessible_ip/plex_accessible_port to advertise_ip/advertise_post like you did, but I want to make sure we don't do any settings-breaking changes until a major release. Let me know if this is good logic for your setup.

@deathbybandaid
Copy link
Contributor

For fHDHR, I use address and discovery_address.
SSDP does no good with 0.0.0.0

It will try to use discovery_address, and if not set, it will try to use address. If whichever value it uses is 0.0.0.0 ssdp simply does not run.

If the config defaults to 0.0.0.0 for address, and docker users don't override that in their config file, then the is_docker() is not really needed.

#101 goes hand in hand with this. If 0.0.0.0, the lineup won't work well. Gathering the base_url at the time of client connect is an easy way for 0.0.0.0 to work properly.

Honestly, for fHDHR, runnng locally, I purposely don't use ssdp. It only 'helps' with auto discovery in Plex. It's easy enough to manually insert the ip:port and not run an extra thread.

@tgorgdotcom
Copy link
Owner

tgorgdotcom commented Oct 28, 2020

Right. bind_ip/bind_port would only be used as the listen address, not the address returned in an SSDP reply message or the value in . I think you're right about the is_docker function -- we probably don't need it.

So maybe it should be more like this:

  • add bind_ip and bind_port to main config
  • remove is_docker function entirely
  • when we need to figure out what IP/port to listen on, rely on the following rules:
    • If bind_ip and bind_port are set, use these settings
    • If not, default to 0.0.0.0/6077
  • when we need to figure out what IP/port to report as the server IP, rely on the following rules:
    • use advertise_ip (renamed from plex_accessible_ip) for ip
    • use advertise_post (renamed from plex_accessible_port) or 6077 for port

I'll try to see if I can change it in 2.x, as this changes some config names and meanings

@deathbybandaid
Copy link
Contributor

I don't think you need 2 port settings

@tgorgdotcom tgorgdotcom deleted the branch tgorgdotcom:master November 26, 2020 04:23
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