ghoneycutt / puppet-module-pam

Puppet module to manage PAM
Other
18 stars 80 forks source link

(GH-211) Add support for RHEL 8 #213

Closed ph84172 closed 4 years ago

ph84172 commented 5 years ago

I've tested this on a fresh install of RHEL 8 and it appears to be working correctly.

Garrett, I'm not sure how you want to handle the vagrant image for this test as there's no CentOS 8 yet. There does appear to be a vagrant box for RHEL 8 but I haven't tested it.

ghoneycutt commented 5 years ago

Hi @ph84172 could you also please add a vagrant box to the Vagrantfile so we can test it works that way. We also need to add a docker image to https://github.com/ghoneycutt/puppet-module-pam/tree/master/spec/acceptance/nodesets and https://github.com/ghoneycutt/puppet-module-pam/blob/master/.travis.yml which I forgot to do for SLES15.

ph84172 commented 5 years ago

@ghoneycutt is there a way to specify a different container registry in the nodesets file? Redhat publish their "Universal Base Image" which is based on RHEL8 but it's hosted on registry.access.redhat.com rather than Docker Hub.

ph84172 commented 5 years ago

Alternatively we wait until CentOS 8 has been released and there's a centos8 image on Docker Hub :)

ghoneycutt commented 5 years ago

Try specifying the registry through the environment variable DOCKER_REGISTRY such as

DOCKER_REGISTRY="registry.access.redhat.com" \
BEAKER_set="el-8" \
BEAKER_PUPPET_COLLECTION=puppet6 \
bundle exec rake beaker
ghoneycutt commented 5 years ago

If that does not work, suggest using one of the unofficial images from docker hub.

ghoneycutt commented 5 years ago

Created https://tickets.puppetlabs.com/browse/BKR-1593

ghoneycutt commented 5 years ago

Hi @ph84172 Seems we just have these two left before we can merge. For the docker image, suggest using an unofficial one and putting a comment in there with a link to this issue that explains why it is being used and that we are waiting on the BKR ticket to use the official one.

ph84172 commented 5 years ago

@ghoneycutt I've tried adding unofficial RHEL 8 Vagrant boxes / Docker images - does this run through Travis correctly?

ghoneycutt commented 5 years ago

Vagrant is done manually and the travis config update looks great. Before a release, I'll run https://github.com/ghoneycutt/puppet-module-pam/blob/master/tests/vagrant_test_all.sh and ensure that you can apply this code on a vanilla platform and still login through ssh.

ph84172 commented 5 years ago

It's all yours, thanks Garrett.

Might be worth thinking about switching the box/container images over to CentOS 8 when it's released to keep it consistent with the previous use of CentOS 7 as the canonical "EL7" target.

ghoneycutt commented 5 years ago

Docker and Vagrant failing because you need credentials to use yum. Can we work around this?

"This system is not registered to Red Hat Subscription Management. You can use subscription-manager to register."
ph84172 commented 5 years ago

I've removed the "yum -y install wget" in the nodeset file as wget is already installed in the base RHEL 8 environment.

I also reworked provision_basic_el.sh into a new provision_basic_rhel.sh for the RHEL 8 target which again, removes the wget reference and removes references to EPEL (doesn't exist yet for RHEL 8) and CentOS repositories.

ph84172 commented 5 years ago

I've also renamed the Vagrant machine from "el8-pam" to "rhel8-pam" thinking that it's probably better to start distinguishing explicitly between CentOS (el) and RHEL (rhel).

ph84172 commented 5 years ago

Ping @ghoneycutt - how's this PR looking now? Thanks.

ghoneycutt commented 5 years ago

@ph84172 Could you please rebase and I'll get this merged. Great work on this PR!

ph84172 commented 5 years ago

@ghoneycutt looks like Beaker tries to do a "yum install" of some openssh components once the container is up, which will fail without a valid RH subscription. Given that we can't be too far away from a CentOS 8 release I'm wondering if we should nix this until then?

We could either add support for EL 8 without functioning tests for now and put it on the "unsupported" list or just wait for CentOS 8 and try this PR again then. What do you think?

ghoneycutt commented 5 years ago

Seems like a reasonable approach to put them in unsupported, pending centos 8 release. Besides the README, could you update travis with the an allowed_failures section and put a comment that these should be changed to centos 8 and link back to this pull request. That should hopefully make it apparent to everyone what's going on and when to expect support.

here's an example

https://github.com/sensu/sensu-puppet/blob/master/.travis.yml#L140-L144

ph84172 commented 5 years ago

@ghoneycutt thanks - now updated.

ghoneycutt commented 5 years ago

@ph84172 Drats! Noticed a typo with centos7 vs 8. Could you please update/rebase and then I'll get this merged for real :)

ph84172 commented 5 years ago

@ghoneycutt should be good to go 👍

ph84172 commented 5 years ago

Ping @ghoneycutt 😊

anders-larsson commented 4 years ago

HI @ghoneycutt,

Is there anything preventing this from being merged? We would love to see RHEL8 support added :)

anders-larsson commented 4 years ago

Hi again,

After some further investigation is appears we might need to do further changes for this pull request. In RHEL8 they've introduced authselect which appears to be a new way of customizing PAM configuration. It allows you to have multiple PAM configuration and select which PAM configuration should be used.

Current implemention (old way) still appears to work however maybe we should try to align us to RedHat's way of working? This could make it easier to introduce non-default setups as well such as QAS.

ph84172 commented 4 years ago

It's certainly worth revisiting - the current PR had some workarounds for the fact there were no CentOS 8 images around at the time.

ghoneycutt commented 4 years ago

Thanks everyone!