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

chore(docs): add additional information for using RES_OPTIONS and LOCALDOMAIN #13323

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chobits
Copy link
Contributor

@chobits chobits commented Jul 2, 2024

Summary

By default, when Nginx master starts worker processes, it removes all environment variables. This prevents the use of $RES_OPTIONS and $LOCALDOMAIN. Therefore, this PR adds additional information to kong.conf.default to guide users on utilizing these variables effectively.

Checklist

  • N/A The Pull Request has tests
  • N/A A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • N/A There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #13301 and KAG-4845

@chobits
Copy link
Contributor Author

chobits commented Jul 2, 2024

A potential thorough fix would be to set these env variables in Kong's nginx configuration by default. However, for now, this documentation fix is simpler.

@chobits chobits added cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee skip-changelog labels Jul 2, 2024
Comment on lines +1497 to +1499
# they have been set. To use these environment variables, set the option
# `nginx_main_env = RES_OPTIONS; env LOCALDOMAIN`. Otherwise, Nginx will
# removes these environment variables when starting worker processes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# they have been set. To use these environment variables, set the option
# `nginx_main_env = RES_OPTIONS; env LOCALDOMAIN`. Otherwise, Nginx will
# removes these environment variables when starting worker processes.
# they have been set. To use these environment variables, set the options
# `nginx_main_env = RES_OPTIONS;` and `env LOCALDOMAIN` in your nginx
# configuration template to prevent nginx from removing them during worker
# process start.

It seems to me that this suggestion, while workable, is pretty difficult. Is there a good reason not to put these directives into the default nginx template?

Copy link
Contributor Author

@chobits chobits Jul 3, 2024

Choose a reason for hiding this comment

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

I originally thought this fix would be easier and more stable for 3.8 release, considering we have another DNS cname regression that needs fixing in next release (#13294). Therefore, I prefer not to involve additional code modification in the DNS client at this time.

Copy link
Contributor Author

@chobits chobits Jul 3, 2024

Choose a reason for hiding this comment

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

OKay I'll research how to introduce environment variables into the default template of Kong later.

Copy link
Member

Choose a reason for hiding this comment

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

isn't all there is to do, adding:

env RES_OPTIONS;
env LOCALDOMAIN;

to the template? It doesn't even have to be configurable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's it. Just like in your example, we just need to hard code them.

@chobits chobits marked this pull request as draft July 3, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee size/XS skip-changelog
3 participants