Skip to content

Update gixy nginx config checker#2093

Draft
aequitas wants to merge 2 commits into
mainfrom
gixy-update
Draft

Update gixy nginx config checker#2093
aequitas wants to merge 2 commits into
mainfrom
gixy-update

Conversation

@aequitas

Copy link
Copy Markdown
Collaborator

No description provided.

@aequitas aequitas marked this pull request as draft June 19, 2026 10:47
@bwbroersma bwbroersma self-requested a review June 24, 2026 11:34

# monitoring, requires authentication, override headers, since CSP is too strict
location /grafana {
add_header_inherit off;

@bwbroersma bwbroersma Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The include below both have add_headers, therefore the default behaviour seems okay:

There could be several add_header directives. These directives are inherited from the previous configuration level if and only if there are no add_header directives defined on the current level. Inheritance rules can be redefined with the add_header_inherit directive (1.29.3).

Is the additional add_header_inherit really needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The new gixy-ng (https://github.com/dvershinin/gixy) fails on the current config because of the double headers https://github.com/internetstandards/Internet.nl/actions/runs/27760088839/job/82132454086#step:14:107. Maybe we can mark them so they are ignored (https://github.com/dvershinin/gixy/blob/master/docs/en/configuration.md)?. The only other issue found was a HSTS age of 0 which we also want to ignore. So the newer gixy didn't find anything interesting, maybe in future changes it might?

@bwbroersma bwbroersma Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Problem: [add_header_redefinition] Nested "add_header" drops parent headers.

Which is all by design, and the first time you work with nginx will find this out. I don't really get the warning, but I think we could add change:

Internet.nl/Makefile

Lines 313 to 315 in a5f620d

check-gixy: env=test
check-gixy: ## run nginx config check
${DOCKER_COMPOSE_CMD} exec webserver /opt/gixy/bin/gixy /etc/nginx/nginx.conf

To add -skips:

 	${DOCKER_COMPOSE_CMD} exec webserver /opt/gixy/bin/gixy -skips hsts_header,add_header_redefinition /etc/nginx/nginx.conf 

Another question, shouldn't it be better if this would move to lint/check? (although it's way easier to integrate here I guess).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't like disabling tests all out because then we might miss valid issues in the future. But then again, we don't have these checks at all with the current gixy.

I think I put it into integrationtest originally because it needs the container to render the config from the templates at runtime and lint/test is more for static tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I pushed the code just a few minutes before your comment. I agree, but is it possible on line level?
Does it still complain if add_header_inherit on; is set on http level (the highest level)?
Then maybe only HSTS can be skipped, until #324 is fixed.

    sed -ir '/add_header_inherit/d' docker/webserver/nginx_templates/default.conf.template
@bwbroersma

bwbroersma commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

@aequitas gixy also complains about nginx .. and this is the original/default nginx.conf:

Problem: [worker_rlimit_nofile_vs_connections] The worker_rlimit_nofile should be at least twice than worker_connections.
Severity: MEDIUM
Description: The worker_rlimit_nofile should be at least twice than worker_connections.
Additional info: https://gixy.getpagespeed.com/checks/worker-rlimit-nofile-vs-connections/
Reason: Missing worker_rlimit_nofile with at least twice the value of worker_connections
Pseudo config:

events  {
	worker_connections 1024;
}

Ignore another one?

@aequitas

Copy link
Copy Markdown
Collaborator Author

It kind of weird, because we do not set worker_connections, but upstream does. But if this is a common issue they should know better and have it set properly right? But then again, upstream doesn't set worker_rlimit_nofile so it defaults to the OS setting (20480) which is enough. So yes, i think we can ignore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants