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

First steps to nginx config template #4397

Closed
wants to merge 30 commits into from
Closed

Conversation

acozine
Copy link
Contributor

@acozine acozine commented Oct 11, 2023

Partial fix for #4349.

This PR:

  • creates a template for nginx config files
  • adds four sites to the vars consumed by the template (plus more, commented out)
  • adds tasks to create and upload site configs for those four sites using the template, to the /tmp directory so we can test/diff
  • removes tasks for Stream config, which we are not using
  • uses | instead of : in task names in the upload-config.yml file so we don't have to quote them
  • uses FQCN for tasks in the upload-config.yml file

Copy link
Contributor

@bess bess left a comment

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

@maxkadel maxkadel left a comment

Choose a reason for hiding this comment

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

Looks good! Left a couple questions, but I really like where this is going!

ansible.builtin.template:
src: http/library.conf.j2
dest: /tmp/{{ item.name }}.conf
# dest: /etc/nginx/conf.d/{{ item.name }}.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put in a comment explaining that this is where they will eventually go, once testing is complete, or is that self-explanatory?

proxy_intercept_errors on;
# TODO don't do a health check if there's only one server
{% if item.health_check_URI is defined %}
health_check interval=10 fails=3 passes=2 uri={{ item.health_check_URI }};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could health_check_URI have a default of / (which I'm guessing is the nginx default) and thus avoid a conditional? I think it's fine either way, just trying to think of ways to simplify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm kinda cheating here. We don't want to do health checks on all sites, because for sites with only one server a health check is all overhead and no gain. This way we can use "health_check_URI" tell which sites should have one at all. But yeah, your solution is a good one longer-term.

# vars file for roles/nginxplus
# the 'sites' dictionary is used by the library.conf.j2 template
sites:
# - name: example-prod
Copy link
Contributor

Choose a reason for hiding this comment

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

Really appreciate this in-file documentation

location: "/"
visibility: private
app_protect: enabled
- name: allsearch-api-prod
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not have been in when you started this, but I think you could do allsearch and allsearch-staging as well (in addition to the api-only boxes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! I'll add them in.

@acozine
Copy link
Contributor Author

acozine commented Oct 17, 2023

Try tigerdata staging and prod, orcid staging and prod.

@acozine
Copy link
Contributor Author

acozine commented Oct 19, 2023

For bibdata, we need to have three additional proxy settings:

  • proxy_connect_timeout
  • proxy_send_timeout
  • proxy_read_timeout

All are set to 2h - is this a reasonable default for all our sites?

@acozine
Copy link
Contributor Author

acozine commented Nov 3, 2023

Known remaining work to a first-round merge:

  • add a directory for templated config
  • include the templated config directory in the main nginx config file, so both static and templated config files are loaded by the load balancer
  • update the task to send the templated files to that new directory
  • remove the template_test tag
  • find a way to remove unneeded/old files from the /etc/nginx/conf.d/dynamic directory
  • make sure the SSL directories on the load balancer match the directory names in the /etc/nginx/conf.d/dynamic/*.conf files. Use roles/nginxplus/vars/main.yml to create the SSL certificates in the appropriate directory structure.
  • Update /etc/nginx/nginx.conf to include the dynamic directory configs, either manually or by running playbooks/nginxplus_production_rebuild.yml.
  • test playbook again

@acozine
Copy link
Contributor Author

acozine commented Dec 9, 2023

@acozine
Copy link
Contributor Author

acozine commented Dec 22, 2023

@sandbergja and I tested the tasks for identifying and removing obsolete dynamic config files today. I added tasks for doing the same for obsolete static config files. I think after the break we can start moving the easy configs over. Procedure:

  • confirm that the SSL certs for the first sites to migrate have the correct dir names
  • update the main config to include the new dynamic config directory
  • remove the static config files for the first sites to migrate
  • run the playbook and test

If that works as expected, then we can merge this PR and migrate one or two configs per PR for the next few months.

acozine and others added 20 commits December 22, 2023 15:16
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Christina Chortaria <christinach@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Alicia Cozine <acozine@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Alicia Cozine <acozine@users.noreply.github.com>
Co-authored-by: Alicia Cozine <acozine@users.noreply.github.com>
Co-authored-by: Alicia Cozine <acozine@users.noreply.github.com>
Co-authored-by: Christina Chortaria <christinach@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Ryan Laddusaw <rladdusaw@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Christina Chortaria <christinach@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Ryan Laddusaw <rladdusaw@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Christina Chortaria <christinach@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Co-authored-by: Ryan Laddusaw <rladdusaw@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
acozine and others added 9 commits December 22, 2023 15:16
Co-authored-by: Francis Kayiwa <kayiwa@users.noreply.github.com>
Co-authored-by: Vickie Karasic <vickiekarasic@users.noreply.github.com>
Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
…lete the entire directory when removing a single site
@acozine
Copy link
Contributor Author

acozine commented Dec 22, 2023

Rebased ahead of the holidays.

@acozine
Copy link
Contributor Author

acozine commented Jan 30, 2024

Next (final?) steps for current PR:

  • backup config directories for easy rollback
  • run nginx -t after main config tasks are done
  • if nginx -t fails, restore prior config from the backed up directories

Future work before we move our most complicated sites to the config template:

  • engineer this so we can, from a branch, test our configs without going live - so for a test run we could just put config in non-active directories for comparison and debugging
    Workflow for migrating a site from static to templatized config would then be:
  1. open a branch, add vars to make the templatized config
  2. run playbook from branch with the flag or other solution to output all configs to a non-working directory
  3. compare static and templatized configs and do any needed debugging
  4. delete static config on branch once things look good
  5. run the playbook from branch "for realz" to affect actual working config

@acozine
Copy link
Contributor Author

acozine commented Feb 6, 2024

More progress today. When the nginx -t verification fails, instead of doing an automated recovery from the backed-up config directory, we decided it's better to post a message with the failure message - something like:
Nginx config update failed. Time to get a human involved, please restore from <dir>.

@acozine
Copy link
Contributor Author

acozine commented Mar 6, 2024

Closing in favor of #4654

@acozine acozine closed this Mar 6, 2024
@acozine acozine deleted the nginx_template_intro branch May 24, 2024 16:19
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.

4 participants