karmada

karmada copied to clipboard

What would you like to be added: Support defining variables in override policy to simplify duplicate overrider config. Now:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: nginx-op
spec:
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
  overrideRules:
    - targetCluster:
        clusterNames:
          - member2
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: skoala-dev
    - targetCluster:
        clusterNames:
          - member1
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: skoala-stage

If it has variables:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: nginx-op
spec:
  variables: 
     member1:
         value1: skoala-dev
     member2:
         value1: skoala-stage
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
  overrideRules:
    - targetCluster:
        clusterNames:
          - member2
          - member1
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: ${value1}

If you wanna add some overrider configs on member2 cluster only:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: nginx-op
spec:
  variables: 
     member1:
         value1: skoala-dev
     member2:
         value1: skoala-stage
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
  overrideRules:
    - targetCluster:
        clusterNames:
          - member2
          - member1
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: ${value1}
    - targetCluster:
        clusterNames:
          - member2
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env1: test

Why is this needed:

Decoupling values from overrider rules.

It looks very attractive. I have some questions.

1. If other fields are used in targetCluster, such as labelSelector, is the key of the variable still the cluster name?
2. Are variable values limited to strings?

Invite some people who may interested to have a look. /cc @chaunceyjiang @whitewindmills @RainbowMango

If other fields are used in targetCluster, such as labelSelector, is the key of the variable still the cluster name?

Variable's minimum scope is cluster. Using cluster name as a key is acceptable. But if there are so many clusters, defining each cluster variables is tedious same as overrider too.
The one config below that I don't like group variables by targetCluster.

  variables: 
     value1: 
      - targetCluster:
            clusterNames:
             - member1
         value:  skoala-dev
       - targetCluster:
             clusterNames:
              - member2
         value:  skoala-stage        
  overrideRules:
    - targetCluster:
          clusterNames:
           - member1
           - member2
      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: ${value1}

The other choice is that we define a ClusterGroup which could be shared to other override policy.

apiVersion: policy.karmada.io/v1alpha1
kind: ClusterGroup
metadata:
 name: group1
spec:
targetCluster:
  clusterNames:
   - member3
---
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterGroup
metadata:
 name: allclusters
spec:
targetCluster:
  clusterNames:
   - member3
   - member1
   - member2
---
apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
 name: nginx-op
spec:
 variables: 
    clusters:
        member1: 
           value1: skoala-dev
        member2: 
           value1: skoala-stage
    clusterGroups:
       group1: 
           value1: skoala-test
 resourceSelectors:
   - apiVersion: apps/v1
     kind: Deployment
     name: nginx
 overrideRules:
   - targetClusterGroup: allclusters
     overriders:
       labelsOverrider:
         - operator: add
           value:
             env: ${value1}

Are variable values limited to strings?

Supporting every variable types is best.

Amazing~~. This is a feature I've wanted to implement for a long time. Your proposal is very good and can solve the problem of duplicate variables.

Currently OverridePolicy has an obvious problem in that it can only pre-set some variables without a way to dynamically decide whether to override some values based on certain conditions.

Regarding these two issues, is it possible to consider combining them? I once tried to introduce Go template syntax into OverridePolicy but never found a good solution.

Yeah, we should discuss the value syntax.

Currently OverridePolicy has an obvious problem in that it can only pre-set some variables without a way to dynamically decide whether to override some values based on certain conditions.

The '${value1}' expression is only able to set custom variables. We can't event use value which is resource's field to set in OverridePolicy.

Excepting Go Templates, there are listing some other expression options here.

• cel
• jinja2
• other customize expression

I like cel between this options. Because it is used in k8s validation. See: https://kubernetes.io/zh-cn/docs/reference/using-api/cel/#kubernetes-url-library.

@chaunceyjiang I'm not sure how to determine the value is a CEL expression or not.

Maybe we need another field, for example:

      overriders:
        labelsOverrider:
          - operator: add
            expression:
              env: variables.value1
---
      overriders:
        labelsOverrider:
          - operator: add
            expression:
              singleton: self.metadata.name == 'singleton'? 'yes' : 'no'

Or we use {{ and }} to mark CEL expressions that similar to go templates's method , for example:

      overriders:
        labelsOverrider:
          - operator: add
            value:
              env: {{ variables.value1 }}
---
      overriders:
        labelsOverrider:
          - operator: add
            value:
              singleton: "Is it singleton? {{ self.metadata.name == 'singleton'? 'yes' : 'no' }}"

@chaunceyjiang Any idea?

CEL expression

As far as I know, there is a limitation that CEL can't handle more complex logic.

For example, it can't handle arrays with variable lengths in k8s.

it can't handle arrays with variable lengths in k8s.

Do you mean CEL cant assign a variable. Yeah, CEL is not designed to modify things.

CEL is simple so that requiring only a single line to write. The expression is a enhancement. If we still encourage people using a bunch of overrider config, not doing everything in a expression, CEL is ok.

I think If we wanna a template which be patched to resource that like the way of helm, not the method of jsonpath patch, the overrider rule in OverridePolicy is not good now. Maybe we need a template overrider?

Currently, karmada already uses Lua in ResourceInterpreterCustomization to provide flexible configuration. Could we reuse Lua?

@chaunceyjiang Can use list some lua examples in this situation? In my opinion, lua is more significantly complex than single line expression.

Lua in ResourceInterpreterCustomization

@Patrick0308 https://github.com/karmada-io/karmada/blob/master/examples/karmadactlinterpret/resourceinterpretercustomization.yaml

## CEL
      overriders:
        labelsOverrider:
          - operator: add
            expression:
              singleton: self.metadata.name == 'singleton'? 'yes' : 'no'
              value1: variables.value1
--- 
## LUA
      overriders:
        labelsOverrider:
          - operator: add
             scripts: 
                singleton: > 
                  function GetVal(obj)
                    if obj.metadata.name == 'singleton' then 
                       return 'yes'
                    else 
                       return 'no'
                  end
                value1: > 
                  function GetVal(obj, variables)
                       return variables.value1
                  end

As you can see, lua is quite complex. Is it worth using it for the flexible configuration?

I opened https://github.com/karmada-io/karmada/issues/4799 to track the expression feature.

As we discussed at the community meeting(2024-08-27), we agree to postpone this feature for a while until there is more evidence showing that this is a common requirement.

We can bring up this at anything and continue the discussion when there is a need. /close

@RainbowMango: Closing this issue.

In response to this:

As we discussed at the community meeting(2024-08-27), we agree to postpone this feature for a while until there is more evidence showing that this is a common requirement.

We can bring up this at anything and continue the discussion when there is a need. /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.