Kuadrant CR policy support proposal #6
Conversation
alexsnaps
requested review from
didierofrivia,
eguzki,
guicassolato and
maleck13
| both policies API will be exposed and the reconcilers will still be registered. Creating a policy that | ||
| is disabled would be denied. | ||
|
|
||
| A disabled policy can be enabled by changing the value of the `policy` key. Flipping the value can |
There was a problem hiding this comment.
how would we enforce this?
|
|
||
| ### In an OSSM cluster | ||
|
|
||
| The deployment of `Kuadrant` should fail if `authentication` isn't explicitly disabled. |
There was a problem hiding this comment.
should we make this explicit in this proposal? I assume we will want it to succeed in the future?
There was a problem hiding this comment.
Not sure this is required after Kuadrant/kuadrant-operator#111
There was a problem hiding this comment.
@eguzki's right. This section can now be removed from the RFC.
| While it would be good to be able to enable the support for a policy while such policies already | ||
| exist, there is much to consider as to what this would entail - here are a few things to consider: | ||
|
|
||
| - can we apply all these policies? |
There was a problem hiding this comment.
I feel we have this issue anyway? If I remove all the policies and recreate post enabling we will need to be able to reconcile them
| exist, there is much to consider as to what this would entail - here are a few things to consider: | ||
|
|
||
| - can we apply all these policies? | ||
| - what ordering should be considered to apply them? |
There was a problem hiding this comment.
I am not sure the controller should be concerned about the ordering, unless I have missed something. Is it about conflict? To resolve a conflict do we not need to fall back to the oldest policy wins anyway from policy attachment?
| - what would disabling `authentication` for a cluster, while existing routes are currently protected by these? | ||
|
|
||
| Requiring to first remove all policies and then only safely disable or enable the support for that | ||
| kind of policy seems reasonable at this stage and can always be added later, which clearly specified |
There was a problem hiding this comment.
Agree so the questions above may still be valid for discussion but I think as a starting point this is simple set of states to deal with initially but also agree that we shouldn't rule this out in the future as likely the flow a user will follow here is kubectl delete -f AuthPolicy.yaml enable auth support kubectl create -f AuthPolicy.yaml which from an end user perspective will feel entirely redundant.
| We could disable `authentication` on a heuristic base approach: running within OSSM? Disable | ||
| `authentication`. The user experience seems less than desirable and confusing as best, as in future | ||
| version this would probably change. That being said we're still pre-v1. We'd also need to find a way | ||
| to know very early on what environment we are being deployed in. |
There was a problem hiding this comment.
Hmm. So this for me comes to the fundamental question. Do we want to support these different system states as feature or do we want to say in v0.x.y authpolicy is not supported in a particular environment add the webhook mentioned earlier during installation and reject any AuthPolicies in that environment.
It also raises an interesting question for me that if we want to offer individual features that perhaps RateLimitPolicy and AuthPolicy should become a part of those components (IE Limitador/Limitador operator and Authorino/Authorino operator) should support AuthPolicy and RateLimitPolicy respectively. Have we given that approach any consideration? This would mean enabling in the kuadrant operator would install or uninstall the operator and in turn would install or remove the CRD, if memory serves removing the CRD will remove any of those resource types also. Doing this way would mean that we would not need the webhook as the CRD is either their or not.
cc @eguzki @guicassolato
There was a problem hiding this comment.
I would not follow the approach of adding/removing CRDs dynamically based on some Kuadrant CR. CRD's are global cluster objects and that raises potential conflicts. What if some user in the cluster is using the Limitador operator to limit their service by their own (not using kuadrant)? Or how does this idea fit when kuadrant supports multiple kuadrant data plane instances?
Regarding having AuthPolicy part of the Authorino Operator (or RateLimitPolicy part of the Limitador Operator) is interesting to consider. I do not see now a hard stopper.
There was a problem hiding this comment.
I don't see Kuadrant's RLP and KAP as definitions of Limitador Operator and Authorino Operator respectively. These CRDs of Kuadrant are committed to GW-API's policy attachment value proposition, which the aforementioned operators aren't. In fact, I'd see such as a violation of the current dependency that exists between Kuadrant and its functional components, where the lower dependencies (Limitador and Authorino) currently can exist without knowing anything about Kuadrant or its purpose.
Furthermore and especially within the scope of this particular RFC, I agree with @eguzki's point of CRDs being global definitions in the cluster and therefore their installation not the way to control whether a particular workload in a particular namespace shall be deployed or not.
For both of these reasons, I believe that installing the CRD is one thing, while choosing which functional component to deploy is another separate one. Therefore, RLP and KAP CRDs should continue living in the Kuadrant Operator IMO.
| # Unresolved questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| - Is this really what we want? |
There was a problem hiding this comment.
I think this is the key question.
There was a problem hiding this comment.
I think we want this. However we might want other features before this one to make users/consumers adopt Kuadrant. I know I do not know :)
| # Prior art | ||
| [prior-art]: #prior-art | ||
|
|
||
| While I couldn't find, probably because of lacking knowledge about similar use-cases, anything that |
There was a problem hiding this comment.
I think in general not enabling an API in a cluster or disabling an API is done by making the CRD available or deleting it
| disable the support for a policy by specifying the following in the `spec` of the resource: | ||
|
|
||
| ```yaml | ||
| authentication: |
There was a problem hiding this comment.
| authentication: | |
| auth: |
so it leaves room for 'authorization' in the meaning of the flag as well.
| [rationale-and-alternatives]: #rationale-and-alternatives | ||
|
|
||
| Possibly the gain of not running unnecessary services on the cluster isn't big enough for us to | ||
| justify adding this complexity. Yet, as of today, we need a way to disable `authentication` in the |
There was a problem hiding this comment.
No longer true. See https://github.com/Kuadrant/architecture/pull/6/files#r1038517609
|
@Kuadrant/engineering Should we shelve this? Close it entirely and forget about the idea given where we are now? |
No description provided.