feat(cmd/run): autogenerated configmap for resource/config local files

[squakez]

With this PR the CLI will take care to parse a file, autogenerate to a Configmap and bind it to the Integration as it would be any other configmap. The resource will be owned by the Integration, so, deleted as soon as the Integration will be deleted. We also watch for resource file change to allow --sync option and regenerate the configmap upon a file change detection.

With this approach we can still keep the possibility for the user to provide a file and at the same time we can remove the need to include the resource content in the Integration. We can extend it to the --openapi and deprecate the ResourceSpec.

• Convert config/resource into container trait
• Convert volumes into container trait
• Deprecate Integration.Configuration in favor of related traits
• Manage --openapi to create an autogenerated configmap - relates Let user configure configmaps/secrets for kamel run --openapi option #2772
• Deprecate Integration.Resources in favor of the new logic
• Group volumes logic in a new mount trait

Closes #2320
Closes #2772

Release Note

feat(cmd/run): autogenerated configmap for resource/config local files

[lburgazzoli]

I'm not deep into the code at the moment to do a review but I'm +1 on the goal.
What we have to think about is the backward compatibility so if we remove the ResourceSpec, we may need to bump APIs to v2 ?

[squakez]

I'm not deep into the code at the moment to do a review but I'm +1 on the goal. What we have to think about is the backward compatibility so if we remove the ResourceSpec, we may need to bump APIs to v2 ?

No problem, more than the code, the goal of the draft PR is to discuss about the feature. About bumping API version, I actually don't know which would be the recommended procedures to follow.

[astefanutti]

A couple of points:

• Would the ultimate place to reference these ConfigMaps be in a new configuration trait, as discussed in [Trait] Configuration refactoring #2320, maybe as a second iteration to this?
• Should the operator watch these owned ConfigMaps for changes, and reconcile the parent Integration?
  • If yes, as a sub-case, the Integration phase could be set to Error if a referenced ConfigMap got deleted by mistake.
  • It relates to Automatically redeploy on config change #1235 and Runtime reload configmap/secret when they change #2106 possibly
• Are the generated ConfigMaps really immutable?
• Would that make sense to go the same direction for the SourceSpec field?

[astefanutti]

For the versioning, I think that it'd be acceptable to apply the current strategy, that is marking the field(s) as deprecated, let the users do the migration, and have the fields removed in later releases. Versioning CRDs is quite involved, and we would possibly have to implement a conversion Webhook, which comes at an extra operational cost.

[squakez]

Thanks for the review @astefanutti, those are very good points to discuss.

A couple of points:

* Would the ultimate place to reference these ConfigMaps be in a new _configuration_ trait, as discussed in #2320, maybe as a second iteration to this?

That is partially correct. The idea is to move configmap/secret managment there as soon as this part would be completed. So, once autogenerated, the configmaps will be managed as any user provided configmap into the configuration trait. However, the generation part must be done in the CLI, because the trait is executed by the operator and won't be able to access the local file content.

* Should the operator watch these owned ConfigMaps for changes, and reconcile the parent Integration?
  
  * If yes, as a sub-case, the Integration phase could be set to `Error` if a referenced ConfigMap got deleted by mistake.
  * It relates to #1235 and #2106 possibly

I don't think this would apply to autogenerated configmaps. In my view, these kind of configmaps are just a support to the local file content. I've worked to support sync option and have an Integration redeploy when we detect a change in the local file, that will translate in a new configmap. However, in a production environment, I expect the user to provide "normal" configmaps if there is the possibility to change the underlying content (and in that case still apply the issues reported above).

* Are the generated ConfigMaps really immutable?

They must be, IMO. The name is generated from a SHA based on the file content and nobody should alter them as they are thought as a representation of the local file.

* Would that make sense to go the same direction for the `SourceSpec` field?

Not sure if it makes sense. I think that the SourceSpec is a core part of the Integration, and we must allow anybody that want to provide an Integration with kubectl to do that without adding additional steps (ie, creating a configmap, then setting it in the Integration). A resource, instead, is already something complementary to the Integration. If we go in this direction, in fact, the user that creates directly Integration won't be allowed any longer to create a ResourceSpec with content and will need to create a configmap on his own.

Old review, more commit have been done since then.

[astefanutti]

Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this.

My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle

The idea we discussed was to go in the direction to remove any arbitrary file content from the integration. With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via kamel CLI to simplify his life. If the user is not able to run an Integration via kamel run, in fact, the feature cannot be provided.

That is my background too. My question was more, why does the file prefix leaks beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones?

The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource?

Good point, I did not consider this situation. The configmap is updated when there is a file change and --sync/--dev enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point.

What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration.

The container trait is already in charge to convert the configmaps/secret/... into mounted volumes (we discussed it shortly as well), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea.

I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the container trait.

[squakez]

Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this.

My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle

The idea we discussed was to go in the direction to remove any arbitrary file content from the integration. With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via kamel CLI to simplify his life. If the user is not able to run an Integration via kamel run, in fact, the feature cannot be provided.

That is my background too. My question was more, why does the file prefix leaks beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones?

If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap is generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource.

Hope it answer your question, I'm not entirely sure we're talking about the same thing here :)

The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource?

Good point, I did not consider this situation. The configmap is updated when there is a file change and --sync/--dev enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point.

I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can provide an additional loop to reconcile the generated Configmap not needed right after the binding step. Or we can keep it the way it is, if we consider the pattern to create an Integration with a file and later update the same integration without a file as a rare one. I don't have a strong opinion on this part.

What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration.

The container trait is already in charge to convert the configmaps/secret/... into mounted volumes (we discussed it shortly as well), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea.

I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the container trait.

The idea to move the volumes logic into a separate trait could be interesting. However, we need to attach those volumes into the corev1.Container. If we create a new trait, then it will be very dependent on the container trait, requiring to be executed after it, and it will need to get the corev1.Container spec. After all, I think it is responsibility of the container trait to manage its own volumes.

[astefanutti]

Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this.

My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle

The idea we discussed was to go in the direction to remove any arbitrary file content from the integration. With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via kamel CLI to simplify his life. If the user is not able to run an Integration via kamel run, in fact, the feature cannot be provided.

That is my background too. My question was more, why does the file prefix beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones?

If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap is generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource.

Hope it answer your question, I'm not entirely sure we're talking about the same thing here :)

I think we are talking about the same thing :) But that is general description, that I still fail to reconcile with the actual implementation :) Maybe this could be cleared if you could help me understand:

• Why the file information leaks into the container trait configuration, following the comment Syntax: [configmap|secret|file]:name[key], where name represents the local file path or the configmap/secret and my interpretation of the current implementation?
• Also why the generated ConfigMap name is hashed based on local information? Could we mimic what kubectl create configmap does?

I'm trying to understand if the consistency of the Integration configuration being created is still satisfied, meaning if user A create an integration with a local file resource, user B should still be able to operate the Integration. I hope that makes sense :)

The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource?

Good point, I did not consider this situation. The configmap is updated when there is a file change and --sync/--dev enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point.

I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can provide an additional loop to reconcile the generated Configmap not needed right after the binding step. Or we can keep it the way it is, if we consider the pattern to create an Integration with a file and later update the same integration without a file as a rare one. I don't have a strong opinion on this part.

What I'm trying to determine here, aren't we about to duplicate the garbage collection that we already have for resources generated by the operator. I get the particularity of the "local file" use case, but would there be a way to make it fit into the existing garbage collection mechanism.

What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration.

The container trait is already in charge to convert the configmaps/secret/... into mounted volumes (we discussed it shortly as well), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea.

I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the container trait.

The idea to move the volumes logic into a separate trait could be interesting. However, we need to attach those volumes into the corev1.Container. If we create a new trait, then it will be very dependent on the container trait, requiring to be executed after it, and it will need to get the corev1.Container spec. After all, I think it is responsibility of the container trait to manage its own volumes.

Yes, that is also my understanding of the technical details. I'm trying to reason at the functional level, from the end-user standpoint.

[squakez]

Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this.

My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle

The idea we discussed was to go in the direction to remove any arbitrary file content from the integration. With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via kamel CLI to simplify his life. If the user is not able to run an Integration via kamel run, in fact, the feature cannot be provided.

That is my background too. My question was more, why does the file prefix beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones?

If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap is generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource.
Hope it answer your question, I'm not entirely sure we're talking about the same thing here :)

I think we are talking about the same thing :) But that is general description, that I still fail to reconcile with the actual implementation :) Maybe this could be cleared if you could help me understand:

* Why the file information leaks into the _container_ trait configuration, following the comment ` Syntax: [configmap|secret|file]:name[key], where name represents the local file path or the configmap/secret` and my interpretation of the current implementation?

Ah, this is a mistake. I copied the description from the run command. In the trait we cannot have a file but only configmap|secret. The run is in charge to convert a file into the related autogenerated configmap, ie , -t container.resource=configmap:cm-xyz.

* Also why the generated ConfigMap name is hashed based on local information? Could we mimic what `kubectl create configmap` does?

Originally I used the file name as a configmap name. But that leads to 2 problems:

1. We are not able to provide the same file name in the namespace, ie, kamel run -n i1 -r file:test.txt and kamel run -n i2 -r file:test.txt as the second would reuse the content of the previously created configmap
2. We are not able to detect easily a change in the content, as we need to inspect the content of the configmaps when --sync is enabled

Using an hash solves both problems.

I'm trying to understand if the consistency of the Integration configuration being created is still satisfied, meaning if user A create an integration with a local file resource, user B should still be able to operate the Integration. I hope that makes sense :)

The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource?

Good point, I did not consider this situation. The configmap is updated when there is a file change and --sync/--dev enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point.

I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can provide an additional loop to reconcile the generated Configmap not needed right after the binding step. Or we can keep it the way it is, if we consider the pattern to create an Integration with a file and later update the same integration without a file as a rare one. I don't have a strong opinion on this part.

What I'm trying to determine here, aren't we about to duplicate the garbage collection that we already have for resources generated by the operator. I get the particularity of the "local file" use case, but would there be a way to make it fit into the existing garbage collection mechanism.

Yes, reason why I'd keep the generated content until the Integration lives.

What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration.

The container trait is already in charge to convert the configmaps/secret/... into mounted volumes (we discussed it shortly as well), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea.

I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the container trait.

The idea to move the volumes logic into a separate trait could be interesting. However, we need to attach those volumes into the corev1.Container. If we create a new trait, then it will be very dependent on the container trait, requiring to be executed after it, and it will need to get the corev1.Container spec. After all, I think it is responsibility of the container trait to manage its own volumes.

Yes, that is also my understanding of the technical details. I'm trying to reason at the functional level, from the end-user standpoint.

Definitely, the usage of container is driven by the fact that, technically speaking, the volumes belong to the Container. As we had the option only in the kamel run, so far, we did not expose that kind of information. If we move into a trait it becomes public, so, we may think to move into maybe a volume trait. Or maybe a resource trait.

[squakez]

I've finished another iteration of development in order to address the points discussed previously. With d85257c we're now validating the input submitted to the trait: we won't allow anything but configmap or secret. With 78e62f4 we're introducing a new trait, called mount which can be used to mount volumes from the different resources we allow.

I prefer this name to a generic resource as it declare exactly its purpose.

The last outstanding point is about garbage collecting the autogenerated configmaps that are used and later removed. Right now they will live beside the Integration, until this one is deleted. I cannot figure out how to manage differently and let them be garbage collected as soon as they are updated (unless we do a manual clean). IMO we can keep them along.

@astefanutti please, have a further look and let me know what do you think.

[astefanutti]

I've finished another iteration of development in order to address the points discussed previously. With d85257c we're now validating the input submitted to the trait: we won't allow anything but configmap or secret. With 78e62f4 we're introducing a new trait, called mount which can be used to mount volumes from the different resources we allow.

I prefer this name to a generic resource as it declare exactly its purpose.

It sounds and looks great to me 👍🏼.

The last outstanding point is about garbage collecting the autogenerated configmaps that are used and later removed. Right now they will live beside the Integration, until this one is deleted. I cannot figure out how to manage differently and let them be garbage collected as soon as they are updated (unless we do a manual clean). IMO we can keep them along.

What I'm thinking is a process like this:

1. The CLI creates the ConfigMap with the camel.apache.org/generated annotation, and does only that
2. The mount trait adds the ConfigMap to the Integration resources, by iterating over its configured ConfigMaps, and filtering those annotated with camel.apache.org/generated
3. The owner trait sets ownership on the ConfigMap OOTB
4. The gc trait associates the ConfigMap to the Integration current generation, and eventually garbages it OOTB

[squakez]

Very clever suggestion @astefanutti. I like how it looks like now. Please see 1b405ee which has captured the changes you proposed. Both mount and openapi will look for autogenerated configmaps and let the owner and gc performs their responsibility.

[astefanutti]

Thanks! I've left a couple of final comments, and it'll be good to go 👍🏼.

[astefanutti]

Great!

[squakez]

@astefanutti any last look before I can merge?

[astefanutti]

@astefanutti any last look before I can merge?

@squakez LGTM 👍🏼 Great work!