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: run basic credentials validation despite relation with consumers #4887

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Oct 18, 2023

What this PR does / why we need it:

Runs credsvalidation.ValidateCredentials just after ensuring the Secret has the kongCredType key. It will make the admission webhook validate credentials' Secrets' schema even if they're not associated with any KongConsumer yet.

Which issue this PR fixes:

Fix for #4885.

Special notes for your reviewer:

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

@czeslavo czeslavo force-pushed the fix/credentials-basic-validation branch from 50564ef to 2a7749c Compare October 18, 2023 11:11
@czeslavo czeslavo added this to the KIC v3.0.0 milestone Oct 18, 2023
@czeslavo czeslavo force-pushed the fix/credentials-basic-validation branch 2 times, most recently from 3df29b0 to 193476a Compare October 18, 2023 11:14
@czeslavo czeslavo marked this pull request as ready for review October 18, 2023 11:28
@czeslavo czeslavo requested a review from a team as a code owner October 18, 2023 11:28
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (a93d720) 77.7% compared to head (2c5a5f6) 77.7%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4887     +/-   ##
=======================================
- Coverage   77.7%   77.7%   -0.1%     
=======================================
  Files        164     164             
  Lines      18443   18459     +16     
=======================================
+ Hits       14340   14346      +6     
- Misses      3283    3290      +7     
- Partials     820     823      +3     
Files Coverage Δ
internal/admission/handler.go 59.5% <100.0%> (-1.7%) ⬇️
internal/admission/validator.go 73.5% <71.4%> (-1.5%) ⬇️

... and 8 files with indirect coverage changes

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

@czeslavo czeslavo force-pushed the fix/credentials-basic-validation branch from 193476a to ee99694 Compare October 18, 2023 11:48
pmalek
pmalek previously approved these changes Oct 18, 2023
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Since 2.12 is an LTS and this change targets 3.0 I believe this is worth back porting to 2.12.

@czeslavo czeslavo force-pushed the fix/credentials-basic-validation branch from a6e060e to 2c5a5f6 Compare October 18, 2023 17:34
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, this might have a small performance penalty due to validating more secrets (which is sort of an obvious cost for the validation) right?

Until we remove the old way - field - (which was deprecated now in favor of a label - introduced in #4825 - which should make it more efficient (checking only those with a label

@czeslavo czeslavo merged commit f7361da into main Oct 19, 2023
35 checks passed
@czeslavo czeslavo deleted the fix/credentials-basic-validation branch October 19, 2023 10:41
@github-actions
Copy link

The backport to release/2.12.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/2.12.x release/2.12.x
# Navigate to the new working tree
cd .worktrees/backport-release/2.12.x
# Create a new branch
git switch --create backport-4887-to-release/2.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f7361dae5b5322501477715826cfc292ce02cf14
# Push it to GitHub
git push --set-upstream origin backport-4887-to-release/2.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/2.12.x

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

@pmalek
Copy link
Member

pmalek commented Oct 19, 2023

@czeslavo Note that since this PR has a changelog entry the backport failed 😢

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.

2 participants