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

[PoC] feat(admission): validate secrets referred by KongPlugins #4670

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

randmonkey
Copy link
Contributor

What this PR does / why we need it:

validate Secrets if it is used in ConfigFrom of any KongPlugin.

Which issue this PR fixes:

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

@randmonkey randmonkey requested a review from a team as a code owner September 18, 2023 08:15
@randmonkey randmonkey marked this pull request as draft September 18, 2023 08:47
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Patch coverage: 34.2% and project coverage change: -0.1% ⚠️

Comparison is base (2df82cb) 67.8% compared to head (11de2be) 67.7%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4670     +/-   ##
=======================================
- Coverage   67.8%   67.7%   -0.1%     
=======================================
  Files        164     164             
  Lines      19284   19346     +62     
=======================================
+ Hits       13077   13105     +28     
- Misses      5435    5468     +33     
- Partials     772     773      +1     
Files Changed Coverage Δ
internal/controllers/reference/indexer.go 65.8% <0.0%> (-7.4%) ⬇️
internal/manager/controllerdef.go 99.4% <ø> (-0.1%) ⬇️
internal/admission/handler.go 28.9% <4.0%> (-3.9%) ⬇️
internal/admission/validator.go 37.1% <27.2%> (-0.8%) ⬇️
internal/manager/run.go 57.2% <92.8%> (+1.5%) ⬆️
...al/dataplane/sendconfig/inmemory_error_handling.go 61.5% <100.0%> (+2.6%) ⬆️
internal/manager/setup.go 75.6% <100.0%> (+0.1%) ⬆️

... and 4 files with indirect coverage changes

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

@randmonkey randmonkey force-pushed the feat/validate_secrets_when_referred_by_kongplugin branch from 7aadb9e to 11de2be Compare September 18, 2023 09:58
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.

Admission checks that cross resource boundaries are always dicey due to eventual consistency, as we've seen with consumer credentials.

We can easily perform opportunistic validation--if the Secret is available at admission time, we validate configuration, but if not, we skip and hope for the best--but guaranteed validation is harder.

Retries likely handle the common race case where you've simultaneously created a Secret and a KongPlugin referencing it. I don't think we tried that for KongConsumers when we found issues, but it should generally work--we can probably reasonably hard fail a KongPlugin if a referenced Secret doesn't show up within 5s or so, at the cost of making kubectl block.

We'd also need to handle updates to Secrets independent of their KongPlugin. AFAIK we're already watching the specific referenced Secrets only, but we'd need to build something new to trigger validation of the attached plugin on Secret update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants