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

Adjusting SSL Portion of HealthCheck #1827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dintho
Copy link

@dintho dintho commented Mar 10, 2025

dintho Quick dintho /patch-1 → Lissy93/dashy Commits: 1 | Files Changed: 1 | Additions: 8 Label Unchecked Tasks Powered by Pull Request Badge

Category:

Bugfix

Overview

I ran into a issue running dashy in a podman oci container with the default value for public & private key paths not being set the same way in healthcheck.js vs ssl-server.js

This difference was allowing SSL to start but causing the healthcheck to get a 302 redirect to https as the criteria for checking
the isSsl in healthcheck.jswas not the same as whatenableSSLis using inssl-server.js`

hopefully this change removes the need for SSL_PUB_KEY_PATH & SSL_PRIV_KEY_PATH to also be set as env vars in the compose file (although i assume with the nature of how the check is currently written bare metal installs have the same issue)

Issue Number (if applicable) #00

This probably solves #843 and #1410 i do not think this totally solves #768.

i had a hard time following that thread on #768 and if it is the same issue then awesome!!! but if not may need to unlink it from the other 2 issues because issue scope creep might be a thing there.

side note: if this does not solve #768 please let me know if i can be of any help there as well. i wasnt able to follow it and understand how it was related to the SSL issues but am happy to help with it if i can

New Vars (if applicable)

this change copies const fs & const httpsCerts from ssl-server.js and adjusts the logic of const isSsl

side note: i realize httpsCerts is probably not the right name for the constant in healthcheck.js but i kept it the same as ssl-server.js to hopefully help maintainability in the future by others

Screenshot (if applicable)

N/A

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • (If significant change) Bumps version in package.json

I ran into a issue running dashy in a podman oci container with the default value for public & private key paths  not being set the same way in healthcheck.js vs ssl-server.js

This difference was allowing SSL to start causing the healthcheck to get a 302 redirect to https .

This change adds in the same default path vars and i am going to say similar behavior that ssl-server.js uses. i tried to match the same style as the existing code between the two files

another alternative is to set `SSL_PUB_KEY_PATH` and `SSL_PRIV_KEY_PATH` in the compose file as env vars 

but after seeing Lissy93#843 which lead me Lissy93#768 and then finding  
Lissy93#1410

ill be honest this probably solves Lissy93#843 and Lissy93#1410  i dont think this totally solved Lissy93#768  tbh.  i had a hard time following that thread
@dintho dintho requested a review from Lissy93 as a code owner March 10, 2025 23:36
Copy link

netlify bot commented Mar 10, 2025

Deploy Preview for dashy-dev ready!

Name Link
🔨 Latest commit a130d6c
🔍 Latest deploy log https://app.netlify.com/sites/dashy-dev/deploys/67cf777ba73ee30008956f52
😎 Deploy Preview https://deploy-preview-1827--dashy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

1 participant