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

[Pending umbrelOS feature] - App Submission: Omada Controller #1196

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hugofnm
Copy link
Contributor

@hugofnm hugofnm commented Jul 11, 2024

App Submission

App name

Omada Controller

256x256 SVG icon

(Submit an icon with no rounded corners as it will be dynamically rounded with CSS. GitHub doesn't allow uploading SVGs directly, so please upload your icon to an alternate service, like https://svgur.com, and paste the link below.)
We will help finalize this icon before the app goes live in the Umbrel App Store.

https://www.tp-link.com/res/images/icons/tag-omada.svg

Gallery images

(Upload 3 to 5 high-quality gallery images (1440x900px) of your app in PNG format, or just upload 3 to 5 screenshots of your app and we'll help you design the gallery images.)
We will help finalize these images before the app goes live in the Umbrel App Store.

Screenshot1
Screenshot2
Screenshot3

I have tested my app on:

  • umbrelOS on a Raspberry Pi
  • umbrelOS on an Umbrel Home
  • umbrelOS on Linux VM

Copy link
Contributor

@sharknoon sharknoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @hugofnm for your awesome submission! I have left some suggestions for this PR. Sorry that you had to wait so long, but we have just finished our linter for any new app submissions. You will see it when you are adding new commits to this PR :)

omada-controller/umbrel-app.yml Outdated Show resolved Hide resolved
omada-controller/umbrel-app.yml Outdated Show resolved Hide resolved
omada-controller/umbrel-app.yml Outdated Show resolved Hide resolved
omada-controller/umbrel-app.yml Show resolved Hide resolved
omada-controller/umbrel-app.yml Show resolved Hide resolved
omada-controller/docker-compose.yml Outdated Show resolved Hide resolved
@hugofnm
Copy link
Contributor Author

hugofnm commented Jul 27, 2024

Thanks @sharknoon for the suggestions !
Should be good to go :)

@sharknoon
Copy link
Contributor

Awesome work @hugofnm! Our new linter is also happy 😀
@nmfretz I think this app is ready to go

@nmfretz
Copy link
Contributor

nmfretz commented Jul 29, 2024

Thanks for the ping and review @sharknoon 🤝.

And great work packaging Omada Controller @hugofnm!

The only outstanding task is to avoid potential port clashes with Omada Controller running in host networking mode. It’s on the umbrelOS roadmap to mitigate port clashes automatically, but this currently isn't implemented, and port 8088 is in use by Element: https://github.com/getumbrel/umbrel-apps/blob/master/element/umbrel-app.yml

This means if a user tries to run both Omada Controller and Element on the same device, there will be a port clash, causing one of the apps to fail to launch (or fail to install).

To mitigate this, here are the ports Omada Controller uses:
https://github.com/getumbrel/umbrel-apps/blob/master/element/umbrel-app.yml

We don’t need to worry about UDP ports, as they can be shared. The only one we need to plan for right now is 8088 I believe.

Here are the env vars we have access to:
https://github.com/getumbrel/umbrel-apps/blob/master/element/umbrel-app.yml

So we can use MANAGE_HTTP_PORT to change 8088 to a free port.

@hugofnm
Copy link
Contributor Author

hugofnm commented Jul 30, 2024

Hi @nmfretz 👋,
I've changed the HTTP port 8088 -> 8078 as suggested.

