karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

how to add/replace container env in OverridePolicy and keep other envs alive? #3362

Open mkloveyy opened 1 year ago

mkloveyy commented 1 year ago

This is part of my deployment:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
      - env:
        - name: CPUS
          value: "1"

This is part of my OverridePolicy:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
spec:
  overrideRules:
  - targetCluster:
        clusterNames:
          - sharb-x
      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env
            operator: add
            value:
              - name: TEST
                value: captain-test

I wish my cluster sharb-x deployment may be:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
      - env:
        - name: CPUS
          value: "1"
        - name: TEST
          value: captain-test

But actual is:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
      - env:
        - name: TEST
          value: captain-test

CPUS env is missing.

So how can I add or replace container env in OverridePolicy and keep other envs alive? Thanks!

Environment:

RainbowMango commented 1 year ago

@chaunceyjiang Could you please help? How can we add an item to a slice?

I'm thinking if we need another overrider dedicated for container env, just like what we did for ImageOverrider .

XiShanYongYe-Chang commented 1 year ago

This is more like a issue of json-patch using, how about testing with the below op:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
spec:
  overrideRules:
  - targetCluster:
        clusterNames:
          - sharb-x
      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/0
            operator: add
            value:
              - name: TEST
                value: captain-test
Poor12 commented 1 year ago

This is more like a issue of json-patch using, how about testing with the below op:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
spec:
  overrideRules:
  - targetCluster:
        clusterNames:
          - sharb-x
      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/0
            operator: add
            value:
              - name: TEST
                value: captain-test

Yes. Another optional overridepolicy is like:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
spec:
  overrideRules:
  - targetCluster:
        clusterNames:
          - sharb-x
      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/-
            operator: add
            value:
              - name: TEST
                value: captain-test
chaunceyjiang commented 1 year ago

I'm thinking if we need another overrider dedicated for container env, just like what we did for ImageOverrider .

+1

chaunceyjiang commented 1 year ago

/assign

RainbowMango commented 1 year ago

@chaunceyjiang Can you help add an agenda to the community meeting? Let's gather some feedback there, I'm not sure if this(override envs) is a common case.

@mkloveyy Can you confirm if the suggestion shared by @XiShanYongYe-Chang and @Poor12 works for you?

mkloveyy commented 1 year ago

@RainbowMango Thank for help. I've tried both two suggestions but still didn't work and got same response:

