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

Tweek logging to make it more useful to track errors on a production service #930

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

dmargery
Copy link
Contributor

The default behaviour trashes the log file with large blobs of compressed results for calls to ListResources, and warns of failure with too little information for debug. This PR changes the balance.

For large outputs of ListResources, even when debug is not activated,
the message built upon exit of each call is far too large, especialy
when it is compressed : this brings little value to the log
@ahelsing
Copy link
Member

Changes look pretty good to me.

dmargery added 5 commits May 21, 2019 15:55
A per python docs, the built-in HTTP request handler is not fit for
production. Add an option to use geni-tools in production by having it
listen on localhost, using http, to be called by a frontend that will
handle the TLS handshake and pass the client certificate as an HTTP
Header.

To use this variant, you need to set proto and certheader options in the
configuration file.
A per python docs, the built-in HTTP request handler is not fit for
production. Add an option to use geni-tools in production by having it
listen on localhost, using http, to be called by a frontend that will
handle the TLS handshake and pass the client certificate as an HTTP
Header.

To use this variant, you need to set proto and certheader options in the
configuration file.
@bluke
Copy link

bluke commented Aug 24, 2021

Any news on whether this PR (or !938), are being considered for upstreaming.

@hussamnasir
Copy link
Contributor

unfortunately, only minimal patchwork when needed to plug security holes is only being taken up. Many other tools are dependent on this repo for their functionality and we do not have enough resources to test for all those scenarios.

@ahelsing
Copy link
Member

The new changes allowing fronting with Apache make sense to me.

Can you verify that old behavior / arguments will not see any changes to which RPCServer is used and use of https/threading (it appears so eyeballing the code)?

Can you speak to what testing you have done?

Eyeballing the code it looks good, but as Hussam says, others depend on this code so he has to be cautious. (And to that point, combining the logging changes with the https/RPCServer changes make this PR harder to merge.)

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.

4 participants