sealerio / sealer

Build, Share and Run Both Your Kubernetes Cluster and Distributed Applications (Project under CNCF)
http://sealer.cool
Apache License 2.0
2.06k stars 362 forks source link

merge config did not take effect #1482

Open sfeng1996 opened 2 years ago

sfeng1996 commented 2 years ago

What happened:

i want to use sealer config to update calico network interface IP discovery rule,but not effective this is my config clusterfile

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

this is "etc/custom-resources.yaml" after sealer apply

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: {}

i guess 'etc/custom-resource.yaml' have not key of 'nodeAddressAutodetectionV4:\n interface:' , so sealer deepMerge config did not take effect

Environment:

bxy4543 commented 2 years ago

Merge is only suitable for merging existing structures, and there are too many uncertainties for attributes that do not exist in this way, such as:

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

result:

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    nodeAddressAutodetectionV4:
      interface: "eth.*|en.*|wlp.*|bond.*"
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: 
  calicoNetwork:
  nodeAddressAutodetectionV4:
    interface: "eth.*|en.*|wlp.*|bond.*"

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

allencloud commented 2 years ago

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

@bxy4543 It is the limitation of current functionality. While we should clarify that as clearly as possible. Otherwise, user tries, then user abandons. We need quite a long time to improve the service quality. 😢

sfeng1996 commented 2 years ago

Merge is only suitable for merging existing structures, and there are too many uncertainties for attributes that do not exist in this way, such as:

---
apiVersion: sealer.aliyun.com/v1alpha1
kind: Config
metadata:
  creationTimestamp: null
  name: nodeAddr
spec:
  data: |
    spec:
      calicoNetwork:
        nodeAddressAutodetectionV4:
          interface: "eth.*|en.*|wlp.*|bond.*"
  path: etc/custom-resources.yaml
  strategy: merge
status: {}

result:

apiVersion: operator.tigera.io/v1
kind: Installation
metadata:
  name: default
spec:
  calicoNetwork:
    nodeAddressAutodetectionV4:
      interface: "eth.*|en.*|wlp.*|bond.*"
    ipPools:
    - blockSize: 26
      cidr: 100.64.0.0/10
      encapsulation: VXLANCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()
  registry: sea.hub:5000

---
apiVersion: operator.tigera.io/v1
kind: APIServer
metadata:
  name: default
spec: 
  calicoNetwork:
  nodeAddressAutodetectionV4:
    interface: "eth.*|en.*|wlp.*|bond.*"

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

i know what you mean,can you develop a new strategy for similar situation,the overwrite strategy is not convenient for long text files

bxy4543 commented 2 years ago

The properties of APIServer are also merged in, so I think merge strategy is more suitable for merging of already existing properties.

@bxy4543 It is the limitation of current functionality. While we should clarify that as clearly as possible. Otherwise, user tries, then user abandons. We need quite a long time to improve the service quality. 😢

yes, I will explain it in all the docs.

bxy4543 commented 2 years ago

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

sfeng1996 commented 2 years ago

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

tks,look forward to it

kakaZhou719 commented 2 years ago

@sfeng1996 I think we can provide a shallowMerge strategy to achieve this feature.

@bxy4543 , my opinion about this is could we find another way to do merge rather then provide shallowMerge, thats because it make our config function more complexed. on the other hand, merge strategy native means add a new key which is not exist on the dest.