admission webhook xxx denied the request: xxxx Spec: v1.PodSpec.Containers: []v1.Container: v1.Container.Env: []v1.EnvVar: readObjectStart: expect { or n, but found [, error found in #10 byte of ...|],"env":[[{"name":"T|..., bigger context ...|=128","-threadCount=1","-waitSeconds=10"],"env":[[{"name":"TEST","value":"captain-test"}]

And I was wondering if env can be replaced?

mkloveyy commented 1 year ago

@chaunceyjiang Could you please help? How can we add an item to a slice?

I'm thinking if we need another overrider dedicated for container env, just like what we did for ImageOverrider .

Agreed. Maybe an EnvOverrider which can support add/replace/delete env as env operation is a bit complicated.

chaunceyjiang commented 1 year ago

@chaunceyjiang Can you help add an agenda to the community meeting? Let's gather some feedback there

ok.

Agreed. Maybe an EnvOverrider which can support add/replace/delete env as env operation is a bit complicated.

Yes, if users directly use json-patch to modify the env, it is a very complex operation. And it's very easy to make mistakes.

RainbowMango commented 1 year ago

Thank for help. I've tried both two suggestions but still didn't work and got same response:

@XiShanYongYe-Chang @Poor12 Could you please take a look? I'm curious about how complex it is when using plaintext overrider.

chaunceyjiang commented 1 year ago

Thank for help. I've tried both two suggestions but still didn't work and got same response:

      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/0
            operator: add
            value:
              name: TEST
              value: captain-test

You can try this.

image

mkloveyy commented 1 year ago

Thank for help. I've tried both two suggestions but still didn't work and got same response:

      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/0
            operator: add
            value:
              name: TEST
              value: captain-test

You can try this.

image

@chaunceyjiang Still not work. I just override all envs temporarily now for use. @RainbowMango I wonder if can support an EnvOverrider in future as its very useful for users like me. Thx!

RainbowMango commented 1 year ago

I wonder if can support an EnvOverrider in future as its very useful for users like me. Thx!

@mkloveyy we are going to talk about this feature at the community meeting next week, will you present the meeting then? We are glad to hear about your use case there.

mkloveyy commented 1 year ago

@RainbowMango Of course. Its my honor to join the meeting.

mkloveyy commented 1 year ago

@RainbowMango Hello, I've found correct ways to use json patch to add/replace/remove container env:

- path: /spec/template/spec/containers/0/env/-
  operator: add
  value:
    name: zone
    value: a
- path: /spec/template/spec/containers/0/env/11
  operator: replace
  value: 
    name: K8S_CLUSTER
    value: rb-a
- path: /spec/template/spec/containers/0/env/0
  operator: remove

However it's still not very easy as we must know its index in env list. So an EnvOverrider may still be helpful I think. Thanks for you help!

RainbowMango commented 1 year ago

Congrats! And thanks for sharing the result.

However it's still not very easy as we must know its index in env list.

Yeah, I agree. When people want to modify(replace) an item in a slice, we must explicitly specify the index, it can be very difficult to maintain and configure.

RainbowMango commented 1 year ago

Speaking of the EnvOverrider, we just talked about it at the community meeting today.

Personally, I tend to introduce an EnvOverrider(placeholder name), the only concern is we don't know if it is a common use case, so, I wonder could you share your use case?(Why your application reply on different envs on different clusters?)

In addition, are your using Karmada in production or evaluating Karmada now?

mkloveyy commented 1 year ago

Due to historical reasons, we stored zone-related information in environment variables and currently we are investigating how to migrate from Kubefed to Karmada. Now we prepare to use OverridePolicy to change zone-related envs in member cluster objects. That's the case.

part of our demo op as below:

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: app-demo
  namespace: demo-ns
spec:
  resourceSelectors:
    - apiVersion: xxxxxxx
       kind: OurCustomResource
       name: demo
       namespace: demo-ns
  overrideRules:
    - targetCluster:
        clusterNames:
          - zone-x
      overriders:
          - path: /spec/template/spec/containers/0/env/11
            operator: replace
            value: 
              name: K8S_CLUSTER
              value: zone-x
          - path: /spec/template/spec/containers/0/env/12
            operator: replace
            value: 
              name: AZ
              value: X
          - path: /spec/template/spec/containers/0/env/13
            operator: replace
            value: 
              name: zone
              value: ZONE-X
    - targetCluster:
        clusterNames:
          - zone-z
      overriders:
        plaintext:
          - path: /spec/template/spec/containers/0/env/11
            operator: replace
            value: 
              name: K8S_CLUSTER
              value: zone-z
          - path: /spec/template/spec/containers/0/env/12
            operator: replace
            value: 
              name: AZ
              value: Z
          - path: /spec/template/spec/containers/0/env/13
            operator: replace
            value: 
              name: zone
              value: ZONE-Z

By the way,the object to be migrated is our own customized CRD.

RainbowMango commented 1 year ago

Due to historical reasons, we stored zone-related information in environment variables and currently we are investigating how to migrate from Kubefed to Karmada.

As far as I know, Kubefed supports the overrides by plaintext, Aren't you using this feature?

mkloveyy commented 1 year ago

As far as I know, Kubefed supports the overrides by plaintext, Aren't you using this feature?

Sry, maybe my expression was ambiguous. Migrating from kubefed to Karmada is our roadmap. So now we need to use op to implement zone-related information in envs. In kubefed, we generate whole federated object include envs. So not face to the case of overriding envs.

RainbowMango commented 1 year ago

OK, thanks. I got it.

I'll try to get some feedback from Karmada end users and get back to this soon(please ping me if I don't back in more than a week :))

mkloveyy commented 1 year ago

Ok,thanks a lot.

RainbowMango commented 1 year ago

Hi @mkloveyy @chaunceyjiang I talked to @lfbear who has a lot of experience with it. He said there are quite a few scenarios that need to use env as startup/runtime environments. So it sounds good to me to introduce an env overrider.

Who can help with it? @mkloveyy Please let me know if you want help.

chaunceyjiang commented 1 year ago

/assign

mkloveyy commented 1 year ago

Hi @mkloveyy @chaunceyjiang I talked to @lfbear who has a lot of experience with it. He said there are quite a few scenarios that need to use env as startup/runtime environments. So it sounds good to me to introduce an env overrider.

Who can help with it? @mkloveyy Please let me know if you want help.

Sure. What can I do for you?

RainbowMango commented 1 year ago

Thanks @mkloveyy. I think you can try to update the PlaintextOverrider document, based on your experience on above.

@chaunceyjiang Can you help open another issue to track the iteration tasks of env overrider? @mkloveyy want to be part of it. By the way, this issue is for the solution of how to vary env content. We can update the document and then close it, new feature should be tracked by another.

chaunceyjiang commented 1 year ago

@chaunceyjiang Can you help open another issue to track the iteration tasks of env overrider? @mkloveyy want to be part of it.

OK

mkloveyy commented 1 year ago

Thanks @mkloveyy. I think you can try to update the PlaintextOverrider document, based on your experience on above.

Ok, I'll try to update the related doc first.

RainbowMango commented 8 months ago

I'm trying to figure out which PR/Issue should be included in the coming v1.7 release which is planned at the end of this month. I guess we don't have enough time for this feature, so I'm moving this to v1.8.