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

fix(konnect) avoid collisions when redacting #5964

Merged
merged 4 commits into from
May 10, 2024
Merged

fix(konnect) avoid collisions when redacting #5964

merged 4 commits into from
May 10, 2024

Conversation

rainest
Copy link
Contributor

@rainest rainest commented May 3, 2024

What this PR does / why we need it:

Change the static redacted pretend Vault string to a function that generates a random pretend Vault string.

This avoids collisions for certain types of values when go-database-reconciler builds configuration. It uses an in-memory database that looks up Kong entities by unique fields. If these fields aren't actually unique, go-database-reconciler will detect a conflict and abort.

Which issue this PR fixes:

Reported by @mheap in chat.

Special notes for your reviewer:

Manual smoke testing: creating a key-auth with {vault://04a84f95-91f0-4caa-87d1-91a2caa2a6ca} as the key value to validate that UUIDs are valid Vault keys.

Originally did this for all redacted strings, but turns out that the random stuff needs accounting for in tests in less than intuitive ways, so limited to the resources we know have this problem for certain. key-auth is probably alone there, as one of the few (only?) things where the primary key has to be sensitive.

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

Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@755e2ce). Click here to learn what that means.
Report is 17 commits behind head on main.

❗ Current head 36538e8 differs from pull request most recent head 2ee6705. Consider uploading reports for the commit 2ee6705 to get more accurate results

Additional details and impacted files
@@          Coverage Diff           @@
##             main   #5964   +/-   ##
======================================
  Coverage        ?   68.0%           
======================================
  Files           ?     179           
  Lines           ?   18307           
  Branches        ?       0           
======================================
  Hits            ?   12464           
  Misses          ?    4868           
  Partials        ?     975           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

When generating sanitized configuration for Konnect, use random values
to redact unique fields.

Internally, go-database-reconciler builds an in-memory database with
certain fields as primary keys. Using static redacted values for these
results in collisions in that database.
@rainest rainest marked this pull request as ready for review May 3, 2024 23:04
@rainest rainest requested a review from a team as a code owner May 3, 2024 23:04
@rainest rainest enabled auto-merge (squash) May 3, 2024 23:05
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

LGTM, but is the KeyAuth the only credential part that we need to de-deplecate in consumers?

@pmalek pmalek added the area/konnect Issues and PRs related to Konnect label May 6, 2024
@rainest
Copy link
Contributor Author

rainest commented May 6, 2024

So far as we know now, yeah. It seems the only one likely due to its (I think) unique "primary key is the one and only value" situation, though we could potentially have others.

I opted for the more targeted fix given the havoc that random values play with tests. If we find that others are affected we may want to randomize everything and refactor tests to accommodate that.

@rainest rainest requested a review from randmonkey May 6, 2024 19:48
@randmonkey randmonkey added the ci/run-e2e Trigger e2e test run from PR label May 7, 2024
@team-k8s-bot
Copy link
Collaborator

E2E (targeted) tests with KIND-based clusters were started at https://github.com/Kong/kubernetes-ingress-controller/actions/runs/8978822226

@team-k8s-bot team-k8s-bot removed the ci/run-e2e Trigger e2e test run from PR label May 7, 2024
randmonkey
randmonkey previously approved these changes May 7, 2024
Co-authored-by: Grzegorz Burzyński <czeslavo@gmail.com>
@randmonkey
Copy link
Contributor

@rainest Please check the linter comments on import section of plugin_test.go and kongupstreampolicy_test.go.

@rainest rainest requested review from pmalek and randmonkey May 9, 2024 21:55
@rainest rainest merged commit 4b60bf8 into main May 10, 2024
36 checks passed
@rainest rainest deleted the fix/rand-redact branch May 10, 2024 02:52
@team-k8s-bot
Copy link
Collaborator

The backport to release/3.1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.1.x release/3.1.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.1.x
# Create a new branch
git switch --create backport-5964-to-release/3.1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4b60bf8cd8074fb310b8270e623fe8f080cdd934
# Push it to GitHub
git push --set-upstream origin backport-5964-to-release/3.1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.1.x

Then, create a pull request where the base branch is release/3.1.x and the compare/head branch is backport-5964-to-release/3.1.x.

@pmalek
Copy link
Member

pmalek commented May 10, 2024

@rainest The backport to 3.1 has failed.

randmonkey pushed a commit that referenced this pull request May 10, 2024
When generating sanitized configuration for Konnect, use random values
to redact unique fields.

Internally, go-database-reconciler builds an in-memory database with
certain fields as primary keys. Using static redacted values for these
results in collisions in that database.

(cherry picked from commit 4b60bf8)
@randmonkey
Copy link
Contributor

Added manual backport PR: #6001
CHANGELOGS :-(

randmonkey pushed a commit that referenced this pull request May 11, 2024
When generating sanitized configuration for Konnect, use random values
to redact unique fields.

Internally, go-database-reconciler builds an in-memory database with
certain fields as primary keys. Using static redacted values for these
results in collisions in that database.

(cherry picked from commit 4b60bf8)
randmonkey added a commit that referenced this pull request May 11, 2024
…#6001)

* fix(konnect) avoid collisions when redacting (#5964)

When generating sanitized configuration for Konnect, use random values
to redact unique fields.

Internally, go-database-reconciler builds an in-memory database with
certain fields as primary keys. Using static redacted values for these
results in collisions in that database.

(cherry picked from commit 4b60bf8)

* add CHANGELOG entry to backport

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/konnect Issues and PRs related to Konnect backport release/3.1.x size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants