Skip to content

feat: add PROMETHEUS_ALLOWLIST for specific firewall IP rules - #102

Open
nfrozi wants to merge 3 commits into
carlocorradini:mainfrom
nfrozi:feature/firewall-enhancement
Open

feat: add PROMETHEUS_ALLOWLIST for specific firewall IP rules#102
nfrozi wants to merge 3 commits into
carlocorradini:mainfrom
nfrozi:feature/firewall-enhancement

Conversation

@nfrozi

@nfrozi nfrozi commented Jul 15, 2026

Copy link
Copy Markdown
- Add logic in install.sh to apply firewall rules only to specific IPs/CIDRs if PROMETHEUS_ALLOWLIST is defined.
- Update README.md environment variables table.
- Add 'proto' to cspell.json to fix spelling check failure.

    - Add logic in install.sh to apply firewall rules only to specific IPs/CIDRs if PROMETHEUS_ALLOWLIST is defined.
    - Update README.md environment variables table.
    - Add 'proto' to cspell.json to fix spelling check failure.
@nfrozi
nfrozi requested a review from carlocorradini as a code owner July 15, 2026 11:35
@carlocorradini

Copy link
Copy Markdown
Owner

Hey, thanks for your contribution! I have a few comments:

  1. AFAIK, PROMETHEUS_ALLOWLIST is not an official environment variable, so I would prefer renaming it to something more specific, such as INSTALL_NODE_EXPORTER_PROMETHEUS_ALLOWLIST, or perhaps a name that clearly indicates it is related to the firewall rule.
  2. Instead of splitting the logic and duplicating the firewall handling inside the firewall_rule function, I would prefer to expand the IP list only when the environment variable is defined; otherwise, it should remain an empty list. Then, for each firewall, we can iterate over this IP list. This way, we keep the implementation unified rather than having separate paths.

What do you think? 🤗

@nfrozi

nfrozi commented Jul 16, 2026

Copy link
Copy Markdown
Author

Thank you for the feedback.

  1. I agree that the environment variable should follow the existing naming convention for consistency. Apologies for the inconsistency on my side. I think INSTALL_NODE_EXPORTER_FIREWALL_ALLOWLIST would be more appropriate, especially since we already have other variables related to FIREWALL.
  2. I also agree with your suggestion regarding the firewall logic. Expanding the IP list only when the environment variable is defined and keeping it empty otherwise makes the implementation cleaner. Iterating over a single unified list for all firewall rules will help avoid duplication and keep the logic more maintainable.

@carlocorradini

Copy link
Copy Markdown
Owner

@nfrozi
Hey, sorry, I'm a bit busy at the moment.
I'll check it ASAP

Comment thread install.sh
fi

# shellcheck disable=SC2086
set -- $_ips

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why do you use set -- $_ips instead of working with $_ips directly?

Comment thread install.sh

_ips=""
if [ -n "$INSTALL_NODE_EXPORTER_FIREWALL_ALLOWLIST" ]; then
_ips=$(echo "$INSTALL_NODE_EXPORTER_FIREWALL_ALLOWLIST" | tr ',' ' ')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Use printf instead of echo

Comment thread install.sh

_ips=""
if [ -n "$INSTALL_NODE_EXPORTER_FIREWALL_ALLOWLIST" ]; then
_ips=$(echo "$INSTALL_NODE_EXPORTER_FIREWALL_ALLOWLIST" | tr ',' ' ')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe there's a better way to split the different IPs instead of relying on tr? Perhaps we can reuse something that's already used elsewhere and avoid introducing a new dependency on tr?

Comment thread install.sh
printf "%s\n%s\n" \
"$_firewall_path --add-port=$NODE_EXPORTER_PORT/tcp --permanent" \
"$_firewall_path --reload"
for _ip in "$@"; do

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not very familiar with configuring the various firewalls, so I need your help 🆘.
What happens if the IP address is an IPv6 rather than an IPv4?
Do we need to detect the IP version and pass different arguments accordingly, or does firewall-cmd handle that automatically?

More generally, does the same apply to the other firewall implementations?
Are they IP-version agnostic, or do we need to explicitly specify whether an address is IPv4 or IPv6? Thanks 🙌

Comment thread install.sh
# shellcheck disable=SC2086
set -- $_ips

case $FIREWALL in

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

When the IP list is empty, no firewall rules are added.
This is a regression compared to the previous behavior.

@carlocorradini

Copy link
Copy Markdown
Owner

@nfrozi Sorry for the late reply 😥🙌

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.

2 participants