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

Allow setting unquoted or custom flags on add_headers #1249

Merged
merged 3 commits into from
Oct 20, 2018
Merged

Allow setting unquoted or custom flags on add_headers #1249

merged 3 commits into from
Oct 20, 2018

Conversation

itbm
Copy link
Contributor

@itbm itbm commented Sep 28, 2018

Allows any combination of add_headers quoting.
This solves issues highlighted in #991 #992 #1020

Double Quotes:

add_header => {
    'Access-Control-Allow-Origin'      => '*' , 
}

would produce
add_header "Access-Control-Allow-Origin" "*";

Double Quotes and Unquoted

add_header => {
    'Access-Control-Allow-Origin'      => {'*' => 'always'}, 
}

would produce
add_header "Access-Control-Allow-Origin" "*" always;

Custom Quotes

add_header => {
    'Access-Control-Allow-Origin'      => {'' => '\'*\' always'}, 
}

would produce
add_header "Access-Control-Allow-Origin" '*' always;

@LongLiveCHIEF
Copy link

@itbm can you please update test specs to show these conditions? Otherwise, LGTM. Thanks!

@alexjfisher
Copy link
Member

nginx::location also has an add_header parameter. Does its template also need updating? https://github.com/voxpupuli/puppet-nginx/blob/master/templates/server/locations/headers.erb

@itbm
Copy link
Contributor Author

itbm commented Oct 1, 2018

I have added it to locations and updated the tests.

@@ -1,3 +1,11 @@
<%- @add_header.keys.sort.each do |key| -%>
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if we should also rewrite this to @add_header.sort.each do |header, value|. I think that's the same but leads to cleaner code

@@ -117,7 +117,15 @@ server {
<% end -%>
<% if @add_header -%>
<%- @add_header.keys.sort.each do |key| -%>
Copy link
Member

Choose a reason for hiding this comment

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

Could we include the templates/server/locations/headers.erb template here to avoid duplication?

@@ -146,6 +146,14 @@ server {
<% end -%>
<% if @add_header -%>
<%- @add_header.keys.sort.each do |key| -%>
Copy link
Member

Choose a reason for hiding this comment

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

And here

@@ -146,6 +146,14 @@ server {
<% end -%>
<% if @add_header -%>
Copy link
Member

Choose a reason for hiding this comment

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

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Oct 6, 2018
@itbm
Copy link
Contributor Author

itbm commented Oct 8, 2018

I have changed the sort.each and included the locations/headers.erb file in the others.

Copy link
Contributor Author

@itbm itbm left a comment

Choose a reason for hiding this comment

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

.

@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Oct 20, 2018
@bastelfreak bastelfreak merged commit 54a5056 into voxpupuli:master Oct 20, 2018
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Allow setting unquoted or custom flags on add_headers
@avillarreal83
Copy link

avillarreal83 commented Nov 6, 2019

I wanted to share that when attempting use the add_header in a hiera role, using the following puppet Hash would add the header to Nginx but the header would not be seen by Nginx due to not specifying the always setting in quotes:

Initial Puppet settings

add_header:
Strict-Transport-Security: 'max-age=31536000; includeSubDomains; always'

Initial Nginx Results

add_header "Strict-Transport-Security" "max-age=31536000; includeSubDomains;" always";

When Nginx is restarted, the header is not recognized. In order to get Nginx configuration to recognize the "always" setting I had to add some extra double quotes to the puppet Hash I had in place:

Updated Puppet settings
add_header:
Strict-Transport-Security: 'max-age=31536000; includeSubDomains;" "always'

Updated Nginx Results

add_header "Strict-Transport-Security" "max-age=31536000; includeSubDomains;" "always";

Wanted to share in case anyone else has runs into trying to use the add_header option from the server resource and experiences this issue. Using the add_header setting from this module was tricky, would be nice to make it easier to use the always setting in future releases.This is similar to what @benh57 experienced in #1020

@ajcollett
Copy link

Hi.
For all those Heira Users who see the above, here is a better way to do it, for the case above where you need to add a param after the string, and for long strings.

    add_header:
      Strict-Transport-Security: 
        'max-age=<your time>; includeSubDomains': always
      X-Frame-Options: deny
      X-Content-Type-Options: nosniff
      Content-Security-Policy: >-
        default-src 'self'; style-src 'self'; 
        style-src-elem; 

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Allow setting unquoted or custom flags on add_headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants