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

Nginx location as exported resource not working anymore #964

Open
ITler opened this Issue Nov 8, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@ITler

ITler commented Nov 8, 2016

Affected Puppet, Ruby, OS and module versions/distributions

  • Puppet: current pc1 with puppet 4.8.0
  • Ruby: from pc1(irrelevant for this issue)
  • Distribution: Ubuntu 14
  • Module version: 0.5.0 (forge)

How to reproduce (e.g. Puppet code you use)

I need to define a location on an application server as an exported resource, that will get realized on another host with nginx installed. So on my appserver I only use:

create_resources('@@nginx::resource::location', $locations, $location_params)

What are you seeing

In an older version of jfryman-nginx (some version not far before the last 9999999) this worked fine. With the last version of jfryman and the current 0.5.0 from voxpupli this doesn't work anymore.

What behaviour did you expect instead

I would have expected to have things working, like they did before.

Output log

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Unknown variable: '::nginx::config::proxy_redirect'. at /etc/puppetlabs/code/environments/dev/modules/nginx/manifests/resource/location.pp:165:27 at [...]

Any additional information you'd like to impart

Of cause, the appserver doesn't have an nginx installation, so I can't call include ::nginx

However when explicitly defining proxy_redirect in $location_params this seem to work. Then issue cascading to next parameter.. and so on... This means, that every parameter definition of nginx/manifests/resource/location.pp assigning a default value from $::nginx will cause this issue. I think the obvious approach (usual pattern) should be to assign default values from $::nginx::parms, which could be included beforehand.

Kind regards
ITler

@wyardley

This comment has been minimized.

Show comment
Hide comment
@wyardley

wyardley Nov 8, 2016

Collaborator

I kind of get what you're doing here, but I'm not totally sure what the expected behavior is, or what changes exactly caused this.

Is it possible to test with current git master branch and see if the behavior works as you expect? The next release will get rid of the nginx::config namespace that was introduced a while back (i.e., all parameters will just be defined at top scope again). However, I'm not sure if that will change this behavior. fwiw, proxy_redirect defaults to undef, not sure if that relates to your problem, though I think you could set an explicit default when you override the class (when you instantiate the class, or in hiera).

Usually, as I understand it, it's parameters that vary depending on, say, OS or other factors, are in params, other defaults are just assigned directly in the class params (if they're overridable). Does that kind of make sense?

Collaborator

wyardley commented Nov 8, 2016

I kind of get what you're doing here, but I'm not totally sure what the expected behavior is, or what changes exactly caused this.

Is it possible to test with current git master branch and see if the behavior works as you expect? The next release will get rid of the nginx::config namespace that was introduced a while back (i.e., all parameters will just be defined at top scope again). However, I'm not sure if that will change this behavior. fwiw, proxy_redirect defaults to undef, not sure if that relates to your problem, though I think you could set an explicit default when you override the class (when you instantiate the class, or in hiera).

Usually, as I understand it, it's parameters that vary depending on, say, OS or other factors, are in params, other defaults are just assigned directly in the class params (if they're overridable). Does that kind of make sense?

@ITler

This comment has been minimized.

Show comment
Hide comment
@ITler

ITler Nov 14, 2016

Hy ...
I'll test with git master as soon as possible.

If I understand correctly, it seems, that it is not fully clear, what I'm doing and why. I'll try to explain in other words:
The expected behaviour is, to not get syntax errors from nginx module, when only defining location defined type on a node, that may not include nginx or nginx::config classes. That's because nginx is not to be installed on that machine. However location needs to be defined on appserver, because I can only gather several information on that specific node (appserver)
Using exported resources and puppetdb then allow me to transfer the location resource to real nginx host to be implemented.
You said:

Usually, as I understand it, it's parameters that vary depending on, say, OS or other factors, are in params, other defaults are just assigned directly in the class params (if they're overridable). Does that kind of make sense?

I think this makes sense, however currently (where my issue comes from, i.e. for proxy_redirect), this pattern is broken, as location uses default values defined by nginx::config (which also does implementation).
I agree, as far as I know the common design pattern is to serve default values, only, from a nginx::params
class, which could get included elsewhere, but does no real configuration to a node.

Conclusion

So there are 2 possible approaches. Define useful parameter defaults in classes or defined types directly or encapsulate those in a ::params class, which only hold variable OS dependent and independent values, but does no configuration like file, exec, etc.

Greets

ITler commented Nov 14, 2016

Hy ...
I'll test with git master as soon as possible.

If I understand correctly, it seems, that it is not fully clear, what I'm doing and why. I'll try to explain in other words:
The expected behaviour is, to not get syntax errors from nginx module, when only defining location defined type on a node, that may not include nginx or nginx::config classes. That's because nginx is not to be installed on that machine. However location needs to be defined on appserver, because I can only gather several information on that specific node (appserver)
Using exported resources and puppetdb then allow me to transfer the location resource to real nginx host to be implemented.
You said:

Usually, as I understand it, it's parameters that vary depending on, say, OS or other factors, are in params, other defaults are just assigned directly in the class params (if they're overridable). Does that kind of make sense?

I think this makes sense, however currently (where my issue comes from, i.e. for proxy_redirect), this pattern is broken, as location uses default values defined by nginx::config (which also does implementation).
I agree, as far as I know the common design pattern is to serve default values, only, from a nginx::params
class, which could get included elsewhere, but does no real configuration to a node.

Conclusion

So there are 2 possible approaches. Define useful parameter defaults in classes or defined types directly or encapsulate those in a ::params class, which only hold variable OS dependent and independent values, but does no configuration like file, exec, etc.

Greets

@ITler

This comment has been minimized.

Show comment
Hide comment
@ITler

ITler Nov 15, 2016

Additional Info:
It looks like this problem occurs only on younger puppet systems, with strict_variables = true. Older versions seem to not respect this setting. However, as strict_variables is a recommendation by puppet, this module's code should work.
This issue escalates to a show stopper for a planned migration project next week. Is it possible for you to refactor sources?

ITler commented Nov 15, 2016

Additional Info:
It looks like this problem occurs only on younger puppet systems, with strict_variables = true. Older versions seem to not respect this setting. However, as strict_variables is a recommendation by puppet, this module's code should work.
This issue escalates to a show stopper for a planned migration project next week. Is it possible for you to refactor sources?

@xaque208

This comment has been minimized.

Show comment
Hide comment
@xaque208

xaque208 Nov 15, 2016

Contributor

The error in the OP references ::nginx::config::proxy_redirect. Has this been updated to be ::nginx::proxy_redirect? If so, could we get the latest error please?

Contributor

xaque208 commented Nov 15, 2016

The error in the OP references ::nginx::config::proxy_redirect. Has this been updated to be ::nginx::proxy_redirect? If so, could we get the latest error please?

@wyardley

This comment has been minimized.

Show comment
Hide comment
@wyardley

wyardley Nov 15, 2016

Collaborator

@ITler: Current version (git master, and soon to be released version) does not have an interface tonginx::config at all, it's a private class, and all parameters are now defined at top scope. So I think most recent changes should resolve these issues for you; if not, agree with @xaque208, show how you're invoking the module and include updated error messages. Any references to ::nginx::config::foo should become ::nginx::foo

Collaborator

wyardley commented Nov 15, 2016

@ITler: Current version (git master, and soon to be released version) does not have an interface tonginx::config at all, it's a private class, and all parameters are now defined at top scope. So I think most recent changes should resolve these issues for you; if not, agree with @xaque208, show how you're invoking the module and include updated error messages. Any references to ::nginx::config::foo should become ::nginx::foo

@ITler

This comment has been minimized.

Show comment
Hide comment
@ITler

ITler Nov 16, 2016

Yes, I understood, that nginx::config is obsolete/removed and you moved variables to top scope: ::nginx.
My problem comes from a different scenario/use case/starting point. We're talking about different things.

In my use case "define location as exported resource on application server node", puppet is not able to access/resolve ::nginx::foo or ::nginx::proxy_redirect or any variable from ::nginx during catalog build, because I intentionally do not include ::nginx. (I can't include it, as this would install nginx to the node (application server), but this may not happen in my configuration.) As ::nginx::<any variable> was never defined, puppet 4.8. and strict_variables = true throws Unknown variable exception.

Maybe another example:
Current master (50cced9) location.pp line 174 has:

$fastcgi_params = "${::nginx::conf_dir}/fastcgi_params",

without include ::nginx the variable conf_dir can never be resolved. So path for fastcgi_params will not be correct. With configuration strict_variables = true puppet throws Unknown variable exception, with strict_variables = false it just evaluates to "empty", which is just wrong (and hard to debug/trace, so maybe this is the reason, why puppetlabs introduced strict_variables setting in their configuration)

Somehow (I can't explain why - maybe bug in puppet itself) I've got no problem on older puppet server version, but I'm forced to upgrade. So I run into that issues here. Also strict_variables = true will become built-in with puppet 5 so for future compatibility this needs to be addressed.

ITler commented Nov 16, 2016

Yes, I understood, that nginx::config is obsolete/removed and you moved variables to top scope: ::nginx.
My problem comes from a different scenario/use case/starting point. We're talking about different things.

In my use case "define location as exported resource on application server node", puppet is not able to access/resolve ::nginx::foo or ::nginx::proxy_redirect or any variable from ::nginx during catalog build, because I intentionally do not include ::nginx. (I can't include it, as this would install nginx to the node (application server), but this may not happen in my configuration.) As ::nginx::<any variable> was never defined, puppet 4.8. and strict_variables = true throws Unknown variable exception.

Maybe another example:
Current master (50cced9) location.pp line 174 has:

$fastcgi_params = "${::nginx::conf_dir}/fastcgi_params",

without include ::nginx the variable conf_dir can never be resolved. So path for fastcgi_params will not be correct. With configuration strict_variables = true puppet throws Unknown variable exception, with strict_variables = false it just evaluates to "empty", which is just wrong (and hard to debug/trace, so maybe this is the reason, why puppetlabs introduced strict_variables setting in their configuration)

Somehow (I can't explain why - maybe bug in puppet itself) I've got no problem on older puppet server version, but I'm forced to upgrade. So I run into that issues here. Also strict_variables = true will become built-in with puppet 5 so for future compatibility this needs to be addressed.

@ITler

This comment has been minimized.

Show comment
Hide comment
@ITler

ITler Nov 16, 2016

Latest error in current master (50cced9) location.pp line 179 :

$uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",

ITler commented Nov 16, 2016

Latest error in current master (50cced9) location.pp line 179 :

$uwsgi_params = "${nginx::config::conf_dir}/uwsgi_params",

@ITler

This comment has been minimized.

Show comment
Hide comment
@ITler

ITler Nov 16, 2016

PR #974 solves my use case with bug fixed from previous comment and has minimal impact on code. Please merge asap.

ITler commented Nov 16, 2016

PR #974 solves my use case with bug fixed from previous comment and has minimal impact on code. Please merge asap.

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