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

(CAT-1281) - Support to add cipher with respective ssl protocol #2440

Merged
merged 3 commits into from
Aug 21, 2023
Merged

(CAT-1281) - Support to add cipher with respective ssl protocol #2440

merged 3 commits into from
Aug 21, 2023

Conversation

Ramesh7
Copy link
Contributor

@Ramesh7 Ramesh7 commented Aug 10, 2023 •
edited
Loading

Summary

Adding support for enable user to pass the different ciphers for different protocols.

Additional Context

Related Issues (if any)

  • N/A

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@Ramesh7 Ramesh7 force-pushed the CAT-1281-support-for-adding-cipher-for-protocols branch 5 times, most recently from 7d60a14 to 3d8e2e0 Compare August 10, 2023 09:02
ekohl
ekohl reviewed Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I did not know this was even an Apache option.

Should it also modify the cipher parameter in apache::vhost?

manifests/mod/ssl.pp Outdated
@@ -95,7 +95,7 @@
Optional[Stdlib::Absolutepath] $ssl_cert = undef,
Optional[Stdlib::Absolutepath] $ssl_key = undef,
Optional[Stdlib::Absolutepath] $ssl_ca = undef,
String $ssl_cipher = $apache::params::ssl_cipher,
Variant[String, Array, Hash] $ssl_cipher = $apache::params::ssl_cipher,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why Array? I don't see code in the template to handle that. And can we further tighten Hash to Hash[String[1], String[1]]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments, I wasn't aware that we can even define variable that level in puppet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Puppet data types are surprisingly powerful and I like them a lot.

@Ramesh7 Ramesh7 force-pushed the CAT-1281-support-for-adding-cipher-for-protocols branch 2 times, most recently from 1a84d7b to 5855384 Compare August 10, 2023 11:11
@Ramesh7
Copy link
Contributor Author

Ramesh7 commented Aug 10, 2023

I did not know this was even an Apache option.

Should it also modify the cipher parameter in apache::vhost?

I have already covered up for vhost here https://github.com/puppetlabs/puppetlabs-apache/pull/2440/files#diff-2842c1252a08a185c551b13fdab39118416a7d296217322d5ebb921c24899397R18-R21
Thanks for reminding this as I missed to update the variable data type for vhost. Updated now. Thanks

@Ramesh7 Ramesh7 force-pushed the CAT-1281-support-for-adding-cipher-for-protocols branch from 5855384 to 9d00180 Compare August 10, 2023 11:15
@Ramesh7 Ramesh7 marked this pull request as ready for review August 10, 2023 13:00
@Ramesh7 Ramesh7 force-pushed the CAT-1281-support-for-adding-cipher-for-protocols branch from 9d00180 to 8acbad4 Compare August 10, 2023 13:00
@Ramesh7 Ramesh7 requested review from a team, bastelfreak and smortex as code owners August 10, 2023 13:00
smortex
smortex reviewed Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Nice! I was not aware of this. I added in-line comments for some improvements I see.

manifests/vhost.pp Outdated Show resolved Hide resolved
manifests/mod/ssl.pp Outdated Show resolved Hide resolved
smortex
smortex reviewed Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

As you said :-) With this fixed, LGTM!

manifests/vhost.pp Outdated Show resolved Hide resolved
@Ramesh7 Ramesh7 merged commit c97699b into main Aug 21, 2023
37 checks passed
@Ramesh7 Ramesh7 deleted the CAT-1281-support-for-adding-cipher-for-protocols branch August 21, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@smortex smortex smortex approved these changes

@bastelfreak bastelfreak Awaiting requested review from bastelfreak bastelfreak is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@Ramesh7 @smortex @ekohl