Looking at the official documentation (https://www.tp-link.com/us/support/faq/3281/), it seems that Omada Controller uses about 10 different TCP ports for device discovery, web management, ...
Is there a spreadsheet or something like that to check port availability and avoid conflicts ?

Also, should this app stay on host mode or should I expose each port individually?

Copy link

github-actions bot commented Aug 5, 2024

🎉   Linting finished with no errors or warnings   🎉

Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store.

Please review the linting results below and make any necessary changes to your submission.

Linting Results

Severity File Description
ℹ️ omada-controller/docker-compose.yml Service "server" uses host network mode:
The host network mode can lead to security vulnerabilities. If possible please use the default bridge network mode and expose the necessary ports.

Legend

Symbol Description
Error: This must be resolved before this PR can be merged.
⚠️ Warning: This is highly encouraged to be resolved, but is not strictly mandatory.
ℹ️ Info: This is just for your information.

@nmfretz
Copy link
Contributor

nmfretz commented Aug 5, 2024

I've changed the HTTP port 8088 -> 8078 as suggested.
Also, should this app stay on host mode or should I expose each port individually?

Thanks for doing that @hugofnm. With this port change, let's leave this in host network mode because Omada will need this for device discovery.

Gallery assets are here: https://github.com/getumbrel/umbrel-apps-gallery/tree/master/omada-controller

@hugofnm @sharknoon, I have made some small tweaks and have tested the app. One thing to look into before this goes live is the ssl cert experience. Currently, users who install Omada Controller will be met with a warning in their browser that the connection is not secure.

image

@hugofnm do you happen to know whether we can avoid this entirely via compose configuration? We should really try to avoid encouraging users to click past these warnings, but as a last resort we could include instructions in the app's description.

@sharknoon
Copy link
Contributor

@hugofnm Is there a way to disable the HTTPS endpoint completly and only use the HTTP one?

@hugofnm
Copy link
Contributor Author

hugofnm commented Aug 6, 2024

Well... I'm afraid it's not possible to disable the HTTPS endpoint @sharknoon
Omada Controller is closed-source and documentation is rare...
The only "way around" it's to disable HTTPS redirect, but you can only disable it in the Web UI.

image

@sharknoon
Copy link
Contributor

Thank you for your answer @hugofnm. I guess we have to accept the warning screen then, until umbrelOS supports domains and therefore HTTPS. Whats your opinion @nmfretz?

@nmfretz
Copy link
Contributor

nmfretz commented Sep 5, 2024

Well... I'm afraid it's not possible to disable the HTTPS endpoint @sharknoon
Omada Controller is closed-source and documentation is rare...
The only "way around" it's to disable HTTPS redirect, but you can only disable it in the Web UI.

Darn, that's a shame. This means that the flow for a user installing Omada Controller would be:

  • Open the app, which will redirect to https. User will be met with a scary warning that we shouldn't be encouraging users to click past.
  • Click past the warning.
  • Set up an account
  • Edit the settings in the UI to disable https redirects.
  • Restart the app from the umbrelOS homescreen (it's required to restart the omada container apparently when changing the redirect setting).

Great work packaging this app @hugofnm. The effort is super appreciated. Unfortunately, I think we should wait to bring this to the official app store until after umbrelOS has integrated HTTPS support, which is on the horizon. That way users of all technical skill levels can easily use the app and we aren't providing a poor experience as a result of the OS.

Would you prefer that we convert this PR to draft but keep it open, or would you prefer to close this PR and reopen once HTTPS support is available?

In the meantime people could still run Omada Controller on their Umbrel by either:

  1. Installing it from a Community App Store https://github.com/getumbrel/umbrel-community-app-store, or
  2. Installing it via the Portainer app: https://apps.umbrel.com/app/portainer

@hugofnm
Copy link
Contributor Author

hugofnm commented Sep 5, 2024

Sure, I'll convert this PR to draft until Umbrel supports HTTPS !

@hugofnm hugofnm marked this pull request as draft September 5, 2024 21:10
@nmfretz
Copy link
Contributor

nmfretz commented Sep 5, 2024

Sounds good, thanks @hugofnm 🤝

@nmfretz nmfretz changed the title App Submission: Omada Controller [Awaiting umbrelOS feature] - App Submission: Omada Controller Sep 6, 2024
@nmfretz nmfretz changed the title [Awaiting umbrelOS feature] - App Submission: Omada Controller [Pending umbrelOS feature] - App Submission: Omada Controller Sep 6, 2024
@nmfretz
Copy link
Contributor

nmfretz commented Sep 11, 2024

Reminder for us: gallery assets have already been prepared for Omada Controller

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