Skip to content

add server/location_acl argument#1491

Open
level-a wants to merge 1 commit intovoxpupuli:masterfrom
ahrefs:acl
Open

add server/location_acl argument#1491
level-a wants to merge 1 commit intovoxpupuli:masterfrom
ahrefs:acl

Conversation

@level-a
Copy link
Copy Markdown
Contributor

@level-a level-a commented Dec 4, 2021 •
edited
Loading

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)

@level-a
add server/location_acl argument
53ab5b7 
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
Copy Markdown

puppet-community-rangefinder Bot commented Dec 4, 2021

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
Copy Markdown
Contributor Author

level-a commented Feb 1, 2022

ping

1 similar comment
@level-a
Copy link
Copy Markdown
Contributor Author

level-a commented Apr 1, 2022

ping

root-expert
root-expert requested changes Apr 1, 2022
View reviewed changes
Copy link
Copy Markdown
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

Comment thread manifests/resource/location.pp
Optional[Enum['any', 'all']] $location_satisfy = undef,
Optional[Array] $location_allow = undef,
Optional[Array] $location_deny = undef,
Optional[String] $location_acl = undef,
Copy link
Copy Markdown
Member

@root-expert root-expert Apr 1, 2022

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 ?

Comment thread manifests/resource/server.pp
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
Copy Markdown
Member

@root-expert root-expert Apr 1, 2022

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

Reviewers

@root-expert root-expert root-expert requested changes

Assignees

No one assigned

Labels

enhancement New feature or request needs-tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@level-a @root-expert