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.

Sign up for GitHub

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

Jump to bottom

(MODULES-11068) Allow apache::vhost ssl_honorcipherorder to take boolean parameter #2152

Merged
merged 4 commits into from
May 18, 2021
Merged

(MODULES-11068) Allow apache::vhost ssl_honorcipherorder to take boolean parameter #2152

merged 4 commits into from
May 18, 2021

Conversation

davidc
Copy link
Contributor

@davidc davidc commented May 17, 2021

  • Allow ssl_honorcipherorder on apache::vhost to take booleans as every other on/off parameter does.
  • Default to not outputting SSLHonorCipherOrder at all, as before.
  • Also accept on/off to maintain backwards compatibility. (Note, the code would previously accept any value and output it verbatim. But since Apache will only accept on/off, I don't think it's necessary to maintain that level of backwards-compatibility).
  • Add tests for the new functionality as well as backwards compatibility - this need scrutiny as I have not written rspec tests before.

(fixes MODULES-11068)

@davidc davidc requested a review from a team as a code owner May 17, 2021 17:16
@CLAassistant
Copy link

CLAassistant commented May 17, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

apache::vhost is a type

Breaking changes to this file WILL impact these 128 modules (exact match):
Breaking changes to this file MAY impact these 34 modules (near match):

This module is declared in 175 of 576 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.

@davidc
Copy link
Contributor Author

davidc commented May 17, 2021

I am confused why these tests are failing when they run OK on my machine:

$ pdk test unit --tests=defines/vhost_spec.rb -v
[...]
      ssl_honorcipherorder
        ssl_honorcipherorder default
          is expected to compile into a catalogue without dependency cycles
          is expected to contain Concat::Fragment[rspec.example.com-ssl] with content !~ /^\s*SSLHonorCipherOrder/i
        ssl_honorcipherorder on
          is expected to compile into a catalogue without dependency cycles
          is expected to contain Concat::Fragment[rspec.example.com-ssl] with content =~ /^\s*SSLHonorCipherOrder\s+On$/
        ssl_honorcipherorder true
          is expected to compile into a catalogue without dependency cycles
          is expected to contain Concat::Fragment[rspec.example.com-ssl] with content =~ /^\s*SSLHonorCipherOrder\s+On$/
        ssl_honorcipherorder off
          is expected to compile into a catalogue without dependency cycles
          is expected to contain Concat::Fragment[rspec.example.com-ssl] with content =~ /^\s*SSLHonorCipherOrder\s+Off$/
        ssl_honorcipherorder false
          is expected to compile into a catalogue without dependency cycles
          is expected to contain Concat::Fragment[rspec.example.com-ssl] with content =~ /^\s*SSLHonorCipherOrder\s+Off$/
[...]
Finished in 40 minutes 11 seconds (files took 43.3 seconds to load)
4527 examples, 0 failures

@davidc
Copy link
Contributor Author

davidc commented May 17, 2021

It seems there was a further test in spec/acceptance/apache_ssl_spec.rb that is setting apache::vhost ssl_honorcipherorder to "test", I have changed it to check boolean true => On instead.

I can't test this locally since it seems to want to actually apply a configuration to my machine, so I'll push the change and let CI run it again.

@DavidS
Copy link
Contributor

DavidS commented May 18, 2021

I'll take a look at it today.

@DavidS
Copy link
Contributor

DavidS commented May 18, 2021 •
edited
Loading

github actions is currently experiencing issues - see https://www.githubstatus.com/incidents/m16jzl31gnqt ; that might explain the delays and weird failures. I'll re-run the checks once the github incident has cleared up.

@DavidS
Copy link
Contributor

DavidS commented May 18, 2021

rekicked all the github actions

@DavidS
Copy link
Contributor

DavidS commented May 18, 2021

The failures all look like still impacts from the github incident rather than actual code issues. Since this is not a platform-dependent change, I'm gonna merge it and continue to monitor scheduled CI for tomorrow.

@DavidS DavidS merged commit e14ec09 into puppetlabs:main May 18, 2021
@DavidS
Copy link
Contributor

DavidS commented May 18, 2021

Thanks a lot @davidc for your contribution and apologies for the bumps along the way!

@pmcmaw pmcmaw added the feature label May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidS DavidS DavidS approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidc @CLAassistant @DavidS @pmcmaw