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

add server/location_acl argument #1491

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

Conversation

level-a
Copy link
Contributor

@level-a level-a commented Dec 4, 2021

Pull Request (PR) description

  • add ability to include conf.d/name.acl ACLs to server/location blocks to not repeat allow/deny rules multiple times
  • allows to combine common allow list (in .acl file) with allow/deny rules in specific block (deny rule will be the last)

use include directive to include server/location ACL file
from nginx_conf_dir/conf.d/ dir to not repeat allow/deny lists
for every location server/location block
@puppet-community-rangefinder
Copy link

nginx::resource::location is a type

that may have no external impact to Forge modules.

nginx::resource::server is a type

that may have no external impact to Forge modules.

This module is declared in 9 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@level-a
Copy link
Contributor Author

level-a commented Feb 1, 2022

ping

1 similar comment
@level-a
Copy link
Contributor Author

level-a commented Apr 1, 2022

ping

Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Please add some units tests for this change

@@ -269,6 +271,7 @@
Optional[Enum['any', 'all']] $location_satisfy = undef,
Optional[Array] $location_allow = undef,
Optional[Array] $location_deny = undef,
Optional[String] $location_acl = undef,
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the name can't be an empty string

Suggested change
Optional[String] $location_acl = undef,
Optional[String[1]] $location_acl = undef,

Also maybe the user must be able to overwrite the default path nginx::conf_dir/conf.d ?

@@ -290,8 +294,10 @@
Variant[Array[Stdlib::Absolutepath], Stdlib::Absolutepath] $listen_unix_socket = '/var/run/nginx.sock',
Optional[String] $listen_unix_socket_options = undef,
Optional[Enum['any', 'all']] $location_satisfy = undef,
Optional[String] $server_acl = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Same here:

Suggested change
Optional[String] $server_acl = undef,
Optional[String[1]] $server_acl = undef,

@root-expert root-expert added enhancement New feature or request needs-tests labels Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants