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

Add configuration dumps for 2.x #1589

Merged
merged 16 commits into from
Jul 28, 2021
Merged

Add configuration dumps for 2.x #1589

merged 16 commits into from
Jul 28, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 26, 2021

What this PR does / why we need it:
Adds --dump-config to 2.x. Expose successful and failed configurations via a new endpoint on the diagnostics server.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1308

Special notes for your reviewer:
This intentionally introduces breaking changes from 1.x:

  • Dumped configs are no longer saved to the filesystem. The diagnostic server is less cumbersome to use.
  • Flag now accepts a boolean with an additional --dump-sensitive-config flag for redaction, rather than a single flag that accepts various string modes.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest temporarily deployed to Configure ci July 26, 2021 18:08 Inactive
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #1589 (0e1b00b) into next (7f3f440) will increase coverage by 0.58%.
The diff coverage is 71.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1589      +/-   ##
==========================================
+ Coverage   45.64%   46.22%   +0.58%     
==========================================
  Files          71       71              
  Lines        6662     6736      +74     
==========================================
+ Hits         3041     3114      +73     
+ Misses       3258     3254       -4     
- Partials      363      368       +5     
Flag Coverage Δ
integration-test 46.22% <71.13%> (+0.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/cmd/rootcmd/run.go 33.33% <50.00%> (+8.33%) ⬆️
internal/sendconfig/common_workflows.go 65.95% <54.16%> (-7.96%) ⬇️
internal/cmd/rootcmd/servers.go 55.93% <70.58%> (+7.93%) ⬆️
internal/proxy/clientgo_cached_proxy_resolver.go 60.26% <75.00%> (+0.53%) ⬆️
internal/diagnostics/server.go 67.56% <78.04%> (+6.02%) ⬆️
internal/manager/config.go 92.50% <100.00%> (+0.12%) ⬆️
internal/manager/run.go 47.82% <100.00%> (ø)
internal/manager/setup.go 66.66% <100.00%> (+0.38%) ⬆️
internal/ctrlutils/ingress-status.go 67.67% <0.00%> (-1.35%) ⬇️
internal/kongstate/kongstate.go 37.55% <0.00%> (+4.48%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f3f440...0e1b00b. Read the comment docs.

@rainest rainest temporarily deployed to Configure ci July 26, 2021 18:33 Inactive
@rainest rainest temporarily deployed to Configure ci July 26, 2021 18:52 Inactive
@rainest rainest temporarily deployed to Configure ci July 26, 2021 19:18 Inactive
@rainest rainest marked this pull request as ready for review July 26, 2021 19:19
@rainest rainest requested a review from a team as a code owner July 26, 2021 19:19
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Overall 👍

I do however have some blocking comments that I feel need resolution before we merge this. Please feel free to get ahold of me in Slack/Zoom if you desire faster turnaround to get this unblocked and we can talk though it.

pkg/sendconfig/common_workflows.go Outdated Show resolved Hide resolved
pkg/sendconfig/common_workflows.go Outdated Show resolved Hide resolved
railgun/internal/diagnostics/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

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

Really looking for unit test case of the change also.

@rainest rainest temporarily deployed to Configure ci July 26, 2021 21:38 Inactive
@rainest rainest temporarily deployed to Configure ci July 26, 2021 22:02 Inactive
@rainest rainest temporarily deployed to Configure ci July 26, 2021 22:22 Inactive
@rainest
Copy link
Contributor Author

rainest commented Jul 26, 2021

Really looking for unit test case of the change also.

We have existing units for the sanitizers, which is only part I think we can easily unit test. Did you have anything in mind for additional unit test strategies? Added 307c4011 to test beyond just "does the controller not crash with the flag on", but I don't know if we can really unit test the interface in any meaningful way.

@rainest rainest requested review from ccfishk and shaneutt July 26, 2021 22:23
@rainest rainest temporarily deployed to Configure ci July 26, 2021 23:19 Inactive
@rainest rainest temporarily deployed to Configure ci July 26, 2021 23:39 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 28, 2021 04:24 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 28, 2021 04:29 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 28, 2021 04:46 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 28, 2021 05:00 Inactive
@ccfishk ccfishk self-requested a review July 28, 2021 16:14
Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

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

lgtm.

@rainest rainest temporarily deployed to Configure ci July 28, 2021 16:37 Inactive
Travis Raines and others added 15 commits July 28, 2021 16:46
Add revised --dump-config handling to 2.x. Config dumps are now exposed
at /debug/config/successful and /debug/config/failed endpoints on the
diagnostic server. --dump-config is now a boolean, with an accompanying
boolean --dump-sensitive-config to include sensitive information.
- Rename diagConfig variable.
- Add explanatory comment.
- Correct inverted boolean check.
- Log errors when diag buffer overflows.
- Move defers before failure returns.
@shaneutt
Copy link
Contributor

shaneutt commented Jul 28, 2021

@rainest I proactively rebased this against next as per #1591, I ran tests and builds at each stop so you should be all set let me know if there are any problems.

As a note, if you want to redo the commits I would recommend doing a git reset --soft origin/next from here and you can completely redo the commit history from the diff.

@rainest
Copy link
Contributor Author

rainest commented Jul 28, 2021

The race occurs because the globs for storing dumps are getting written at the same time something is trying to read them via the HTTP endpoints, the act of R/W that data will need to be made threadsafe.

Right, that much makes sense--I was confused why my initial attempt to add a lock didn't avoid that. Some difference in 0e1b00b appears to resolve that, either using an RWMutex instead (doesn't seem like it should matter, but more correct for what this does, and doesn't prevent concurrent read requests) or making the mutex part of the server struct instead of spawning it in Listen() and passing it into the functions (seems like this would be more likely to break something, but thinking about the specifics is confusing, so 🤷 ).

@rainest rainest merged commit 10c0b42 into next Jul 28, 2021
@rainest rainest deleted the feat/rg-dump-cfg branch July 28, 2021 21:49
@@ -18,6 +19,7 @@ type Server struct {
Logger logr.Logger
ProfilingEnabled bool
ConfigDumps util.ConfigDumpDiagnostic
ConfigLock sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

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

Where the object is instantiated ? I mean ConfigLock = sync.RWMutex{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instantiating a Server implicitly instantiates an empty one. Similar to the example in https://tour.golang.org/concurrency/9. Golangisms 🤷

@rainest rainest mentioned this pull request Jul 28, 2021
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