search

saltstack-formulas / apache-formula

Set up and configure the Apache HTTP server
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 285 forks source link

Allow remote servers to access server status page #273

Closed mcarlton00 closed 4 years ago

mcarlton00 commented 4 years ago

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Allow remote servers to read the Apache status page for monitoring purposes

Pillar / config required to test the proposed changes

apache:
  server_status_access:
    ip:
      - 10.8.8.0/24
    host:
      - foo.example.com

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

myii commented 4 years ago

@mcarlton00 This formula also uses semantic-release so the commit messages are used to evaluate the release. Please see the following section for more info:

You can see how the current commit messages were evaluated by commitlint:

mcarlton00 commented 4 years ago

I'd bounced back and forth between _access and _require and which one made more sense, and then wound up mixing them here. Go figure. Is there a preference of which one you think is clearer?

I'll merge the commits with the right message when I push the updates.

myii commented 4 years ago

I'd bounced back and forth between _access and _require and which one made more sense, and then wound up mixing them here. Go figure. Is there a preference of which one you think is clearer?

@mcarlton00 Probably to go with the "2.4" option, i.e. _require.

I've simulated these changes in Travis and all looks to be working fine:

So for 2.4:

<Location "/server-status">
    SetHandler server-status
    Require local
    Require host foo.example.com
    Require host bar.example.com
    Require ip 10.8.8.0/24
    Require ip 10.10.0.0/16
</Location>

And for 2.2:

<Location "/server-status">
    SetHandler server-status
    Order deny,allow
    Deny from all
    Allow from localhost
    Allow from foo.example.com
    Allow from bar.example.com
    Allow from 10.8.8.0/24
    Allow from 10.10.0.0/16
</Location>

Relevant docs:

myii commented 4 years ago

@mcarlton00 Just a minor change for that commit message:

-feat(server-status): Allow remote servers to reach server-status page
+feat(server-status): allow remote servers to reach server-status page
myii commented 4 years ago

Merged, thanks for the contribution @mcarlton00. One thing we didn't cover here was any documentation changes. Do you think it's worth adding a note under the following section?

saltstack-formulas-travis commented 4 years ago

:tada: This PR is included in version 0.39.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

mcarlton00 commented 4 years ago

It probably should be adjusted some, yeah. I didn't realize until now that it only says localhost there, which could be misleading at this point. It still defaults to localhost only, but could be added to via the pillar, so I'm not sure what the best phrasing for it would be.

myii commented 4 years ago

@mcarlton00 Something based on your description above would be useful:

Allow remote servers to read the Apache status page for monitoring purposes

Also a mention of the section in pillar.example to refer back to.

mcarlton00 commented 4 years ago

Want a new PR for that? afaik, updating a merged branch won't get us anywhere

myii commented 4 years ago

@mcarlton00 Sure, that's the right way to proceed.