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

Configuration changes may not take place until ddev restart #4411

Closed
1 task done
deviantintegral opened this issue Nov 29, 2022 · 7 comments
Closed
1 task done

Configuration changes may not take place until ddev restart #4411

deviantintegral opened this issue Nov 29, 2022 · 7 comments

Comments

@deviantintegral
Copy link
Collaborator

deviantintegral commented Nov 29, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Run a Diagnostic and Paste Link Here

This passes - as it's including project and client names, if it's truly required let me know and I'll sanitize the logs.

Expected Behavior

Adding a new nginx configuration to ~/.ddev/nginx/storybook.conf is applied.

Actual Behavior

It doesn't appear that nginx snippets are applied at all - even the default one shown in the docs: https://ddev.readthedocs.io/en/stable/users/extend/customization-extendibility/#nginx-snippets

Steps To Reproduce

composer create-project drupal/recommended-project nginx-test && \
cd nginx-test && \
ddev config && \
mkdir -p .ddev/nginx && echo "add_header Access-Control-Allow-Origin '*';" > .ddev/nginx/storybook.conf && \
ddev start && \
curl -v https://nginx-test.ddev.site/core/misc/ajax.js 2>&1 | grep -i access-control-allow-origin

Note Using nginx snippets: [/tmp/nginx-test/.ddev/nginx/storybook.conf] does show in the start logs.

Anything else?

I can work around this by editing the full configuration, but for such a simple change I'd rather not. As well, I'm not sure if this directive needs to be in a location or not, but if so then I'd expect this to fail completely.

@rfay
Copy link
Member

rfay commented Nov 29, 2022

You can ddev ssh and explore what's going on in /etc/nginx. I had always planned to deprecate the snippets because they can be confusing. They can only work when their content fits inside server.

If you look at your nginx_full/nginx-site.conf, you'll see that the snippets get loaded last in the server section,

include /mnt/ddev_config/nginx/*.conf;

You can also use nginx -t to see what's going on, etc.

@rfay
Copy link
Member

rfay commented Nov 29, 2022

Overall, try something simpler first. For example, try the example in https://ddev.readthedocs.io/en/latest/users/extend/customization-extendibility/#nginx-snippets

If it works, (it will), then the problem is with your expectations in that level of the configuration.

@rfay
Copy link
Member

rfay commented Nov 29, 2022

The docs do seem to say that add_header can be used in the server block, Context: | http, server, location, if in location

However, there are several constraints, the only add_header processed is the last found.

But wait... using your example, it adds the header just fine:

admin___Umami_Food_Magazine

@deviantintegral
Copy link
Collaborator Author

Thanks Randy. I did some more testing with fresh eyes and found two issues:

  1. I didn't realize you couldn't specify multiple matching location directives. While the config test passes, it looks like nginx ignores following location directives once one is matched. That's why the server level header works, though as you said it's got it's own problems.
  2. It looks like ddev start isn't actually reloading the config. The docs say You can reload the nginx configuration either with ddev start or ddev exec nginx -s reload.. Manually running reload or running ddev restart does work.

I see two next steps.

The main reason I tried using a snippet was I didn't want to fork the whole config for a one line change. If you fixed the config for drupal 10 or 11, that wouldn't apply, and I don't know if it would be obvious to future team members that the config had been overridden. This is totally a feature request, but I wonder if something like adding a header like ## ddev-generated-version <tag> would allow ddev to notify when the config should be checked against upstream changes.

Second, does ddev start need to be fixed to reload the nginx config, or removed from the docs? I used it simply because it was first in the list. If it's just a docs change, I can certainly contribute that. If there's a bug in ddev start... possibly, it just depends how my week goes!

@rfay
Copy link
Member

rfay commented Nov 30, 2022

I think what happened is are using mutagen, and in some situations ddev start doesn't restart the web container (or the webserver). Essentially, ddev restart is what you want for config changes.

What we should really do is stop saying anything about ddev start and just always say ddev restart to absolutely make sure that the container starts. (ddev start lets docker-compose make the decision about whether to rebuild the container(s), and sometimes it doesn't. I'm never certain about its decisionmaking. With mutagen on, it seems to be less likely to recreate. You can see this in the output, whether recreate happens or not.)

@rfay rfay changed the title nginx custom snippets are not applying Configuration changes may not take place until ddev restart Nov 30, 2022
@rfay
Copy link
Member

rfay commented Nov 30, 2022

It does in fact say...

Starting d9...
Using nginx snippets: [/Users/rfay/workspace/d9/.ddev/nginx/storybook.conf]
Custom configuration takes effect when the container is created.
Should something go wrong and no effect be visible use 'ddev restart'.

@deviantintegral
Copy link
Collaborator Author

The web container is not being restarted or recreated. I assumed there was some extra code restarting nginx or similar. I'll file a PR for the docs.

On the:

Should something go wrong and no effect be visible use 'ddev restart'.

I did do that, but I assumed that message was more about making changes to docker compose files and the like. I think treating ddev start behaviour as undefined when containers are already started is a reasonable way to approach this.

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

No branches or pull requests

2 participants