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 pod logs related to Supervisor TLS certificate problems #1662

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Sep 7, 2023

This PR is meant to improve the situation described in #1393.

Because the certificate is selected inside a function which does not know the full details of the request, I was not able to log everything that I had hoped about each request. However, this PR is still an improvement and will hopefully be good enough.

This PR logs warnings that are visible at the default logging level when:

  • there is any error reading the TLS certs from the Supervisor's default TLS cert Secret
  • there is any error reading the TLS certs from the Secrets specified by FederationDomain.spec.tls.secretName
  • there is any incoming request which will fail due to bad or missing user-provided TLS certs

The PR also changes the log level of some useful log lines for more verbose logging to make them available at the info log level, which is a level that should be safe to use on a production server for debugging. Previously, some of these useful log statements were at higher levels that are not safe or not convenient to use on production Supervisor servers.

All of these log messages include the SNI name from the request, to help the user see if their ingress is stripping SNI names from incoming requests.

Any changes aside from log statements are intended to be supporting refactors only, and are not intended to change any behavior at all. Please review for any possible accidental behavior changes.

Note that in the plog package, only plog.Error and plog.Warning are printed to the pod logs at the default log level. In order to see plog.Info messages, a user would need to choose to deploy the Supervisor with the info log level setting (or a higher level).

Release note:

Improvements to Supervisor pod logs to aid in debugging issues related to the Supervisor's user-provided TLS certificates.

@djpbessems
Copy link

After my initial struggles, this seems a very welcome improvement :)

@cfryanr cfryanr force-pushed the supervisor_tls_cert_logging branch from 300c99c to 6c5f64b Compare September 8, 2023 16:22
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #1662 (ce567c4) into main (3331171) will decrease coverage by 0.01%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
- Coverage   75.56%   75.55%   -0.01%     
==========================================
  Files         166      166              
  Lines       15139    15159      +20     
==========================================
+ Hits        11440    11454      +14     
- Misses       3398     3404       +6     
  Partials      301      301              
Files Changed Coverage Δ
...l/controller/supervisorconfig/tls_cert_observer.go 89.41% <70.00%> (-6.18%) ⬇️
internal/oidc/provider/manager/manager.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@cfryanr cfryanr force-pushed the supervisor_tls_cert_logging branch from d21fecc to ce567c4 Compare September 11, 2023 16:14
@joshuatcasey joshuatcasey merged commit 96fcfe4 into main Sep 11, 2023
9 checks passed
@joshuatcasey joshuatcasey deleted the supervisor_tls_cert_logging branch September 11, 2023 17:10
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.

4 participants