Support KeyRef support for plugin configuration

[esatterwhite]

Is there an existing issue for this?

• I have searched the existing issues

Problem Statement

Currently the only way to configure plugins using the KongPlugin or KongClusterPlugin resources is static.
While it is possible to reference a Secret to pull configuration from, the entire configuration for the plugin must be defined in the secret. This makes it difficult to pull data from multiple sources and generates a lot of duplication of value.

It would be highly beneficial to support reading individual values from ConfigMap and/or Secret resources

Proposed Solution

Similar to the way values are pulled in as environment variables in a Pod env spec,

kind: KongPlugin
plugin: my-plugin
metadata:
  name: my-plugin-instance
configFrom:
  - name: <String>
    [value, valueFrom]:
       [configMapKeyRef, secretMapKeyRef]:
         name: <String>
         namespace: <String>
         key: <String> 
         optional: <Boolean>
         default: <String>

value - is a means to specify a literal value which would support similar functionality to the way config does now
valueFrom - is a way to read dynamic values from the cluster

the namespace can be optional and default to the namespace the plugins are defined in. specifying a value would read configmaps from the specified namespace

as a migration path, the literal configuration of existing plugins could be translated to literal values in the new format

config:
  foo: 1

becomes

configFrom:
  - name: foo
    value: "1"

So it would makes sense that the schema allows for only one of config or configFrom. Both may not make sense.
I'm aware that configFrom is used to read the single secret value, so it can be a different name if that is more suitable

Additional information

Currently plugin configuration is updated when the configuration changes. The introduction of this change would inherently change that as the plugin resource itself would not be changing, but the values on the cluster could.

It may be necessary to re-eval plugins periodically or use additional event watchers based on the configuration to maintain the hot reload behavior that the controller exhibits today.

Points of complexity

• Network errors when communicating with the API server may cause edge cases and It may be necessary to allow end users to specify how that is handled. - is it a full stop error? or should processing continue and/or use default values?

• The list style of name/value makes it difficult to support complex nested objects. support for both forms w/ a clearly defined hierarchy maybe needed to facilitate both. Although it is still valid to assign an object/ array to the value property

 - name: foo
   value:
     test:
       valid: true

Acceptance Criteria

• As a user I can read individual plugin configuration values from configmap keys in any namespace
• As a user I can read individual plugin configuration values from secret keys in any namespace, and have them automatically decoded
• As a user I expect plugin configuration to be kept up to date with the underlying values defined by the associated cluster resources
• As a user I expect to be able to specify literal values in a similar fashion to specifying dynamic cluster values
• As a user I expect to be able to specify that dynamic values to be optional when desired, but required by default
• As a user I expect non optional dynamic values to fail to load when the specified key, or resource is missing from the cluster
• As a user I expect optional dynamic values to support default values to be utilized when the key or resource is missing from the cluster

[rainest]

@scseanchow this was previously raised internally as FTI-2476. Copying my initial response from chat:

This hamstrung by it being a breaking change (it'd replace the old config and configFrom or :face_vomiting: add a third competing configKeyValue option alongside the existing two) and furthermore a breaking change that doesn't seem to fit well into how you're supposed to do CRD versioning. AFAICT k8s docs basically say "crd versions need to be both forwards and backwards compatible with one another", and there doesn't seem to be any guidance for when that isn't easily possible (you'd need to do some fancy stuff with secret creation and IIRC it's not immediately clear how you'd properly handle it)

[esatterwhite]

All paths seem less than Ideal, but the current iteration is rather difficult to work with and also less than ideal.

If we have to deal with some pain to get to a better state, We're willing to do that. If there isn't a full stop opposition to the idea in general, Can we settle on which path to take?

[esatterwhite]

Since currently the secret effectively has to contain the entire config, Wouldn't it be enough to just translate something that has a configFrom to be a list of secret key refs that map to the keys in the secret?

configFrom:
  name: <secret-key>
  valueFrom:
    secretKeyref:
      - name: <secret-name>
        key: <secret-key>

As far as supporting forward and backward compatibility for a single version - that doesn't seem too bad.

[rainest]

Unsure, we hadn't looked into the specifics of conversion handling--this hadn't been scheduled yet, so we hadn't tried to write any POC code yet. I do think it'll hit a problem in one direction: if you have the new env-like format and reference multiple Secrets, there's no way to convert that back to a single Secret.

[esatterwhite]

I see, thats fair. But is that an acceptable concession if its documented and communicated that is the case?

[rainest]

It's unknown. The current state of this is that we know we'd like to do it, but that it's not trivial, and we're waiting for the product owners to prioritize it so that we allocate time to develop a POC and review options in depth.

[esatterwhite]

