Skip to content
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

fix: remove trailing whitespace from awk command #913

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

lockejan
Copy link

@lockejan lockejan commented Aug 1, 2024

Proposed changes

Currently the awk command in entrypoint/15-local-resolvers.envsh will always produce a trailing whitespace.
This will break configs that make use of quotation marks around variable in config templates.
Initially opened the issue here: nginxinc/docker-nginx-unprivileged#234

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the CONTRIBUTING document
  • I have run ./update.sh and ensured all entrypoint/Dockerfile template changes have been applied to the relevant image entrypoint scripts & Dockerfiles
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation

@thresheek
Copy link
Collaborator

I don't think it's a proper fix since we can have multiple resolvers defined, and then if ORS is set to "" they will be squashed together, e.g. "127.0.0.1127.0.0.2".

@thresheek
Copy link
Collaborator

We probably just want to check if the latest character of the variable is whitespace and trim it

@lockejan
Copy link
Author

lockejan commented Aug 1, 2024

We probably just want to check if the latest character of the variable is whitespace and trim it

Good point. Probably something like this then?
NGINX_LOCAL_RESOLVERS=$(awk 'BEGIN{ORS=" "} $1=="nameserver" {if ($2 ~ ":") {print "["$2"]"} else {print $2}}' /etc/resolv.conf | awk '{$1=$1;print}')

@thresheek
Copy link
Collaborator

I'd prefer a posix shell-compatible solution rather than another (costly) awk call - e.g. get the last character of a string, and then compare/trim

@lockejan
Copy link
Author

lockejan commented Aug 1, 2024

I'd prefer a posix shell-compatible solution rather than another (costly) awk call - e.g. get the last character of a string,
and then compare/trim

Totally see your point.
I'm not an shell expert, but I found these two ways of achieving this. Personally I like A better.
Does that suit your expectations?

A. xargs

NGINX_LOCAL_RESOLVERS=$(awk 'BEGIN{ORS=" "} $1=="nameserver" {if ($2 ~ ":") {print "["$2"]"} else {print $2}}' /etc/resolv.conf | xargs)

B. shell expansion

NGINX_LOCAL_RESOLVERS=$(awk 'BEGIN{ORS=" "} $1=="nameserver" {if ($2 ~ ":") {print "["$2"]"} else {print $2}}' /etc/resolv.conf)
NGINX_LOCAL_RESOLVERS="${NGINX_LOCAL_RESOLVERS%"${NGINX_LOCAL_RESOLVERS##*[![:space:]]}"}"

@lockejan lockejan force-pushed the fix/entrypoint-script branch from 76fdcf5 to aa664ca Compare August 1, 2024 22:06
@lockejan lockejan closed this Aug 14, 2024
@thresheek thresheek reopened this Aug 16, 2024
@thresheek
Copy link
Collaborator

I think I've missed the last comment - sorry about that.

Wouldnt the following simple patch work, as we're explicitely setting ORS to a space?

diff --git a/entrypoint/15-local-resolvers.envsh b/entrypoint/15-local-resolvers.envsh
index 450a999..e830dda 100755
--- a/entrypoint/15-local-resolvers.envsh
+++ b/entrypoint/15-local-resolvers.envsh
@@ -9,4 +9,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
 [ "${NGINX_ENTRYPOINT_LOCAL_RESOLVERS:-}" ] || return 0

 NGINX_LOCAL_RESOLVERS=$(awk 'BEGIN{ORS=" "} $1=="nameserver" {if ($2 ~ ":") {print "["$2"]"} else {print $2}}' /etc/resolv.conf)
+
+NGINX_LOCAL_RESOLVERS="${NGINX_LOCAL_RESOLVERS% }"
+
 export NGINX_LOCAL_RESOLVERS

Trailing whitespaces break configs that use quotation marks around vars.
See nginxinc/docker-nginx-unprivileged#234
@thresheek
Copy link
Collaborator

I also thought about adding a test to catch issues like that, but more I look into, more I think it's a misconfiguration to put quotation marks in there, e.g. resolver "127.0.0.1 127.0.0.2"; results in:

nginx: [emerg] host not found in resolver "127.0.0.1 127.0.0.2" in /etc/nginx/conf.d/test.conf:3

@thresheek thresheek force-pushed the fix/entrypoint-script branch from aa664ca to ec81a15 Compare August 16, 2024 19:59
@thresheek thresheek merged commit 8b08a26 into nginxinc:master Aug 16, 2024
9 checks passed
@lockejan
Copy link
Author

No worries. Thanks for finalising it. I never used multiple resolvers yet, so I wasn't really aware of that test case. But yeah it's rather a misconfiguration and the trailing whitespace was only problematic for this edge case. Will update my config. ✌️

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