Allow specifying k8s annotations and labels for generated Deployments and other resources

[kadel]

Which area this feature is related to?
/area api
/area library

Which functionality do you think we should add?

Add a way to define annotations and labels that should be applied to the generated Kubernetes resources.

In some cases annotations or labels can be application's functional requirement. In that case it should be defined in devfile.

Why is this needed? Is your feature request related to a problem?

In some cases it is required to add annotations or labels to the resources.
For example can Istio, requires Deployments to have sidecar.istio.io/inject: "true" annotation.
Labels might be usefull ofr external visualizers, cleaners and other tools.

Related feature request for odo redhat-developer/odo#2623

[yangcao77]

we can use attributes with keys pre-defined in devfile ap, e.g. api.devfile.io/resource-annotation
we can either add the same annotation to all resources, or have different key names for different resource annotation, api.devfile.io/deployment-annotation / api.devfile.io/service-annotation etc.
@kadel @elsony any thoughts?

[kadel]

Attributes approach would work, but I'm not sure if it is a good way to approach this.

There are also fields from k8s PodSpec and ContainerSpec that we will need to expose like nodeSelector or ImagePullPolicy (#89). We will probably need to figure out some generic way of setting all this less common fields.

[yangcao77]

Attributes approach would work, but I'm not sure if it is a good way to approach this.

There are also fields from k8s PodSpec and ContainerSpec that we will need to expose like nodeSelector or ImagePullPolicy (#89). We will probably need to figure out some generic way of setting all this less common fields.

For nodeSelector or ImagePullPolicy, we will have another discussion on that.
We have discussed specify annotation in devfile in team meeting, and come up the conclusion that the annotation should be specified in component level, as different component may have different annotation required.

There are two cases we want to have more discussion with you, 1. single container case and 2. multi-container case
(The containers we talk about are inner loop, outer loop can be discard here as build is done outside)
(I will take deployment annotation as an example when we discuss the two cases, other annotations have the same issue)

1. single container:
   For example, with this sample devfile: https://github.com/yangcao77/sample-devfiles/blob/main/annotation/single-container-annotation.yaml
   the deployment annotation should be:

deployment-annotation-value1=deployment-key1
deployment-annotation-value2=deployment-key2

2. multiple containers:
   This is the case we mainly want to discuss with you, because different containers want different annotations, and may have conflicts.
   For example, with this sample devfile: https://github.com/yangcao77/sample-devfiles/blob/main/annotation/multi-containers-annotation.yaml
   for container rhel8-runtime, it wants to set deployment annotation:

deployment-annotation-boolean=false
deployment-annotation-value1=deployment-key1

for container ubi-runtime, it wants to set deployment annotation:

deployment-annotation-boolean=true
deployment-annotation-value2=deployment-key2

for annotation deployment-annotation-value1 and deployment-annotation-value2, they are independent values, and we can add both the deployment. However, one thing to notice that, for deployment annotation deployment-annotation-boolean, the two containers want to set opposite values.

The default run & build commands target ubi-runtime, and non-default run&build commands target rhel8-runtime.
In this case, if run with default commands, should set deployment-annotation-boolean=true, and if run with non-default commands, should set deployment-annotation-boolean=false.

Then library expects Odo to provide the proper annotation when generating the deployment spec, since Odo knows which commands to run, and which container should be treat as primary container in the deployment.
In this case, for instance a non-default build&run commands are selected (target component rhel8-runtime), Odo can append those annotations from those two containers with no conflicts, and choose the annotations from rhel8-runtime if there is a conflict.

deployment-annotation-boolean=false
deployment-annotation-value1=deployment-key1
deployment-annotation-value2=deployment-key2

Library can provide validation function to Odo and returns warning if there is an annotation key with conflict value defined. but odo is responsible to pass in correct resource(e.g. containers, endpoints, etc.) and annotations when calling generator functions.
@kadel Any thoughts or concerns on this?

[kadel]

This is not use case that is specfic to odo in any way.
Setting annotations and labels should be ideally part of the devfile spec.
This is something that will have to be part of all devfile implementations (currently devfile/library and DevWorkspace-operator)

The default run & build commands target ubi-runtime, and non-default run&build commands target rhel8-runtime.
In this case, if run with default commands, should set deployment-annotation-boolean=true, and if run with non-default commands, should set deployment-annotation-boolean=false.

If there is only one Deployment and devfile defines setting conflicting values for deployment labels or annotations than it should be an error state.
It doesn't make sense to modify labels or deployments just based on the command that is executed in the container. There can be a situation where you have two commands running at the same time in a different container (currently not possible in odo, but possible in Che).

Library can provide validation function to Odo and returns warning if there is an annotation key with conflict value defined. but odo is responsible to pass in correct resource(e.g. containers, endpoints, etc.) and annotations when calling generator functions.

Yes tooling using library should pass extra annotations or labels, but the labels and annotations that we are talking about in this case are required for application to run and as such library generators should populate it accordingly.

Because in some cases application won't run correctly if annotations and labels are not set properly I think that this should be part of the core devfile structure, and not just attributes.
A see attributes something that holds additional metadata, that can in some cases improve application functionality but they are not required for application to run.

[yangcao77]

As discussed in the team meeting, each container can have an annotation field. The deployment annotation will append all container's annotation, except for containers with dedicatedPod=true. Any containers with conflict annotation values will result in a validation error, unless dedicatedPod=true. Service is similar with deployment, since all containers within same pod shares same service. Ingress and Route are independent since each container has it's own ingress/route, so will only check conflict within each container.

Now proposing the annotation with struct:

type Annotation struct {
	Deployment    map[string]string
	Service    map[string]string
	Ingress    map[string]string
        Route   map[string]string
}

Example of a valid case:

components:
- name: runtime
  container:
    annotation:
      deployment:
          key1: value1
          key2: value2        
    endpoints:
    - name: http-8888
      targetPort: 8888
    image: registry.access.redhat.com/ubi8/nodejs-12:1-45
- name: runtime2
  container:
    annotation:
      deployment:
          key3: value3
    endpoints:
    - name: http-8080
      targetPort: 8080
    image: registry.access.redhat.com/ubi8/nodejs-12:1-45

Example of an invalid case:

components:
- name: runtime
  container:
    annotation:
      deployment:
          key1: value1
          key2: value2
    endpoints:
    - name: http-8888
      targetPort: 8888
    image: registry.access.redhat.com/ubi8/nodejs-12:1-45
- name: runtime2
  container:
    annotation:
      deployment:
          key1: value3
    endpoints:
    - name: http-8080
      targetPort: 8080
    image: registry.access.redhat.com/ubi8/nodejs-12:1-45

[amisevsk]

For example can Istio, requires Deployments to have sidecar.istio.io/inject: "true" annotation.
Labels might be useful for external visualizers, cleaners and other tools.

I have a question about how this will work. For example, in the DevWorkspace Operator, we're required to maintain sync between the Devfile and the deployment it represents. Adding the istio annotation breaks that assumption, as istio will have to modify the deployment to inject sidecars and could cause conflicts.

How is this handled in other applications (e.g. odo)?

[kadel]

I have a question about how this will work. For example, in the DevWorkspace Operator, we're required to maintain sync between the Devfile and the deployment it represents. Adding the istio annotation breaks that assumption, as istio will have to modify the deployment to inject sidecars and could cause conflicts.

This is quite a common problem. Odo already had the similar problem with Service Biding Operator that injects volumes and environment variables into the Deployment.

How is this handled in other applications (e.g. odo)?

The solution is to use server side apply.
With this you don't keep everything in sync, but only the fields that you care about. In our case, only fields that are defined in devfile.yaml.

[amisevsk]

Sadly, I don't think server-side apply is supported in the controller runtime, but that makes sense.

[kadel]

Sadly, I don't think server-side apply is supported in the controller runtime, but that makes sense.

I think that operators pattern was the main reason this was added to api.
I have never worked with controller runtime, but I would be surprised if this is not there. In client-go it is just Patch call with ApplyPatchType

[yangcao77]

As discussed in today's call, we changed the proposal to have container level annotation defines deployment & service annotation, and move ingress/route annotation to endpoint level.
Since currently we have all containers defined within same pod share the same service. In the future, if we want to have multiple services for a same pod, we can add service annotation to endpoint level, which takes priority to container level service annotation and overwrites it when generating the service resource spec.

type Annotation struct {
	Deployment    map[string]string
	Service    map[string]string
}

Example of a valid case:

components:
- name: runtime
  container:
    annotation:
      deployment:
          key1: value1
          key2: value2     
      service: 
          key1: value1
    image: registry.access.redhat.com/ubi8/nodejs-12:1-45
    endpoints:
    - name: http-8888
      targetPort: 8888
      annotation:
          key3: value3