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.

Sign up for GitHub

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

Jump to bottom

Expose pprof endpoint if tls is not configured #2422

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Expose pprof endpoint if tls is not configured #2422

merged 2 commits into from
Oct 14, 2021

Conversation

awgreene
Copy link
Member

Problem: Currently, if OLM is not configured to use TLS it is not
possible to reach the pprof endpoint. This limitation makes it
difficult to debug complex performance problems on clusters where
OLM is not configured to use tls.

Solution: If OLM is not configured to use tls, do not require a
tls certificate to access the pprof endpoint.

@openshift-ci openshift-ci bot requested review from ecordell and exdx October 13, 2021 13:28
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2021
@awgreene
Copy link
Member Author

CC @benluddy for his thoughts on this.

@njhale
Copy link
Member

njhale commented Oct 13, 2021 •
edited
Loading

I think this behavior is much more intuitive for debugging purposes and brings profiling in line with the behavior of the /metrics and /healthz endpoints (allow unauthorized access). If this PR is merged, it should be followed up by enabling TLS in the default deployment mechanism for OLM (the charts) as well as the GitHub workflow e2e tests.

The alternative (as @benluddy pointed out in other channels) is to add some sort of --allow-unauthorized option that lets a user explicitly accept the consequences of (partly) disabling mtls for profiling. This approach allows us to preserve the current default behavior while also avoids making cert generation (ala ffsctl) a requirement of the common install path.

edit: @benluddy suggested we could also use the existing --debug option to enable/disable mtls for profiling.

@awgreene awgreene force-pushed the pprof-non-tls-endpoint branch 2 times, most recently from cc7a217 to 1643cb5 Compare October 13, 2021 16:32
@awgreene
Copy link
Member Author

edit: @benluddy suggested we could also use the existing --debug option to enable/disable mtls for profiling.

@njhale went with this approach. If OLM is not configured to use certs and is in debug mode the pprof endpoint will be available.

@awgreene
Expose pprof endpoint when debugging
a5e9247 
This commit introduces a change that allows requests to the pprof
endpoint to be made without a client certificate when OLM is not
configured to use certificates and is ran with the --debug
flag.
njhale
njhale reviewed Oct 13, 2021
View reviewed changes
pkg/lib/server/server.go Outdated Show resolved Hide resolved
pkg/lib/server/server.go Outdated Show resolved Hide resolved
njhale
njhale approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021
@openshift-ci
Copy link

openshift-ci bot commented Oct 13, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, njhale

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@timflannagan
Copy link
Contributor

Holding to avoid the bot spam.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021
@timflannagan
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021
@njhale njhale merged commit ff760a1 into operator-framework:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan left review comments

@njhale njhale njhale approved these changes

@ecordell ecordell Awaiting requested review from ecordell

@exdx exdx Awaiting requested review from exdx

Assignees

@njhale njhale

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@awgreene @njhale @openshift-bot @timflannagan