Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refs #30346 - override candlepin client keypair group #358

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

jturel
Copy link
Contributor

@jturel jturel commented Jul 22, 2020

No description provided.

@jturel
Copy link
Contributor Author

jturel commented Jul 22, 2020

The related puppet-certs change broke the CandlepinEventListener by changing the client keypair group to tomcat. This overrides it to the foreman user, and it can be overridden to vagrant in dev

@jturel
Copy link
Contributor Author

jturel commented Jul 22, 2020

See also theforeman/puppet-certs#295

@jturel
Copy link
Contributor Author

jturel commented Jul 22, 2020

@ekohl @ehelms thoughts?

@ekohl
Copy link
Member

ekohl commented Jul 22, 2020

I still want to split the class into two. It still deploys files it simply doesn't need at all on the Candlepin server. For those who do deploy Candlepin on a different physical server than Katello, this will break things. We do have those in the community and IMHO should be the default for a dev server - reduces the size of the dev box itself, making rebuilds easier.

@jturel
Copy link
Contributor Author

jturel commented Jul 22, 2020

@ekohl It makes perfect sense, but my goal here is to get nightlies green

@jturel
Copy link
Contributor Author

jturel commented Jul 27, 2020

@ekohl @ehelms can this collection of PRs be merged to address nightly issues?

@ehelms
Copy link
Member

ehelms commented Jul 27, 2020

The failing acceptance test implies you'll need to create the group in this file: https://github.com/theforeman/puppet-katello/blob/master/spec/setup_acceptance_node.pp

Copy link
Member

@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.

The failing test indicates this is a bad idea and you're breaking the contract that you can deploy Candlepin on a standalone server. The classes should be split into one that is for the server and one that is for the client.

@ehelms
Copy link
Member

ehelms commented Jul 27, 2020

I think we need to fix that based on our ability to, and fix what is broken today to unblock the predominant set of users.

@ehelms
Copy link
Member

ehelms commented Jul 27, 2020

Issue to track -- https://projects.theforeman.org/issues/30497

@jturel
Copy link
Contributor Author

jturel commented Jul 27, 2020

The failing test indicates this is a bad idea and you're breaking the contract that you can deploy Candlepin on a standalone server. The classes should be split into one that is for the server and one that is for the client.

I perceived your prior feedback as an out-of-scope RFE. Nevertheless, I'm struggling to see how it plays out in reality:

  • I spin up my Candlepin server with puppet-candlepin, and it has (new) certs::candlepin::server applied
  • I spin up my Katello server with puppet-katello and it has (new) certs::candlepin::client applied. The Candlepin server needs to generate the client certs - how is that managed?

Thanks in advance for your detailed explanation

@ekohl
Copy link
Member

ekohl commented Jul 27, 2020

So certs are certainly a weak spot here. You do need to generate a tarball with them beforehand. Sadly puppet-certs is not yet very good at that. It is how @timogoebel and his colleagues at DM have done it in production and they use these classes individually.

@jturel
Copy link
Contributor Author

jturel commented Jul 27, 2020

@ehelms thanks for filing a follow-up issue! the failure in travis now appears unrelated (all but one jobs pass).

@ekohl can you / your team look at fixing the larger issue filed by Eric? I don't feel this change has gone far enough in the wrong direction to not merge it if https://projects.theforeman.org/issues/30497 will be addressed soon. I'd like to see nightly passing this week.

@jturel
Copy link
Contributor Author

jturel commented Jul 27, 2020

puppet6 + el8 failing with:

  Generating/signing web server's SSL certificate: localhost-tomcat.crt

  Error: /Stage[main]/Certs::Candlepin/Cert[localhost-tomcat]/ensure: change from 'absent' to 'present' failed: Execution of '/usr/bin/katello-ssl-tool --gen-server --set-hostname localhost --server-cert localhost-tomcat.crt --server-cert-req localhost-tomcat.crt.req --server-key localhost-tomcat.key --server-rpm localhost-tomcat -p file:/etc/pki/katello/private/katello-default-ca.pwd --set-hostname localhost --set-common-name localhost --ca-cert /etc/pki/katello-certs-tools/certs/katello-default-ca.crt --ca-key /etc/pki/katello-certs-tools/private/katello-default-ca.key --set-country US --set-state North Carolina --set-city Raleigh --set-org Katello --set-org-unit SomeOrgUnit --set-email  --cert-expiration 7300' returned 22: ERROR: web server's SSL certificate generation/signing failed:

  

  Using configuration from /root/ssl-build/katello-ca-openssl.cnf

  unable to load number from /root/ssl-build/serial

  error while loading serial number

  139967517005632:error:0D066091:asn1 encoding routines:a2i_ASN1_INTEGER:odd number of chars:crypto/asn1/f_int.c:103:

  

  

  Generating the web server's SSL private key: ./ssl-build/localhost/localhost-tomcat.key

  Rotated: katello-server-openssl.cnf --> katello-server-openssl.cnf.1

@ehelms
Copy link
Member

ehelms commented Jul 28, 2020

@jturel That happens sometimes, not sure why, its a bit transient. Now things are green.

@ehelms ehelms merged commit 5e681e5 into theforeman:master Jul 28, 2020
@jturel jturel deleted the client_keypair_group branch July 28, 2020 13:09
@wbclark wbclark added Bug and removed Needs testing labels Aug 7, 2020
@wbclark wbclark mentioned this pull request Aug 7, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants