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

Implement recursive SanitizedCopy() for KongState, refactor SetCredential #1021

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

mflendrich
Copy link
Contributor

@mflendrich mflendrich commented Jan 26, 2021

Please review commit by commit and merge without squashing.
Inspired by #991.

This PR:

  • defines Go types for credential types in KIC (as wrappers for go-kong types allowing for implementation of KIC business logic in those wrappers) consistently with wrappers for other go-kong types (Consumer, Service, Route, etc.)
  • implements a recursive Kongstate.SanitizedCopy() produces a copy of KongState containing only a subset of KongState fields,
  • refactors SetCredential by breaking its type-specific logic out to individual type creator functions NewXXX()

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #1021 (6c28445) into next (d2442de) will increase coverage by 1.55%.
The diff coverage is 85.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1021      +/-   ##
==========================================
+ Coverage   49.78%   51.33%   +1.55%     
==========================================
  Files          32       33       +1     
  Lines        3224     3327     +103     
==========================================
+ Hits         1605     1708     +103     
  Misses       1487     1487              
  Partials      132      132              
Impacted Files Coverage Δ
internal/ingress/controller/kong.go 6.66% <0.00%> (ø)
...ingress/controller/parser/kongstate/credentials.go 83.50% <83.50%> (ø)
...l/ingress/controller/parser/kongstate/kongstate.go 24.70% <90.00%> (+4.20%) ⬆️
...al/ingress/controller/parser/kongstate/consumer.go 100.00% <100.00%> (+25.00%) ⬆️
...ernal/ingress/controller/parser/kongstate/types.go 60.00% <100.00%> (+60.00%) ⬆️

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 d2442de...6c28445. Read the comment docs.

@mflendrich
Copy link
Contributor Author

mflendrich commented Jan 26, 2021

Closing as WIP

@mflendrich mflendrich closed this Jan 26, 2021
@mflendrich mflendrich reopened this Jan 26, 2021
@mflendrich mflendrich changed the title Implement recursive SanitizedCopy() for KongState Implement recursive SanitizedCopy() for KongState, refactor SetCredential Jan 26, 2021
shaneutt
shaneutt previously approved these changes Jan 27, 2021
rainest
rainest previously approved these changes Feb 1, 2021
@mflendrich mflendrich dismissed stale reviews from rainest and shaneutt via dc111c3 February 2, 2021 10:37
@mflendrich mflendrich changed the base branch from main to next February 2, 2021 10:37
@rainest rainest self-requested a review February 2, 2021 17:35
rainest
rainest previously approved these changes Feb 2, 2021
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Change that dismissed the old approval was just the retarget to next, correct? Re-approving since I don't think anything else changed.

To summarize some conversation from yesterday, there are two different properties I'd ideally like for the redacted dumps:

  • You can diff dumps from before and after a change in K8S configuration to see what changed in the resulting Kong configuration.
  • You can apply a dump to Kong via decK or a /config POST.

These unfortunately work against one another. Replacing sensitive values with just REDACTED prevents you from applying the config because it creates conflicting credentials and invalid certificates (we check key validity), whereas using random valid values breaks the ability to compare before/after dumps effectively.

I'd explored trying to generate garbage valid configuration that only changed if the underlying actual values changed, but that proved tricky to get right. Stripping certificates, credentials, and references to them from config can also produce valid configuration, but goes against @mflendrich's wish to not cherry-pick the redacted content.

In the interest of expediency, I think it makes sense to choose one or the other and live with the shortcomings. This version preserves diffability.

@mflendrich
Copy link
Contributor Author

Thanks for the reviews!

Change that dismissed the old approval was just the retarget to next, correct?

Retarget to next + rebase.

@mflendrich mflendrich merged commit 6c28445 into next Feb 2, 2021
@mflendrich mflendrich deleted the feat/sanitized-copy branch February 2, 2021 20:57
rainest pushed a commit that referenced this pull request Feb 3, 2021
Merge didn't conflict these since they were in different locations in
the file. Remove old versions in favor of the #1021 version.
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.

3 participants