ghoneycutt / puppet-module-ssh

Puppet module to manage SSH
Other
34 stars 184 forks source link

Add support for new OS flavours (ready to merge) #398

Closed Phil-Friderici closed 1 year ago

Phil-Friderici commented 1 year ago

This change add support for these new OSes:

To be able to support new OS flavours these changes have been made too:

Fixes:

Phil-Friderici commented 1 year ago

Comment from @anders-larsson:

Hi @Phil-Friderici,

Would you mind adding RedHat 9 as well? It is currently not supported.

Looking through your changes for EL9 distributions they seem to be missing configuration/a decision needs to be made how the .d directories should be handled. Right now they are included by the main configuration file but not maintained. Is this how we want it to work?

Until this is officially supported by the module should all configuration be moved into the main configuration file ignoring the contents of the .d directories?

Phil-Friderici commented 1 year ago

Would you mind adding RedHat 9 as well? It is currently not supported.

Will do so.

Looking through your changes for EL9 distributions they seem to be missing configuration/a decision needs to be made how the .d directories should be handled. Right now they are included by the main configuration file but not maintained. Is this how we want it to work?

I have started with adding just basic support for the new OS flavours. This should enable users to fully configure SSH in the old style (all configuration inside ssh_config/sshd_config). You are rights, to support the new style (using include and .d directories) the module would need maintaining these as well. If wanted I can start working on that later as well. Maybe @ghoneycutt or @treydock can drop there opinion here too ?

Until this is officially supported by the module should all configuration be moved into the main configuration file ignoring the contents of the .d directories?

For the default configuration I try to touch as less as possible and change nothing from the OS vendor indented settings. As long as the module doesn't manage .d directory configuration files itself you have to do so by yourself or put all your setings into the old configuration files and remove the include directives.

Phil-Friderici commented 1 year ago

Comment from @anders-larsson:

Sounds good to me. It can always be replaced locally by the user. I'll update the Ubuntu 22.04 to support support v4. Our stance than is to maintain the defaults (not setting stuff explicitly)? In previous versions stuff seem to be set explicitly (RedHat 8 for example) instead of using the implicit defaults.

Phil-Friderici commented 1 year ago

Our stance than is to maintain the defaults (not setting stuff explicitly)? In previous versions stuff seem to be set explicitly (RedHat 8 for example) instead of using the implicit defaults.

Well, staying as close as possible to the vendor settings is my approach for modules ;)

Phil-Friderici commented 1 year ago

@anders-larsson: support for RedHat 9 just added

Phil-Friderici commented 1 year ago

@ghoneycutt could you please review and hopefully merge. Btw: v4 is running in production already :)

canihavethisone commented 1 year ago

@Phil-Friderici Would it be possible to also add support for CentOS 9 please? Should be straight forward.

Phil-Friderici commented 1 year ago

@canihavethisone: et voilà, added support for CentOS 9.

Tests do fail due to problems with PDK gems. Guess it needs an update by Puppetlabs.

canihavethisone commented 1 year ago

@Phil-Friderici wow, thanks for the fast work! Perhaps centOS9 should also be added to supported os in metadata?

Phil-Friderici commented 1 year ago

@canihavethisone added CentOS 9 to metadata as @canihavethisone (thanks!) mentioned

Phil-Friderici commented 1 year ago

great Puppetlabs fixed the PDK and now all checks are good :)

ghoneycutt commented 1 year ago

@Phil-Friderici Why ssh::config_file_server and ssh::config_file_client as it seems they are the same. Perhaps we only need ssh::config ?

Phil-Friderici commented 1 year ago

@ghoneycutt We need one for the server directives and the other for the client directives.

Phil-Friderici commented 1 year ago

Acceptance tests seem to be broken again :(

Phil-Friderici commented 1 year ago

@ghoneycutt Anything missing before it could get merged ?

treydock commented 1 year ago

I ran into those same acceptance errors on some of my modules, switching to Voxpupli testing gems (ie newer testing gems) solved the issue: https://github.com/treydock/puppet-module-keycloak/pull/276/files#diff-d09ea66f8227784ff4393d88a19836f321c915ae10031d16c93d67e6283ab55f

Phil-Friderici commented 1 year ago

@treydock Thank you for pointing to the Voxpupli gems. Too bad I have never understood the way Voxpupli is testing. I was able to get the acceptance tests runing with your hint. But now the unit tests doesn't work anymore :(

treydock commented 1 year ago

https://github.com/treydock/puppet-module-keycloak/blob/master/.github/workflows/ci.yaml#L48 . I think you need to update ci.yaml and release.yaml in this repo to bump their Ruby gem cache version number to flush the cache and force Github Actions to download new Ruby gems. The caching of gems can be a problem for CI pipelines when Gemfile.lock is not part of the git repo.

Phil-Friderici commented 1 year ago

To speak frankly, I think that my PR is complete and working well. It is the testing harness that doesn't work as intended. I had managed to get it working for either the unit tests or the acceptance tests but not both. From my perspective the testing harness should be fixed in another PR and shouldn't block merging this PR.

baron1405 commented 1 year ago

@ghoneycutt @Phil-Friderici is there an ETA for getting this committed and a new version released?

treydock commented 1 year ago

@baron1405 I will be taking this and trying to get it across the finish line. I hope to have things ready to merge in next week or two.

baron1405 commented 1 year ago

@treydock Thanks for the update and your efforts are truly appreciated!

ghoneycutt commented 1 year ago

Thank you for your contributions and help! This is merged in https://github.com/ghoneycutt/puppet-module-ssh/pull/402 and released in 4.1.0.