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

RHDEVDOCS-3667 - Improve tlsConfig monitoring docs #41472

Conversation

bburt-rh
Copy link
Contributor

@bburt-rh bburt-rh commented Feb 4, 2022

This PR removes the Note admonition regarding TLS configuration for configuring metrics for user-defined projects.

It also adds a Note admonition informing the user that the prometheus-example-app does not support TLS authentication and adds a link to a RH KCS article about how to scrape metrics using TLS in a ServiceMonitor configuration.

Applies to OCP 4.7+ versions

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 4, 2022
@netlify
Copy link

netlify bot commented Feb 4, 2022

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 398dd73

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/6206b8aef442d400074a833e

😎 Browse the preview: https://deploy-preview-41472--osdocs.netlify.app

@bburt-rh bburt-rh force-pushed the RHDEVDOCS-3667-Improve-tlsConfig-Monitoring-docs branch from d319ada to e2d0980 Compare February 4, 2022 21:15
@bburt-rh
Copy link
Contributor Author

bburt-rh commented Feb 4, 2022

@philipgough Can you provide input about this PR plus suggest ways that I can adjust the configuration settings of the entire example outlined at https://docs.openshift.com/container-platform/4.9/monitoring/managing-metrics.html#setting-up-metrics-collection-for-user-defined-projects_managing-metrics for TLS authentication? Do we completely need to redo these samples along the lines of your write-up? Or is there a simpler way? Thanks in advance for your input!

@philipgough
Copy link

@bburt-rh - thanks for doing this.

Personally, I don't think there is a simple way of doing this and the article I wrote at https://access.redhat.com/articles/6675491 is a detailed step-by-step walkthrough of configuring TLS in a sidecar and tweaking the other resources appropriately. For this to work, all steps from that article are required.

I would think it would be best as a standalone addition alongside the simple example of standard configuration.
The prometheus-example-app does not actually support TLS (although we could make that happen if that was preferred) which is why I added the sidecar.

So I would probably defer back to you as the docs expert. If you think it would be better to have a standalone example we can create a ticket and make the example app support TLS. Otherwise, I think taking the article as it is and adding additional section is the way to go (and maybe a more useful example IMO).

@bburt-rh
Copy link
Contributor Author

bburt-rh commented Feb 8, 2022

Personally, I don't think there is a simple way of doing this and the article I wrote at https://access.redhat.com/articles/6675491 is a detailed step-by-step walkthrough of configuring TLS in a sidecar and tweaking the other resources appropriately. For this to work, all steps from that article are required.

I would think it would be best as a standalone addition alongside the simple example of standard configuration. The prometheus-example-app does not actually support TLS (although we could make that happen if that was preferred) which is why I added the sidecar.

So I would probably defer back to you as the docs expert. If you think it would be better to have a standalone example we can create a ticket and make the example app support TLS. Otherwise, I think taking the article as it is and adding additional section is the way to go (and maybe a more useful example IMO).

Thanks for the detailed reply @philipgough.

I'm hesitant to add a whole new standalone section to the Monitoring docs with the content you provided in the knowledge article for a couple of reasons, First, it breaks up the "user journey" flow of the Monitoring docs through the example app as currently constructed. Second, for the OpenShift docs at least, we generally don't, and can't, provide complete, working, end-to-end code examples for every piece of configuration (like TLSConfig) an Operator supports. That's simply not feasible given the resources we have and the maintenance that would entail. So, for one-off situations or corner cases where we provide a standalone, end-to-end code example to meet a specific need, I think it's more appropriate to share it as a KCS in the hope that it also may be applicable in some other similar situations rather than including that content in the product docs.

As a general principle, it should be enough to include general config procedures with short samples along with complete API reference docs that describe the possible config parameters. Of course, this assumes that users are knowledgeable enough to put the pieces together for their particular needs.

My question at this point is what could we add about TLSConfig to this section of the current docs or to the Troubleshooting section at https://docs.openshift.com/container-platform/4.10/monitoring/troubleshooting-monitoring-issues.html#investigating-why-user-defined-metrics-are-unavailable_troubleshooting-monitoring-issues that would be of general use for users? Do you have any thoughts about this question? Thanks!

@philipgough
Copy link

@bburt-rh yes understood, that seems like a valid enough explanation not to include a full e2e example.

My question at this point is what could we add about TLSConfig to this section of the current docs or to the Troubleshooting section

I don't think there is much we can add that makes any sense outside the complete example to be honest. I actually think it would makes matter more confusing than helping the situation.

@bburt-rh
Copy link
Contributor Author

bburt-rh commented Feb 9, 2022

yes understood, that seems like a valid enough explanation not to include a full e2e example.

My question at this point is what could we add about TLSConfig to this section of the current docs or to the Troubleshooting section

I don't think there is much we can add that makes any sense outside the complete example to be honest. I actually think it would makes matter more confusing than helping the situation.

I agree @philipgough. I think it would be useful to add a note to the docs that the prometheus-example-app doesn't support TLS, just so that users know about this limitation. Would you agree?

@philipgough
Copy link

@bburt-rh sgtm

@simonpasquier
Copy link

I agree with what you both said.

@bburt-rh bburt-rh force-pushed the RHDEVDOCS-3667-Improve-tlsConfig-Monitoring-docs branch from e2d0980 to 6ccd98f Compare February 9, 2022 17:23
@bburt-rh
Copy link
Contributor Author

bburt-rh commented Feb 9, 2022

@philipgough @simonpasquier Can you ptal at the revised content for this PR? Thanks!

Copy link

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

/lgtm

we could support TLS in the example app but IMO that still would not stop the doc being too verbose if we were providing the full e2e example.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2022
@bburt-rh bburt-rh force-pushed the RHDEVDOCS-3667-Improve-tlsConfig-Monitoring-docs branch from 6ccd98f to a872652 Compare February 11, 2022 15:13
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2022
@openshift-ci
Copy link

openshift-ci bot commented Feb 11, 2022

New changes are detected. LGTM label has been removed.

Copy link
Member

@Srivaralakshmi Srivaralakshmi left a comment

Choose a reason for hiding this comment

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

@bburt-rh Small nitpick, otherwise LGTM.

@bburt-rh bburt-rh force-pushed the RHDEVDOCS-3667-Improve-tlsConfig-Monitoring-docs branch from a872652 to 398dd73 Compare February 11, 2022 19:27
@JStickler JStickler merged commit 593823b into openshift:main Feb 11, 2022
@JStickler
Copy link
Contributor

/cherry-pick enterprise-4.7

@JStickler
Copy link
Contributor

/cherry-pick enterprise-4.8

@JStickler
Copy link
Contributor

/cherry-pick enterprise-4.9

@JStickler
Copy link
Contributor

/cherry-pick enterprise-4.10

@openshift-cherrypick-robot

@JStickler: new pull request created: #41799

In response to this:

/cherry-pick enterprise-4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@JStickler: new pull request created: #41800

In response to this:

/cherry-pick enterprise-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@JStickler: new pull request created: #41801

In response to this:

/cherry-pick enterprise-4.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@JStickler: new pull request created: #41802

In response to this:

/cherry-pick enterprise-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bburt-rh bburt-rh deleted the RHDEVDOCS-3667-Improve-tlsConfig-Monitoring-docs branch June 6, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.7 branch/enterprise-4.8 branch/enterprise-4.9 branch/enterprise-4.10 dev-tools Label for all Odo/Pipelines/Helm/Developer Console/Perspective PRs size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants