Simplifying namespaces handling #732
Comments
|
@metacosm sounds good. However i think a more in depth breakdown is required. Here is my swing at it. QOSDK Runtime resolutionRuntime Namespace Resolution
Note: Ignoring the annotation is possibility, HOWEVER it’s going to be a bit confusing for the developers as it's going to create a difference beween QOSDK and JOSDK (however I am generally fine with it). Buildtime Namespace HandlingBuild time configuration only influences what gets generated into manifest via Every operator deployment should be generated with single What to Generate and When
ClusterRoleBindings / RoleBindings
Note: In order for |
|
BTW: overall I still think that the ability to specify watch namespaces per controller is introducing a horrible amount of complexity for cases which are not really used in production. ServiceAccount is specified per pod not per controller (which is an abstract concept) so in the end all controllers do have the same permissions in k8 cluster -- the restriction comes from JODK internals not from K8's RBAC. You are essentially taking an already complex system (K8's RBAC) and making it even more complicated :) |
I second this, but would ask what were the use cases for the original support? Did a user have a case where a set of resources were watched in a single namespace, but others were potentially spead across namespaces? For managed kafka there was some initial thought for the sync component to work this way - have it copy the control plane state into a single namespace, then let the managed kafka operator create namespaces as needed from there. However that was rejected as it either requires two watches / informers, or you have to treat one namespace as special in the reconciliation logic. It could be this is simply allowing users to do something that in general shouldn't be done.
If quarkus.kubernetes.namespace is set, it should use that for the subject namespace. |
Unfortunately it's not that simple.. if that is set, the |
Let me clarify what I mean. Let's ignore the per controller watching for a minute. This really boils down to only a couple of things - the default watching behavior, the deploy namespace, and what you are watching at deploy time. I'd like it to be as simple as: Such that if you use quarkus.kubernetes.deploy or quarkus.openshift.deploy it will take care of whatever needs to be manipulated in the manifests to deploy to that namespace and watch those namespaces. Otherwise the manifests would be usable for olm with the addition of target namespaces annotation handling. |
As far as I'm aware, it is possible to deploy your operator in separate pods and this scenario is supported by the OLM generator that allows you to generate separate bundles for your controllers if needed. That said, it's probably easier and a better practice to separate such controllers into separate projects, maybe.
Well, we're not doing anything RBAC related. The complexity comes from provided the proper rights to the proper bits, not extending RBACs. |
I don't think there was any particular use case but I don't think that deploying a controller in a namespace while watching a different and also having controllers that watch native kubernetes resources in other specific namespaces is not that far fetched. Of course, we could use only watch all namespaces and make things simpler. The issue, though, is about reducing as much as possible the permissions granted to controllers. If people think that's too much complexity, we can definitely revise things: I just want things to be as simple as possible but no less and I'd certainly be happy to remove complexity from the code. :) That said, we have users that dynamically change the namespaces they watch at runtime so I suspect removing that complexity is not going to happen :)
That is already was is being done and the extension even lets you know that this is what you should be doing to get a correctly generated manifest. |
Yes, that's indeed a problem. |
The only practice for that should be separate projects. Only a single jar is being produced and you could via cdi have wirings between your controllers - having them somehow in separate pods greatly confuses things.
@metacosm any thoughts on radical simplification as proposed above? |
I think I'd need to see a more detailed explanation. In any case, this kind of change probably wouldn't happen before the next major version. |
What does "Default" mean in this context? Also, not clear on the priority here, though I assume that
Yes, that is what I'm proposing as well.
I think it should always be generated, period. Are there cases where you wouldn't want the env variable to be output in the manifests?
I don't understand what's the point of this, as watching all namespaces will be the default all the time unless a runtime property is used.
To make things simpler, I think that all controllers should have their associated env variable generated all the time with the resolved value (either the default one, the one from the annotation, or the one from the generate-with-watched-namespaces property)
That's how things are currently done, though we don't use a placeholder. I debated using one and settled on not setting the namespace at all but we definitely could output a placeholder if that's easier.
This is the case as well.
I'm not sure that setting
Not sure I'm following. |
I'm assuming there would still be something like quarkus.operator-sdk.namespaces, but you would not set it directly. quarkus.operator.sdk.default-watch-single=true If true the manifests will be generated with RoleBindings - this would be expected most of the time. Only if you were not targeting usage with OLM and wanted your operator default behavior to watch multiple would you set it to false. If true quarkus.operator-sdk.namespaces (for the purposes of generating the manifests) would effectively be JOSDK_WATCH_CURRENT, otherwise JOSDK_ALL_NAMESPACES. The other properties: quarkus.operator.sdk.deploy.namespace=${quarkus.kubernetes.namespace} Would be used when quarkus.something.deploy is used. If quarkus.kubernetes.namespace is not specified, quarkus.operator.sdk.deploy.namespace would default to current. This of course would be the namespace where the operator manifest is applied. If ClusterRoleBindings are in use, then the subject namespace would use this as well. quarkus.operator.sdk.deploy.watched-namespaces hides most of the complexity. It would likely default to watch current or watch all - whatever quarkus.operator.sdk.default-watch-single implies. If you override it, then something would have to at least take responsibility for converting RoleBindings into ClusterRoleBindings should watch all be specified and the default behavior is default-watch-single=true. This will also take responsibility for setting the necessary env for QUARKUS_OPERATOR_SDK_NAMESPACES. What's not covered by doing it this way:
|
Well technically CSV allows for a list of deployments -- in practice I've never seen such operator. That said... if you generate controllers into sepparate bundles you will esentially create sepparate opperators. In such case it indeed makes sense to have them in sepparate projects/modules
True but RBAC is still invovled when doing anthing related to roles |
Lets pretend the ability to have quarkus deploy the Operator into the kluser is not even an option... You are never ever going to use that outside of testing. Ask Fabric8... there is a reason why they are not around anymore. |
Indeed.. it was form highes priority to lowest.
This was a breakdown so I really specifically talked about when this is explcitly specified during build. But since you asked for the example... Assume we have two controlles, con1 which ought to watch ns1 and con2 which ought to watch ns2. So the generated manifest would contains
What would be the point of having
replacing a value in yaml is easier than adding a new property. But it's not a big deal.
You are creating a RoleBinding in the watched namespace to a service account (subject) from a different namespace (that where operator is deployed). It has to be... It doesn't really matter if you are binding a Role or a ClusterRole.
Ignore that part... those |
I would want the quarkus folks to weigh in on that as well.
I'm not sure I follow what you are saying here. In any case let's categorize more into the deployment scenarios:
|
Agreed. That's ideally something which would be made available via Helm chart and it is IMHO out of scope for this analysis .
I'm not sure these are two diffrent cases. The problem at hand is to be able to clearly define what is generated and to have a clearly define resolution at runtime which can be predictably configured. |
Yes, we agree on the goal. I just want make sure we cut out as much as possible, so that there's less to document / consider. To sum up what I'm advocating:
If well defined / separate deployment through quarkus is desirable, we can also have:
Otherwise it would be sufficient to keep the property:
Beyond that it doesn't seem like we need anything else. Why introduce a quarkus.operator.sdk.default-watch-single rather than just recommending or defaulting to: There are several thoughts here:
However if the conditional defaulting behavior is problematic to implement, then using quarkus.operator-sdk.namespaces instead of quarkus.operator.sdk.default-watch-single could be sufficiently explained. |
|
Keep in mind that I'm advocated for not supporting any properties to individually override the controllers, nor for users to directly set quarkus.operator-sdk.namespaces. That should eliminate the confusion over the role of the annotation property - it would be used only if you want to forcibly set the behavior of the controller. Otherwise it should not be specified. |
If the user sets So while I would agree that settings at controller level are not required (at the same time I don't have particularly strong feelings against them, just that they complicate the implmenetation) I am really against the idea of having build time configuration which sets watched namespaces but can't be overrriden on runtime by any means. |
I am not suggesting that. I don't want Controller.namespaces to be seen as anything other than as a hard-coded value. If you don't want it to be hard-coded, then you wouldn't use it. The absense of the value should mean let this be managed for you based upon other configuration. |
It would be a hardcoded value which seemingly has a runtime override -- except it doesn't. Lets agree to disagree on this one. In the end the decision is on QOSDK guys. |
We had discussions on that front when we decided to remove the duplication of For development, the operator is assumed to be running locally but maybe that's a wrong assumption on my part?
👍🏼
That's the idea, yes, to be only used for testing.
We now have Helm chart generation as an option as well. |
This is not entirely accurate. Actually whether the controller is set to watch the current/single or all namespaces matters. Annotationg a controller with Annotationg a controller with Although there might be cases when you want the later, the former is usually what is desired -- even when the opperator is supposed to watch all namespaces -- due to how OLM handles roles (esentially if you want to watch all namespaces, OLM will create all role bindings for you based on operator groups). |
Again, this is why I'm advocating for only using the namespaces in the annotation if you are hard-coding that behavior. In any other case it is simply adding confusion having it set there and then elsewhere via configuration. |
I still don't follow what you want the use of the annotation to be... Previously you talked about how it should be preferred during runtime... |
If the annotation @ControllerConfiguration.namespaces parameter is set in the code, then that would be treated as a hard-coded value forcing that watching behavior. This would be the moral equivalent in the go sdk of doing something like: mgr, err := ctrl.NewManager(cfg, manager.Options{Namespace: ""}) That declaratively means you will only ever watch all namespaces. It will never use a different namespace, or multiple namespaces. That gives a very definitive purpose for using that annotation parameter. If left unset, then it means what you watch will be determined at runtime.
So referring to these as .deploy properties probably doesn't make sense. Let me try a different tact, since discussing new properties or behavioral changes isn't working. Imagine I'm a greenfield developer using QOSDK. When creating the the @ControllerConfiguration I don't see anything in the Javadocs saying what is overridable by configuration and what is not, nor the affect that the setting will have on the generated manifests. Let's say I'm lucky and didn't set the namespaces parameterl; ideally the behavior of launching the operator in the IDE, use the target/kubernetes directly (or quarkus deploy), or bundling everything up with olm should work without having to set additional stuff in the application.properties. However that is not the case. The direct usage of the manifests and olm bundling won't work. Now I have to go hunting for some property to affect the generation of the manifests. Let's say I find quarkus.operator-sdk.generate-with-watched-namespaces and set it to watch current or a target namespace. I can use the manifests, but things still are a little off - if I launch in the IDE it's still going to watch all namespaces, but if I deploy using the manifests it's watching the single namespace. So I either accept that nuance or I have to override quarkus.operator-sdk.namespaces to match quarkus.operator-sdk.generate-with-watched-namespaces. What I'm looking for is more information upfront or principled defaults that will simply work for the most basic scenario. If out-of-the box is just too messy given legacy concerns, then we'll need more steps and explanation shown in the olm deployment docs to call out exactly what and why you'll set via application.properties. |
Ok, got it. The thing is that GO then doesn't expose a standard environment variable which is supposed to this behvaiour. However I get what you mean and I wouldn't object too much if this was the case -- provied I can ommit the annotation and still generate the kubernetes.yaml (and thus consecutively the CSV) as if the annotation was present there with "whatch current" as value -- the reason being described in my previous comment abotu |
|
The way I see things is, and the way they've evolved is that the controller annotation is historically the only way to change which namespaces are watched in JOSDK. QOSDK changes things because then you can override the annotation value with properties or env variables (if the extension lets you, of course). For a greenfield developer, the annotation is a quick way (I'd argue) to change the settings while running in dev mode: you change the annotation value, QOSDK picks up the change, re-configures things and restart the operator and you're off to the races. Historically, QOSDK has been developed for developer joy while writing the operator. Of course, then reality occurs and has to mess everything up! 😁 If you do a good job at developing your operator, at some point you'll want to deploy it to a cluster. That's where the kube / OLM / Helm manifests come into play. The question thus becomes how to make that transition from an operator running in dev mode locally to a cluster-deployed operator as frictionless as possible. If you start with the idea that the annotation is the main way to change the namespaces during development (and again, if you use pure JOSDK, it's the only way to do it), then you need to be able to override that value at runtime so that you can still develop things locally as you want but change things dynamically after your operator is deployed. The alternative being that you otherwise need to ensure that the annotation values are either removed before you deploy or that they match exactly the deployment environment (which is not realistic). One valid point that could be made is that namespaces are not likely to change much after the topology of the operator has been established so maybe the "emphasis" of making them easy to change during development is ill-founded. That said, I think it's still useful to be able to easily experiment with this aspect during the initial stages of the operator development. We could argue on the value of being able to set different individual namespaces for different controllers. I don't have enough visibility on users' projects and environments to be able to tell either way. Historically, people have had the opportunity to set things this way and unless there's overwhelming evidence that it's not needed, I'd rather not remove such a feature. So these are the constraints I have to work with and which led to propose the solution outline in this issue's description. Of note, this solution is not completely implementable at this point because removing the propagation of build time properties to runtime prevents Quarkus from being able to properly match environment variable names to the associated property. This will be addressed in a future Quarkus release. Alternatively, I am also considering #733 to address this issue but there is also another issue there with Quarkus that prevents the automated relocation of the |
|
@metacosm thanks for the reasoning. One question if I may.. When you say that the annotation is the only way to set the watched namespaces in vanilla JOSDK, I suppose you mean the only declarative way, right? I'm having issues imagining how would be such operator usable if you could really only hardcode this without the ability to provide runtime configuration. |
JOSDK doesn't address deployment at all. How people deal with watched namespaces at runtime is left to the user. It's, however, feasible to programmatically change the watched namespaces at runtime but there is no declarative way to do it outside of the annotation, indeed. |
This I think should not be supported, I remember a discussion, to have only helm that covers all the use cases, and the generated manifests to |
|
Regarding the @ControllerConfiguration.namespaces , to be honest in v5 of JOSDK would remove this property from the annotation. Would still keep the functionality though. I've seens some operators where different controllers had different scope in terms of namespaces, this is not something very extreme or unusual (I think). But the confusing part is really the annotation. What effectively does it overrides some default, what we can later again override. |
It needs to be supported by OLM also.
That could be fine as well, execpt there's no built-in notion of what the deployment namespace is (unless quarkuse.kubernetes.namespace is set) for the operator deployment so the clusterrolebindings subject namespaces are missing.
That is correct, however it's quite clear to the user if they do choose to use an env variable how it will be picked up. Without the clarity of saying the annotation parameter is a fixed value (or removing it entirely as @csviri is open to doing), it's quite odd in how it is handled. Imagine explaining it like it's a quarkus an injected config property: @ConfigProperty(name = "quarkus.operator-sdk.controllers.NAME.namespaces", defaultValue="something") where there would be a natural property hierarchy: quarkus.operator.namespaces=JOSDK_ALL_NAMESPACES However that's not quite correct - the default value takes precedence over quarkus.operator.namespaces and also influences the manifest generation. Both of those would be surprising to a quarkus developer. |
|
IMO the precedence order would be clear if (in priority order):
the point is that with 2. we always override 3. (but also with 1. the 2. - maybe just some) |
Well.. after my change the default wouldn't be used here. due to
This is exactly the order I was proposing :) |
This isn't what was discussed. The decision was that another property was introduced to generate manifests with specific namespaces ( |
One thing we should keep in mind. It might be a good idea to remove the dependency between what is generated for |
Current issues
There are still some issues regarding how namespaces are handled. To wit:
generate-with-watched-namespacesproperty but have annotations on your controllers that contradict what got generated, and you don't explicitly override the namespaces at runtime, then your controllers might not end up watching the namespaces you intend them to and/or not have the proper permissions to do soquarkus.operator-sdk.namespaceneed to be applied or not (basically, it is not straightforward to account for all possible cases and detect whether a runtime configuration occurred due to explicit user ask or because of propagation of build-time values).Proposal to address these issues
/cc @jcechace @shawkins @csviri
The text was updated successfully, but these errors were encountered: