SensorsIot / IOTstack

Docker stack for getting started on IOT on the Raspberry PI
GNU General Public License v3.0
1.45k stars 308 forks source link

Adding new containers removes portainer HTTPS port #670

Closed distante closed 1 year ago

distante commented 1 year ago

When enabled, portainer exposes the UI with port 9443, since IOTstack does not add that port to the docker-compose.yml file, one has to add it manually and then call docker compose up -d.

The problem is, that when a new container is added thru menu.sh the new added port fwd 9443:9443 is deleted.

Since version 2.9 Portainer CE has HTTPS enabled by default. (https://docs.portainer.io/start/upgrade/docker#upgrading-your-portainer-server)

Paraphraser commented 1 year ago

Good catch. But I see a problem:

$ cat ~/IOTstack/.templates/blynk_server/service.yml 
blynk_server:
  build:
    context: ./.templates/blynk_server/.
    args:
      - BLYNK_SERVER_VERSION=0.41.16
  container_name: blynk_server
  restart: unless-stopped
  environment:
    - TZ=Etc/UTC
    - IOTSTACK_UID=1000
    - IOTSTACK_GID=1000
  ports:
    - "8180:8080"
    - "8440:8440"
    - "9443:9443"
  volumes:
    - ./volumes/blynk_server/data:/data
    - ./volumes/blynk_server/config:/config

In other words, any time someone selects both blynk_server and portainer-ce, docker-compose will report a conflict on port 9443.

Generally, we resolve potential conflicts "first come, first served". Blynk was here first so Portainer probably needs to be:

- "9444:9443"

Some other considerations:

  1. I'm adding a cross-reference to #652 so @pieslinger can add whatever port number you settle on to that pending PR.
  2. You might want to add the port number you settle on to:

    ~/IOTstack/docs/Containers/Portainer-ce.md
pieslinger commented 1 year ago

Looking here it appears blynk will be shut down.

http://docs.blynk.cc/

The github project that IOTstack uses in the dockerfile for blynk also appears to be unsupported.

https://github.com/Peterkn2001/blynk-server

Assuming Portainer will be supported moving forward - it might make sense to give priority to Portainer here and have the 9444:9443 mapping be used in the blynk service.yml.

Thoughts?

Paraphraser commented 1 year ago

So I gather (shut down - although @877dev thought it was just a transient - see also #435) . That's why the IOTstack container builds locally.

Not being a user of this thing, I can't really offer very much. But I will say that it's a bit of a dangerous precedent to depart from "first come, first served". You do it once and, suddenly, a whole bunch of "reasons" get invented as to why A deserves priority over B. It's always difficult to push back when there's an example of where you departed from principle.

The underlying problem is we have no way of updating existing stacks so moving ports creates the potential for collisions.

I never worry about people who can look at the situation, realise what's happening, and decide the ports for themselves. The people I worry about are those who start issues by writing "I don't know all that much about [Unix|Docker]".

pieslinger commented 1 year ago

Yeah I can appreciate the continuity and avoiding slippery slopes. I was just throwing that out there.

Although, if you want to keep it simple for new users(of which I am not far removed), allowing me to install blynk may be problematic given its lack of future support.

Do “we” have a plan to depreciate or remove containers that are no longer being supported?

Maybe leave them in the project but with some sort of caveat?

I would assume that IOTstack will keep running into this. Going through the default configs, I noticed many older projects - I think dash machine is another - that no longer receive regular updates.

I could help with that if needed.

Paraphraser commented 1 year ago

I don't think "we" do. I agree that this is a potential problem because presence in the template implies support.

"We" do have a precedent, of sorts, in Supervised Home Assistant (the non-container variant) which used to be an add-on supported through the menu. After several iterations where it didn't work and upstream workarounds stopped being supported (kinda like the Peterkn2001 situation) I got annoyed by the combination of the moving target and the inherent delays in getting PRs into production for IOTstack. I figured out how to build a proper install as part of PiBuilder and, at some point along the journey, rather than also try to keep the IOTstack build working (which was tricky because of ordering issues), I put in a PR removing it from the IOTstack menu. That was all just about bedded down when the folks at HA decided it was their way or the highway as a dedicated appliance. I gave up and pulled it from PiBuilder too.

It's not a particularly good example because, at each waypoint in the journey I've just described, S.HA simply did not work. Anyone with a working implementation prior to the break would continue to be happy, until they tried to "do something", after which they were up the same creek as a Johnny-come-lately.

In general, someone with a working instance of container X which stops being supported will continue to be happy, probably up until the moment when they try to build a new system and find X is no longer on DockerHub or, if X uses a local Dockerfile, find it will no longer build. Thus it's probably the user who fits the definition of a Johnny-come-lately we need to care about and warn off casual adoption. I've just done some tinkering and this might work. Suppose I wanted to stick a deprecation notice on Node-RED:

nodered:
  x-deprecated: ${DEPRECATION_WARNING:?Node-RED is deprecated}

The x- on x-deprecated is treated like a comment so docker-compose accepts the line without complaint. But the right hand side is evaluated for substitution. Providing DEPRECATION_WARNING isn't defined as an environment variable, the whole arrangement will produce:

$ docker-compose up -d nodered
invalid interpolation format for services.nodered.x-deprecated.
You may need to escape any $ with another $.
required variable DEPRECATION_WARNING is missing a value: Node-RED is deprecated

It's not elegant but it does get the job done in the sense of focusing your attention on Node-RED and giving you some idea of what is going on. I can silence the warning with:

$ export DEPRECATION_WARNING=ignore
$ docker-compose up -d nodered
[+] Running 1/1
 ⠿ Container nodered  Started                                                                                                                                                                                                                                                                                                                                          0.9s

If we expand on this theme, something like:

nodered:
  x-deprecated: ${DEPRECATION_WARNING:?Node-RED is deprecated}
  # Node-RED has been deprecated and will be removed from IOTstack on 2024-02-28
  # If you depend on Node-RED, please open an issue before 2024-02-28
  # To silence this warning, remove or comment out the "x-deprecated" line.

Either that or someone knuckles down and adds smarts to the menu to handle this kind of stuff.


ps:

pieslinger commented 1 year ago

yeah my spell check got me there.

distante commented 1 year ago

I am just a Javascript/Typescript guy, but, referring to

The underlying problem is we have no way of updating existing stacks so moving ports creates the potential for collisions.

If I had to afront that on a NodeJs environment, I would convert the docker-compose.yml file and the new selected service.yml file to JSON objects so I could compare each key (or maybe just some of them, like environment and ports), and if the existent docker service declaration has more information, I will add it to the in-memory service JSON and then use that as a buffer to write the updated docker-compose.yml