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

Improve tls certificate documentation #644

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Improve tls certificate documentation #644

wants to merge 5 commits into from

Conversation

scoopex
Copy link
Contributor

@scoopex scoopex commented Jul 11, 2024

No description provided.

@scoopex scoopex self-assigned this Jul 11, 2024
@scoopex scoopex marked this pull request as draft July 11, 2024 14:15
@scoopex scoopex changed the title improve documenatin [DRAFT] improve documentation Jul 11, 2024
@scoopex scoopex changed the title [DRAFT] improve documentation [DRAFT] improve tls certificate documentation Jul 23, 2024
@scoopex scoopex marked this pull request as ready for review September 3, 2024 11:13
@scoopex scoopex changed the title [DRAFT] improve tls certificate documentation Improve tls certificate documentation Sep 3, 2024
Copy link
Contributor

@artificial-intelligence artificial-intelligence 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 we need to spell out which settings exactly are hard to change later on.

@artificial-intelligence
Copy link
Contributor

Rereading this I'm actually not so sure what value this does bring.

Maybe we could add a generic warning, which really isn't specific to TLS imho, that if you want to do sweeping changes to your Cloudconfiguration, e.g. when testing things, building an evaluation lab etc. you are way better off always to reprovision from scratch your complete cloud.

E.g. if you decide in the middle of the cloud install to switch to let's encrypt, or change your cinder backend I wouldn't bet my life on it that everything is properly cleaned up by neither kolla, nor osism.

These are - at it's core - configuration management tools, which have trouble tracking current state. These are not infrastructure as code tools such as Terraform, which do track state.

So there is always the need in ansible to write dedicated cleanup tasks, if you want to purge/remove something. This also assumes that you can just reconfigure any deployed openstack service on the fly, when these store some settings also persisted in databases, which also might not be as easily available to outside changes, even if you would use something like terraform.

The result of all these constraints is, that you usually figure out a working deployment setup, with all your tested settings in git, and then deploy that once.

It's relatively smooth to upgrade this then, and with care, you also can introduce changes, but sometimes you need to do manual cleanups, given the above constraints.

So in general I would put this up as a warning.

A Cloud Infrastructure isn't something where you can easily rip out parts and just replace them with something else.

That's even the case if you run everything inside k8s openstack-operators instead of ansible, as these are usually also full of assumptions on what a "supported" configuration looks like.

In practice, this often means only one or two knobs are actually configurable by the end user and the rest is enforced via the operators code, which can't be easily changed as well.

Copy link
Contributor

@artificial-intelligence artificial-intelligence left a comment

Choose a reason for hiding this comment

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

See my global comment.

I don't think a detailed warning in the loadbalancer guide makes sense. This is a rather global feature about how cloud provisioning works in general.

So we could add a global warning to make large changes with care and that they might require manual cleanup.

Thanks

@scoopex
Copy link
Contributor Author

scoopex commented Sep 18, 2024

I don't think a detailed warning in the loadbalancer guide makes sense. This is a rather global feature about how cloud provisioning works in general.

So we could add a global warning to make large changes with care and that they might require manual cleanup.

Thanks

I agree with you in general. Actually, it would be better to bundle such things in a kind of “pre-installation checklist” and provide information on this topic.

As a result of a small pilot project that I have with a prospective scs-user of the public sector, I recently started writing something like this. However, it is still in a very very early and very immature stage and I am very happy to receive ideas and contributions.

Nevertheless, I also think it is important to describe this here, because there are specific references to the configuration of domains and certificates (we don't always know which path a user takes or whether the user is aware that information is provided elsewhere).

I rephrased the last state a bit to make it better understandable/readable.

I also added a new documentation section for Let's encrypt based on the kolla-ansible documentation.
(i haven't practiced that yet, so please provide feedback if there is something to precise enough or wrong)

Copy link
Contributor

@artificial-intelligence artificial-intelligence left a comment

Choose a reason for hiding this comment

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

The concrete commands are mostly looking fine, I made just some spelling corrections.
But I think the general warnings throughout the text are to vague and unspecific to be helpful, see my more detailed comments.

Thanks for working on this!

Apart from my comments, the rest LGTM. :)

docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
Comment on lines 33 to 34
To avoid unnecessary additional work and problems, it is recommended that you configure TLS with the intended target
configuration of the specific environment during the initial rollout process.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very vague, without the knowledge what can go wrong I don't think any user without deep knowledge about this topic can understand this warning.

What work? What problems? Imho a warning should be very specific what can go wrong why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have completely revised the explanation and made it more specific. Does that fit better? What do you suggest?

Comment on lines 40 to 41
As a result, the involved Ansible Plays must at least be executed in the correct order, and not all Ansible Plays can handle all possible configuration transitions on their own.
Therefore, in some cases, manual adjustments must be made to the systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

how could the operator run this in a wrong order? ansible plays in general need to be executed in the correct order, this is not really a specific property of setting up TLS in osism? Also if we mention this at all, what is the correct order to execute the ansible plays? It's not mentioned here?

```yaml title="environments/kolla/configuration.yml"
kolla_enable_tls_internal: "yes"
```
For this reason, we recommend that you define domains, obtain certificates, and perform configurations in advance.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this either. "in advance", before doing what next?

Please be specific about the order of things being done, e.g. make a numbered list, if it is deemed important.

e.g. when you use let's encrypt, it's not possible to obtain certificates in advance.

You certainly need to define your used FQDN (domain) before deploying osism, but that's orthogonal to using TLS? What kind of "configuration" are you referring to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the description more detailed.
Please recheck, if that's a bit clearer now?

docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
docs/guides/configuration-guide/loadbalancer.md Outdated Show resolved Hide resolved
Co-authored-by: Sven <[email protected]>
Signed-off-by: Marc Schöchlin <[email protected]>
@scoopex scoopex force-pushed the loadbalancer branch 2 times, most recently from 17c3768 to 725af1e Compare October 11, 2024 07:46
Signed-off-by: Marc Schöchlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants