-
-
Notifications
You must be signed in to change notification settings - Fork 629
Base context path can be changed (#88) #1424
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
base: master
Are you sure you want to change the base?
Conversation
- based on work done here:
https://github.com/simonhammes/gatus/tree/88-dynamic-base
- Additions:
- docker compose for testing reverse proxy scenario
- rolled back 'SERVER_URL' in order to allow hosting ui via node during development
- updated manifest.json so it will honour relative paths
7c42ced to
10d5114
Compare
README.md
Outdated
| | `ui.dashboard-subheading` | Dashboard description between header and endpoints | `Monitor the health of your endpoints in real-time` | | ||
| | `ui.header` | Header at the top of the dashboard. | `Gatus` | | ||
| | `ui.logo` | URL to the logo to display. | `""` | | ||
| | `ui.base` | `href` attribute of the HTML `<base>` tag. Use this if you want to host Gatus on a subpath (e.g. `/status/`). Has to end with '/'. | `/` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this option might actually be better fit in the web config section and maybe also renaming it to make it easier to read without the actually reading the docs: web.base-path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This time I'll ask before doing anything. The value of the 'base path' is needed by the 'UI config' (to render index.html) and by the 'security config' (for proper redirects and cookie paths). Currently, these parts are quite isolated, and I'm about to introduce a dependency from UI & security to web config. I can think of a plethora of ways of doing it, but I get a feeling there will be a few back-and-forth discussions over it 😉. What do you think about passing 'basePath' as an argument to 'ValidateAndSetDefaults' in these different configuration objects that need it? This way it won't introduce dependencies to 'web' config everywhere, will be easily mocked in tests, and this method is mutating everything anyway...or is somewhere else a pattern I can use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently commited version does that. It is by no means final; still missing some validation and tests, but it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about passing 'basePath' as an argument to 'ValidateAndSetDefaults' in these different configuration objects that need it?
I think your suggested solution passing it as argument might not scale very well in case there are more options that have the same problem are added in the future.
I opened a PR on your fork with another solution that so far looks like it is consistent with the MaximumNumberOfResults storage option where we would not need to add any additional arguments or package dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this 'your' way has a drawback, that it makes it difficult to validate the RedirectURL in oidc configuration, as validation occurs before settings are distributed. But I'm not entirely sure there is a universe where not including a base-path in redirect url is a desired thing.
Anyway I think I've finished.
README.md
Outdated
| Currently, you can expose the Gatus UI using a fully qualified domain name (FQDN) such as `status.example.org`. However, it does not support path-based routing, which means you cannot expose it through a URL like `example.org/status/`. | ||
|
|
||
| For more information, see https://github.com/TwiN/gatus/issues/88. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be updated
refactor: Update how base-path cross config is handled
| } | ||
|
|
||
| func (c *OIDCConfig) loginHandler(ctx *fiber.Ctx) error { | ||
| // ??? state and nonce are not 'secure random' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not quite understand what you mean. Can you elaborate on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself to think about it and open separate issue. Didn't intend to push it. Not related to this PR.
What I was wondering was that 'state' in oauth flow is kinda important. This value should not be easily guessable. I don't know how UUID is implemented here but UUIDs are usually quite 'guessable'. By 'secure random' i meant something like https://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html. crypto in go probably has something like this.
|
We missed a location where we need to update the base path: gatus/web/app/src/components/Settings.vue Lines 133 to 135 in 862613e
|
|
There is #1430, I think i'll wait with rebase until it gets merged. |
| }, | ||
| serverUrl: { | ||
| type: String, | ||
| default: '..' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the default also be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. In my opinion default should be removed. It is used only once, and where it is used it is set explicitly. Default for this kind of stuff does not make much sense to me. If it is not set consequently throughout the application it will not work, but will not fail during type checking / linting.
Summary
Issue: #88 (changing base context root for hosting behind url rewriting reverse proxy)
based on work done here: https://github.com/simonhammes/gatus/tree/88-dynamic-base, that exists as a draft PR (Add subpath support #908)
Additions:
Checklist
README.md.Notes: