camel-k icon indicating copy to clipboard operation
camel-k copied to clipboard

Move trait configuration off the CRD

Open squakez opened this issue 1 year ago • 10 comments

This is an issue to discuss the possibility to revert what I think was an unfortunate decision when we worked on https://github.com/apache/camel-k/issues/1614.

Although the idea of including trait specification into the custom resource definition was good (ie, being able to validate the schema), the reality is that this is introducing an enormous amount of changes in the API itself, when we aim the API to be as much stable as possible.

Having a look at the latest changes in the API, the changes are 80-90% (quick rough calculation) driven by changes happening in traits. Even a change in a trait documentation is changing the API spec. Although majority of the time we're managing to handle backward compatible changes, the API is still changing, requiring the need to delete and install the CRDs or the replace them. Just to mention a real problem is the case of Helm upgrade which procedure does not update the CRD, forcing the user to do that manually.

I think that, by nature, the traits are quickly evolving in order to add features, above all when it comes to Kubernetes features. For example, adding a new Kubernetes feature mostly of the time would require altering the traits (hence, altering the CRD and possibly breaking compatibility).

My proposal would be to remove them from the API and keep using in their free form. Additionally we'd remove the risk to reach the limit of a CRD which has to fit into a size limited by the platform.

squakez avatar Apr 05 '24 09:04 squakez

That is a good proposal to improve our freedom to improve the traits but we would need to provide an alternative way to offer a schema for the tooling. I know at least the Jbang Camel K plugin uses java artifact org.apache.camel.k:camel-k-crds builded from the CRDs. Maybe you have some more inputs on that @apupier.

I also wonder if this change have an impact on modeline ?

gansheer avatar Apr 11 '24 07:04 gansheer

it is currently used by the Camel Language Server (after removal from the kamel CLI). We need a way to have the list of available traits in one way or another.

apupier avatar Apr 11 '24 07:04 apupier

It's a precious feedback, thanks. If it is only required for tooling we can extract the same set of information in a separate spec file, even in the same CRD. Would it be fine?

As for the modeline, I don't think it would affect it as, instead of marshalling into an object we'd be doing it into a map.

squakez avatar Apr 11 '24 07:04 squakez

I think that, by nature, the traits are quickly evolving in order to add features, above all when it comes to Kubernetes features. For example, adding a new Kubernetes feature mostly of the time would require altering the traits (hence, altering the CRD and possibly breaking compatibility).

I do think that in relation to backward compatibility it wouldn't change much except the failure would happen at runtime, do you already know how backward compatibility and validation would be performed?

lburgazzoli avatar Apr 11 '24 08:04 lburgazzoli

I do think that in relation to backward compatibility it wouldn't change much except the failure would happen at runtime, do you already know how backward compatibility and validation would be performed?

That's correct. This is merely a validation problem. The failure would neither happen at runtime but at building time, ie, the operator would quickly set the Integration in an error state (reporting the failing trait condition as well). About compatibility this should be the same as it is today, ie, we must ensure that any change to the traits is treated correctly being able to work with any previous version of the operator when it happens in a non major release. The only difference is that we're not altering the API spec with such a frequency.

squakez avatar Apr 11 '24 08:04 squakez

I do think that in relation to backward compatibility it wouldn't change much except the failure would happen at runtime, do you already know how backward compatibility and validation would be performed?

That's correct. This is merely a validation problem. The failure would neither happen at runtime but at building time, ie, the operator would quickly set the Integration in an error state (reporting the failing trait condition as well).

For runtime I mean that is the operator that would move the resource to a failed state rather than having kubernetes validating the reaource.

About compatibility this should be the same as it is today, ie, we must ensure that any change to the traits is treated correctly being able to work with any previous version of the operator when it happens in a non major release. The only difference is that we're not altering the API spec with such a frequency.

IMHO this is not true because the APIs are changing anyway, it is only the CRD that is not getting amended so for a user POV, nothing is really changing except it won't get upfront validation, am I wrong ?

lburgazzoli avatar Apr 11 '24 08:04 lburgazzoli

IMHO this is not true because the APIs are changing anyway, it is only the CRD that is not getting amended so for a user POV, nothing is really changing except it won't get upfront validation, am I wrong ?

No, nothing would change, we'd only lose the validation vs the continuous necessity to amend the API. It is the same that we have now with the Camel YAML DSL which is part of the Integration API but is open. Notice that I'm not objecting the goodness of an upfront validation but probably we need to balance with the need to continuously change the API that it does not seems to be either a good practice.

squakez avatar Apr 11 '24 10:04 squakez

IMHO, the biggest issue here is that we are mixing up different topics. As an example, the sentence below, does not seems (to me) right or it at least is not clear enough:

I think that, by nature, the traits are quickly evolving in order to add features, above all when it comes to Kubernetes features. For example, adding a new Kubernetes feature mostly of the time would require altering the traits (hence, altering the CRD and possibly breaking compatibility).

The reason why it is not clear, is that it seems to assumes that the APIs are only composed by the CRDs but in fact, they are the result of the intersection of what is defined in the CRDs and what it is not, an example of a non described APIs are the annotations. For such reasons, a change in a trait can lead to break backward compatibility even if we remove its traits definition from the CRDs.

So it seems to me that this issue has not so much to do with the APIs per se, but more about eventually reducing the amount of work to maintain the CRDs, topic for which I cannot say much since it's been long time since I'm involved in that. If we move in this direction, we must make crystal clear that this has no impact on the way we deal we the APIs and we must ensure that every change on the traits is then handled as it it were part of the CRDs.

I would really recommend to re-phrase the issue and also include some pro/cons so we can have a better understanding of the complexity/impacts.

lburgazzoli avatar Apr 12 '24 10:04 lburgazzoli

Sorry for the confusion. API here means this package https://github.com/apache/camel-k/tree/main/pkg/apis which impacts CRD directly. Hope it clarifies, thanks.

squakez avatar Apr 12 '24 10:04 squakez

This issue has been automatically marked as stale due to 90 days of inactivity. It will be closed if no further activity occurs within 15 days. If you think that’s incorrect or the issue should never stale, please simply write any comment. Thanks for your contributions!

github-actions[bot] avatar Jul 12 '24 00:07 github-actions[bot]