Its something we'd like to see sooner rather that later. Is there any opposition outside contribution on this?
We're happy to collaborate on design, direction and do the leg work on the POC code to get through the major obstacles

[shaneutt]

Its something we'd like to see sooner rather that later. Is there any opposition outside contribution on this? We're happy to collaborate on design, direction and do the leg work on the POC code to get through the major obstacles

For something like this we would ask for a KEP to help build the consensus around the initiative as well as documenting implementation intent. You can find examples in the keps/ directory and there's a README there that provides some background and explanation. I would suggest starting with a provisional KEP that includes only the summary, motivation, goals, and non-goals for the first iteration (PR) so that we can build consensus on the "what" and "why", but wait on the "how".

Also please keep in mind that while we will try to accommodate, there is a real chance we may have to eventually consider the KEP declined. If we do that however we commit to providing a very informative reasoning as to why.

If you have any questions about the KEP process please don't hesitate to ask in a discussion on this repository.

[shaneutt]

Thanks @esatterwhite for creating a KEP for this, which is now merged in a provisional state for us to continue iterating on:

https://github.com/Kong/kubernetes-ingress-controller/blob/main/keps/0010-dynamic-plugin-configuration.md

At this time I've discussed this effort and the KEP with other maintainers and we're currently leaning towards accepting this proposal and getting it on our roadmap. Nothing confirmed yet, but we'll use this issue to communicate on acceptance and milestones so that we can provide some insights as to if and when this would be available.

If you have any questions or concerns in the meantime, please feel free to reach out in the comments here.

[esatterwhite]

Is this something Kong is looking to implement internally or is outside contribution welcome?

[shaneutt]

Is this something Kong is looking to implement internally or is outside contribution welcome?

We're open to outside contributions to the KEP to move it towards implementable state, after which we would consider it ready for implementation, but for the actual implementation we're not yet certain as there's significant complexity and there's timing considerations to figure out. In the KEP so far we have the "what" and "why" pretty well established. What is your appetite for adding the "how" (technical implementation details, graduation criteria, e.t.c.)? (we the maintainers would intend to further fill out the KEP otherwise though, so things will still progress even if you want to be hands off from here)

[esatterwhite]

Happy to help push this forward anyway possible.
what kinds of specifics are you looking for?
And of the possible problems/solutions we talked about in passing on the KEP, which of them sounded the most promising, and which things sounded like areas of concern?

[esatterwhite]

and there's timing considerations to figure out

What are the timing considerations?

[shaneutt]

what kinds of specifics are you looking for?

At this point filling out the implementation details and then making sure we have consensus between the maintainers and the community (just you so far) about those implementation details.

And of the possible problems/solutions we talked about in passing on the KEP, which of them sounded the most promising, and which things sounded like areas of concern?

Originally I was thinking about this being a v2, but after consideration and thinking about the fact that I don't think we're ready to add any other major features that would help push us to a v2 so I'm leaning towards v1. However if we do it in v1 it needs to be done in a backwards compatible kind of way which you noted in the drawbacks we're not clear on whether that's fully doable yet. So the next major area of concern would appear to be for us to navigate that and come up with a solution that is either 1) on v1 but backwards compat 2) on v2 with compatibility via conversion.

What are the timing considerations?

This actually adds context to the "areas of concern" question above: we (the maintainers) have been considering this one something to do for our v3.0.0 release, which is still quite a ways out at this point as we already have a project roadmap that we're on with several high priorities (including the new Kong Gateway Operator). It's not out of the question for this to be done via external contributions so that it can theoretically get included sooner, but we want to avoid mis-coordination. Getting the KEP into an implementable state (with consensus among maintainers and community (just you for the moment)) with a plan that's as least disruptive, and as minimally complex as possible would make it less frictional to get this out in a v2.x.x release.

cc @Kong/team-k8s

[esatterwhite]

As for the implementation details, you're looking for a high level how / design, yes? not how it would theoretically implemented in code?

I'm curious as to what the common pattern for handling version translations like we are talking about is?

What is the timeline to v3? 6mo? 12mo? 18mo?

[shaneutt]

As for the implementation details, you're looking for a high level how / design, yes? not how it would theoretically implemented in code?

Ideally we would start with the high, and even yes some low level details. Maybe some code for examples.

I'm curious as to what the common pattern for handling version translations like we are talking about is?

The relevant documentation is https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/. It's usually desired to not have to implement a conversion webhook (just because these can become a bit high maintenance, and esoteric).

What is the timeline to v3? 6mo? 12mo? 18mo?

We don't have a timeline for it at the moment, and I'm not in a position to provide a good guess, all I can say is I feel a great deal of confidence next year would be the earliest. If your need in terms of timing is much greater than that, then we should probably be digging into the path of feasibility of adding this in the v1 api.