-
Notifications
You must be signed in to change notification settings - Fork 593
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
Support secrets for plugin configuration #618
Conversation
Add a "configFrom" field to KongPlugin and KongClusterPlugin. This contains a secretKeyRef (and namespace, for KongClusterPlugin) pointing to a secret key containing a JSON or YAML plugin configuration. Plugins that either reference an invalid secret/key or contain both config and configFrom are rejected. Add a "configFrom" field to KongPlugin. This can contain a secretKeyRef to a secret containing the plugin config, in lieu of including it directly with the existing "config" field.
b1a52f5
to
5cde7f2
Compare
Force push after PR was to fix a vet issue. Originally:
Not sure why that's not caught by my editor's vet on save, but whatever. |
Big mess of things to squash since there were a lot of requests to keep track of and mark done. Should be all complete other than the requests that require a new type. Merge conflict is quite thorny in the test, so leaving that for the end. |
Convert the original Secret reference format to use a non-standard reference type. The namespaced variant includes the namespace at the same level as the reference. Add unit test for secretToConfiguration.
Something weird happened and 8a439d5 is somehow here despite being something from a badly-done merge that I reverted to an earlier point in the reflog. Not sure how it got pushed as it's no longer in local history, but end result looks the same as the clean merge, so whatever, it'll get squashed anyway. |
return plugin, err | ||
} | ||
|
||
func (p *Parser) secretToConfiguration(reference configurationv1.SecretValueFromSource, namespace string) (configurationv1.Configuration, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use NamespacedSecretValueFromSource
?
I cannot comment on #618 (comment) for some reason, but I think this should remain as is: we don't necessarily have a namespaced value and always have to obtain the namepsace from somewhere. In the case of a This could be flipped around, such that we instead construct a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final tweaks, this is almost ready for merge.
Rework the CRD types used for secret configuration plugins to have a struct for each level of the JSON configuration. SecretValueFromSource is now wrapped in a new ConfigSource struct. Some additional minor changes to remove outdated code.
This is hopefully good to go code-wise, but I'd still like to do a last manual practical test to confirm that I haven't missed something that testing doesn't cover properly, since this has changed quite a bit since my initial check. |
Manual test run, everything looks good there. |
What this PR does / why we need it:
Add a
configFrom
field to KongPlugin and KongClusterPlugin. This contains asecretKeyRef
(andnamespace
, for KongClusterPlugin) pointing to a secret key containing a JSON or YAML plugin configuration.Plugins that either reference an invalid secret/key, reference a key that does not contain JSON or YAML, or contain both config and configFrom are rejected.
Special notes for your reviewer:
Rebased to a single commit since our discussion on the WIP. For reference, the type split is handled with https://github.com/Kong/kubernetes-ingress-controller/compare/next...feat/plugin-conf-secret?expand=1#diff-b1c61ff32890f6c8189e0ff5be6e4cc8R1665-R1689 and the YAML/JSON dual support is at https://github.com/Kong/kubernetes-ingress-controller/compare/next...feat/plugin-conf-secret?expand=1#diff-b1c61ff32890f6c8189e0ff5be6e4cc8R1677-R1682