Added Hass.io Ingress support by ammmze · Pull Request #63 · hassio-addons/app-tasmoadmin

ammmze · 2019-12-18T04:03:41Z

Proposed Changes

Added Hass.io Ingress support

Related Issues

#44, #47, #59, #62

addons-assistant · 2019-12-18T04:03:45Z

💖 Thanks for opening this pull request! 💖 If your PR gets accepted and merged in, we will invite you to the project 🎉

ammmze · 2019-12-18T04:06:19Z

+    location /data/firmwares {
+    }
+
+    location ^~ /data/ {


Note: I added ^~ to this line so that it actually matched. In the current master version I found I was able to access /data/MyConfig.json which exposes your credentials.

frenck · 2019-12-18T09:15:41Z

    \
    && apk add --no-cache --virtual .build-dependencies \
-        git=2.22.0-r0 \
+        git=2.22.2-r0 \


This should not be in this PR.

The docker image failed to build without it...so 🤷‍♂

frenck · 2019-12-18T09:15:58Z

  "hassio_api": true,
  "hassio_role": "default",
+  "ingress": true,
+  "ingress_port": 8099,


Is there a reason why the port is hardcoded for this add-on?

The reasoning behind it is that the port is hard coded in the nginx ingress, so should hass.io ever change it's default port, it wouldn't matter as we have explicitly defined it here.

frenck · 2019-12-18T09:16:12Z

  ],
  "ports": {
-    "9541/tcp": 9541
+    "9541/tcp": null


That seems like a bad idea. How do the firmware updates get across?

TBH, I haven't used the firmware update process. Initially I figured it would go through the ingress, but I guess that wouldn't work since it would need to be authenticated.

frenck · 2019-12-18T09:16:30Z

    "certfile": "fullchain.pem",
    "keyfile": "privkey.pem"
  },
+  "panel_icon": "mdi:network",


Does this fit?

frenck · 2019-12-18T09:17:42Z

+# ==============================================================================
+
+
+# The general idea is that the nginx server will be configured to have a root of


I like comments in code, but this extreme.

frenck · 2019-12-18T09:18:28Z

+declare ingress_dir="${ingress_root_dir}${ingress_entry}"
+
+# Create nginx server
+sed "s#%%ingress_entry%%#${ingress_entry}#g" \


Please follow the standard as we do in the community add-ons. I like to keep method the same across all add-ons to be able to maintain things.

frenck · 2019-12-18T09:19:15Z

+awk '1;/public function read\(/{ print "if ($key == \"login\" && @$_SERVER[\"ASSUMED_AUTH\"] == \"1\") return \"0\";"}' \
+    "${app_root_dir}/includes/Config.php" > "${app_root_dir}/includes/IngressConfig.php"
+
+cat << EOF > "${app_root_dir}/includes/IngressInit.php"


There is no reason to do this in Bash.

frenck · 2019-12-18T09:19:44Z


-    sed -i "s#%%certfile%%#${certfile}#g" /etc/nginx/nginx.conf
-    sed -i "s#%%keyfile%%#${keyfile}#g" /etc/nginx/nginx.conf
+    sed "s#%%certfile%%#${certfile}#g ; s#%%keyfile%%#${keyfile}#g" /etc/nginx/servers/direct-ssl.disabled > /etc/nginx/servers/direct-ssl.conf


Please don't combine replacements in a single call.

frenck · 2019-12-18T09:21:11Z

@@ -0,0 +1,47 @@
+#!/usr/bin/with-contenv bashio


Ingress is not an application. Please split across the nginx.sh and tasmoadmin.sh

frenck · 2019-12-18T09:22:09Z

-            rewrite ^/([a-zA-Z_]+)/?$ /index.php?page=$1;
-        }
-    }
+    include /etc/nginx/servers/*.conf;


Please use the Nginx config structure as used by other community add-ons as well.
Which is not this. Keeping things similar makes maintenance easier.

This was how vscode did it... https://github.com/hassio-addons/addon-vscode/tree/master/vscode/rootfs/etc/nginx/servers

frenck · 2019-12-18T09:25:30Z

Thanks for your work, but this is made from a single add-on perspective (instead of the similarity between all the add-on in this repository).

One of the main reasons to use TasmoAdmin is to manage your Tasmota devices across your network centrally, including firmware upgrades. This makes this add-on unsuitable for Ingress from a general standpoint.

I'm really not keen on supporting Ingress in this application.
Therefore, I'm going to close this PR. Thanks for you contribution though! 👍

ghost · 2019-12-18T18:12:44Z

If you not support ingress for me it is not addon for Home Assistant. It is as other external software. If it is addon to Hassio i think that should supoort ingress. It is my opinion. We need central managment for all and if we install in Hassio we want have access from Hassio GUI. Look many people need this in GUI as side bar - Ingrees

ammmze · 2019-12-19T05:49:45Z

I completely agree! Honestly the only rub I see is that without also exposing the container port then you can't do firmware upgrades of your devices. But I imagine an enhancement to the hass.io ingress could be made to allow an addon to whitelist some anonymous paths which would then allow our devices access to the firmware files and avoid the need to expose the port. But until that happens, I think opening the port is a fine workaround but we'd still get the benefit of easy access to the tasmoadmin web ui to manage the device settings.

frenck · 2019-12-19T07:46:48Z

If you not support ingress for me it is not addon for Home Assistant.

That is nice, but not all add-ons are capable or sane to use Ingress on. Ingress is a feature, not a requirement. For example, some add-ons require non-TCP ports, others serve things that are not "inside" Home Assistant or the application itself is simply not able to handle dynamic subfolders in requests paths.

We need central managment for all and if we install in Hassio we want have access from Hassio GUI

Home Assistant has offered a iframe_panel for that for ages already.

Honestly the only rub I see is that without also exposing the container port then you can't do firmware upgrades of your devices

Which is one of the main goals of this project! Centrally managing your Tasmota Devices. For control, we use Home Assistant, right?

But I imagine an enhancement to the hass.io ingress could be made to allow an addon to whitelist some anonymous

That is not how Ingress works.

ammmze · 2019-12-19T15:24:14Z

Home Assistant has offered a iframe_panel for that for ages already.

But that requires that I expose that port externally just to manage the devices from the UI. The purpose of the ingress is to allow access to the web applications without having to expose a given port.

frenck · 2019-12-19T23:50:11Z

But that requires that I expose that port externally just to manage the devices from the UI.

Only if you actually want to upgrade/configure devices when you are externally yourself, other cases that seems not needed.

ammmze · 2019-12-20T19:38:21Z

Okay...I appreciate the little bit of direction in the review, i've made many updates...feel free to continue to close the Issues that I'm sure will continue to roll in ;)

If anyone wants a version of the TasmoAdmin addon with Ingress support, i've setup a hass.io repository you can add: https://github.com/MashupMill/hassio-addons

frenck · 2019-12-20T19:42:00Z

@ammmze In all fairness and friendliness, that is really short-sighted to say like that. Some users want A others want B, there is no possible way to please everyone. For example, Grafana added Ingress, and I'm still closing issues that continue to roll in asking to remove it.

So it is not black or white, it is a choice that I made carefully. I'm fine with somebody not to agree on that. That is what forks are for 👍

ammmze · 2019-12-20T19:59:01Z

Not trying to be insulting or anything. Though to be honest it seems like it would be relatively easy to have a configuration option to enable/disable the ingress which gives the user the ability to choose. After all, Home Assistant was designed to give the user the ability to customize, why should the hass.io add-ons be any different.

frenck · 2019-12-20T20:07:52Z

Hass.io cannot disable Ingress, it is statically enabled in the add-on configuration.

ammmze · 2019-12-20T20:11:23Z

Sounds like an opportunity for improvement! If nothing else disable the ingress server in nginx though maybe hass.io may consider it as not having come up...idk. My point is that we're not at the end of the road. Things can (and should) change and be enhanced over time.

addons-assistant · 2020-01-19T20:28:42Z

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

bump git dependency to 2.22.2-r0

1d59072

ammmze commented Dec 18, 2019

View reviewed changes

configure for ingress

e893014

ammmze force-pushed the master branch from ab42625 to e893014 Compare December 18, 2019 04:06

ammmze commented Dec 18, 2019

View reviewed changes

Comment thread tasmoadmin/config.json

frenck reviewed Dec 18, 2019

View reviewed changes

frenck closed this Dec 18, 2019

ascillato mentioned this pull request Dec 20, 2019

Add the possibility to use Ingress feature in HA TasmoAdmin/TasmoAdmin#330

Closed

addons-assistant Bot locked as resolved and limited conversation to collaborators Jan 19, 2020

		# ==============================================================================


		# The general idea is that the nginx server will be configured to have a root of

Uh oh!

Uh oh!

Conversation

ammmze commented Dec 18, 2019

Proposed Changes

Related Issues

Uh oh!

addons-assistant Bot commented Dec 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frenck commented Dec 18, 2019

Uh oh!

ghost commented Dec 18, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ammmze commented Dec 19, 2019

Uh oh!

frenck commented Dec 19, 2019

Uh oh!

ammmze commented Dec 19, 2019

Uh oh!

frenck commented Dec 19, 2019

Uh oh!

ammmze commented Dec 20, 2019

Uh oh!

frenck commented Dec 20, 2019

Uh oh!

ammmze commented Dec 20, 2019

Uh oh!

frenck commented Dec 20, 2019

Uh oh!

ammmze commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addons-assistant Bot commented Jan 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented Dec 18, 2019 •

edited by ghost

Loading

ammmze commented Dec 20, 2019 •

edited

Loading