feat: make it possible to restrict control server commands to stats#3787
feat: make it possible to restrict control server commands to stats#3787MSP-Greg merged 2 commits intopuma:masterfrom
Conversation
|
@schneems What do you think of this pull request? |
|
I have thought about this previously. I was thinking more a 'data_only' switch, so the command strings weren't needed. Another option would be two passwords, one for 'admin/all' access, another (or nil) for 'data only'. |
a86df44 to
b6fced0
Compare
|
@MSP-Greg Ok, I updated this pull request accordingly. I didn't include |
b6fced0 to
d865c6d
Compare
|
To the above message I should have added "Let's see if @schneems has any thoughts". Otherwise, LGTM. Nit pick: data_only = @options[:control_data_only]
app = Puma::App::Status.new @launcher, token, data_onlySince app = Puma::App::Status.new @launcher, token, @options[:control_data_only] |
d865c6d to
32ff524
Compare
32ff524 to
5943b48
Compare
|
Just pushed a slight correction to fix the commit message to |
| # @param data_only [Boolean] if true, restrict to read-only data commands | ||
| # | ||
| def initialize(launcher, token = nil) | ||
| def initialize(launcher, token = nil, data_only = false) |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm unsure if people use it directly
I've got several "Don't break" bookmarks, and I don't recall any using this directly.
you think it's okay to change token to also be a kwarg or is that breaking?
Do you really think these need to change to kwargs? Adding an additional positional at the end, where the proceeding arg also has a default value shouldn't be a problem. Also, I don't see the arg list growing. Granted, kwargs are 'identifying', it not like the init code is used frequently...
There was a problem hiding this comment.
I refactored this method using kwargs. I think it would be rare for projects to instantiate Puma::App::Status.new directly.
There was a problem hiding this comment.
This comes down to personal preference. I have ADHD and kwargs help offload some memory of “what is this art supposed to be representing” to the name. Makes it easier for me to spot mistakes.
the downside is that it’s slightly slower than an all-positional method call. But for object instantiation, especially one that’s not in a hot loop, it’s more than worth it (and maybe even in a hot loop might not move the needle, I’ve not actually benchmarked.
Don't break" bookmarks
If not already in existing docs, it would be a good PR to note this in the code.
In production, we want to make the `/stats` and `/gc-stats` endpoints
available, but we don't want to expose the ability to manipulate the
state of the processes.
This commit adds a `data_only` option. For example:
```
activate_control_app "tcp://127.0.0.1:8182", { data_only: true }
```
5943b48 to
ad53342
Compare
|
@schneems Thanks for the review! Could you take a look again? |
|
Thanks! Approved the change and restarted the failing test. |
In production, we want to make the
/statsand/gc-statsendpoints available, but we don't want to expose the ability to manipulate the state of the processes.This commit adds a
data_onlyoption. For example:Description
Thank you for contributing! You're the best.
We can read your code, so consider leaving some comments here that are more about your motivations and decision making. Some things that may be helpful to address in your description:
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.