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.

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

Refs #29715: Add mongodb server and client certs #285

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented May 11, 2020

No description provided.

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.

Would you mind creating an acceptance test so it actually runs the code?

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.

Looks like it fails because /etc/pulp doesn't exist when it it tries to create the files.

       	Error: /Stage[main]/Certs::Mongodb/Certs::Keypair[mongodb_client]/Privkey[/etc/pulp/mongodb-client-certificate.key]/ensure: change from 'absent' to 'present' failed: Could not set 'present' on ensure: No such file or directory @ rb_sysopen - /etc/pulp/mongodb-client-certificate.key (file: /etc/puppetlabs/code/environments/production/modules/certs/manifests/keypair.pp, line: 18)

This is also very likely going to be a problem when you deploy them because the class is usually set up to first create certs and then deploy pulp.

@ehelms ehelms force-pushed the refs-29715 branch 3 times, most recently from d5af860 to 0e9a37e Compare May 13, 2020 01:15
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.

I think this should be split in certs::mongodb::server and certs::mongodb::client. This will allow us to deploy just the client certs on a server without mongodb present.

manifests/mongodb.pp Outdated Show resolved Hide resolved
manifests/mongodb.pp Outdated Show resolved Hide resolved
manifests/mongodb.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented May 13, 2020

I do appreciate the idea and power behind splitting, this class is intended for a rather specific purpose and will be short lived as Pulp 2 is not long left of life in our ecosystem. I prefer to keep this is together and simple.

@ekohl
Copy link
Member

ekohl commented May 13, 2020

My reasoning is that we will need them in Pulpcore to import. There the parameters (/etc/pulpcore/mongodb + pulpcore user) will make sense. In that case there will not be a server.

@ehelms ehelms force-pushed the refs-29715 branch 2 times, most recently from d70e910 to 83c38c0 Compare May 29, 2020 13:08
@ehelms ehelms force-pushed the refs-29715 branch 3 times, most recently from ffdf4ec to 26fc14e Compare May 29, 2020 14:28
@ehelms
Copy link
Member Author

ehelms commented May 29, 2020

Tis green now and split into two classes.

@ehelms
Copy link
Member Author

ehelms commented Jun 3, 2020

@ekohl Ready for re-review please

manifests/mongodb_client.pp Outdated Show resolved Hide resolved
manifests/mongodb_client.pp Outdated Show resolved Hide resolved
manifests/mongodb.pp Outdated Show resolved Hide resolved
manifests/mongodb.pp Outdated Show resolved Hide resolved
@ehelms ehelms requested a review from ekohl June 4, 2020 19:49
@ehelms ehelms merged commit fc8e889 into theforeman:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants