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

Let admission webhook check KongPlugins with secret configuration #1036

Merged
merged 2 commits into from
Feb 18, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Feb 17, 2021

What this PR does / why we need it:
We don't currently support pulling configuration a KongPlugin's ConfigFrom configuration in the admission webhook. Some plugins have fields that must be populated and have no default, e.g. issuer for the OpenID Connect plugin. The only current remedy is to disable validation entirely, which isn't ideal.

This PR:

  • Provides access to the store in the admission webhook, so that it can retrieve content from resources (in this case, a Secret) referenced by proposed resource.
  • Validates that Secrets referenced by ConfigFrom exist, and uses the configuration in them when validating the plugin schema.
  • Checks that a KongPlugin doesn't provide both Config and ConfigFrom.
  • Uses the configurationv1 import convention we use elsewhere for github.com/kong/kubernetes-ingress-controller/pkg/apis/configuration/v1 in the admission webhook.

Which issue this PR fixes:
fixes #1023

Special notes for your reviewer:
Reviewed validation unit tests. We currently lack units for most of the webhook checks because most of them require a Kong instance's /schemas/<whatever>/validate endpoint or other admin API access (e.g. GET /consumers/<username> to check whether a consumer would be a duplicate). We'd need integration tests to validate these, but our current framework doesn't allow us to easily test just whether we can upload configuration, since it's based around testing proxy behavior after. We should consider how to work such validation webhook tests into the new Go integration test system.

@rainest rainest requested a review from a team February 17, 2021 01:31
@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #1036 (48f3795) into main (b6fd524) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1036      +/-   ##
==========================================
- Coverage   49.65%   49.42%   -0.24%     
==========================================
  Files          32       32              
  Lines        3198     3207       +9     
==========================================
- Hits         1588     1585       -3     
- Misses       1481     1492      +11     
- Partials      129      130       +1     
Impacted Files Coverage Δ
cli/ingress-controller/main.go 0.23% <0.00%> (-0.01%) ⬇️
internal/admission/validator.go 26.31% <0.00%> (-4.30%) ⬇️
pkg/parser/parser.go 83.92% <0.00%> (-1.34%) ⬇️

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 b6fd524...48f3795. Read the comment docs.

Travis Raines added 2 commits February 16, 2021 17:31
For KongPlugins that set ConfigFrom:
- Verify that the KongPlugin does not also set Config.
- Use the Secret value indicated by ConfigFrom when validating plugin
  configuration.

Fix #1023
Use configurationv1 for the controller configuration v1 package import,
following convention elsewhere.
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.

No explicit blockers, but a lack of available testing as you mentioned in the description leaves me with some concerns.

How would you feel about putting together a written (manual) test plan for the admission controller which covers base functionality and the fix as a hack? Let me know your thoughts? 🤔

Other thoughts: The railgun prototype brings testing utilities that may enable us to test this more simply going forward, do you think that's potentially helpful or something we could use for this iteration?

@rainest
Copy link
Contributor Author

rainest commented Feb 17, 2021

Manual testing plan is:

  1. Deploy with the standard 1.1.1 image.
  2. Attempt to apply an OIDC KongPlugin with configFrom (without creating the referenced Secret). The admission webhook rejects it because its configuration lacks issuer, and doesn't care that the Secret isn't actually available.
  3. Redeploy using an image that contains this change.
  4. Attempt to apply the plugin again. The webhook rejects it because the Secret isn't available.
  5. Create the Secret, apply plugin again. The webhook accepts it.
  6. Attempt to apply a variant of the plugin with both configFrom and config. It's rejected because we don't allow you to use both.

Sample output: test.txt

I think the additional kubebuilder test scaffolding can help us here, but am not entirely sure, since the book documentation on it is a bit light. We'd still need a live Kong instance to test against, and I'm not sure if it supports that or just more thorough interactions with a mock K8S client and API server.

@rainest rainest merged commit 1033dee into main Feb 18, 2021
@rainest rainest deleted the fix/validate-secret-config branch February 18, 2021 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admission controller cannot read secret plugin configuration
2 participants