envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25k stars 4.81k forks source link

configdump `include_eds` giving unexpected behavior for `additional_addresses` #35100

Closed howardjohn closed 3 months ago

howardjohn commented 4 months ago

Followup to https://github.com/envoyproxy/envoy/issues/34740.

What we send to envoy:

clusterName: outbound|80||echo-v6v4.default.svc.cluster.local
endpoints:
- lbEndpoints:
  - endpoint:
      additionalAddresses:
      - address:
          socketAddress:
            address: fd00:10:244::8
            portValue: 80
      address:
        socketAddress:
          address: 10.244.0.8
          portValue: 80
    healthStatus: HEALTHY
    loadBalancingWeight: 1
    metadata:
      filterMetadata:
        istio:
          workload: echo;default;echo;;Kubernetes
  loadBalancingWeight: 1
  locality: {}
'@type': type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment
clusterName: outbound|15010||istiod.istio-system.svc.cluster.local
endpoints:
- lbEndpoints:
  - endpoint:
      additionalAddresses:
      - address:
          socketAddress:
            address: fd00:10:244::e
            portValue: 15010
      address:
        socketAddress:
          address: 10.244.0.14
          portValue: 15010
    healthStatus: HEALTHY
    loadBalancingWeight: 1
    metadata:
      filterMetadata:
        istio:
          workload: istiod;istio-system;istiod;;Kubernetes
  loadBalancingWeight: 1
  locality: {}

What configdump shows

- endpoint_config:
    '@type': type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment
    cluster_name: outbound|80||echo-v6v4.default.svc.cluster.local
    endpoints:
    - lb_endpoints:
      - endpoint:
          address:
            socket_address:
              address: 10.244.0.8
              port_value: 80
          health_check_config: {}
        health_status: HEALTHY
        load_balancing_weight: 1
        metadata:
          filter_metadata:
            istio:
              workload: echo;default;echo;;Kubernetes
      load_balancing_weight: 1
      locality: {}
    policy:
      overprovisioning_factor: 140
- endpoint_config:
    '@type': type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment
    cluster_name: outbound|15010||istiod.istio-system.svc.cluster.local
    endpoints:
    - lb_endpoints:
      - endpoint:
          additional_addresses:
          - address:
              socket_address:
                address: 10.244.0.13
                port_value: 15010
          - address:
              socket_address:
                address: fd00:10:244::d
                port_value: 15010
          address:
            socket_address:
              address: 10.244.0.13
              port_value: 15010
          health_check_config: {}
        health_status: HEALTHY
        load_balancing_weight: 1
        metadata:
          filter_metadata:
            istio:
              workload: istiod;istio-system;istiod;;Kubernetes
      load_balancing_weight: 1
      locality: {}
    policy:
      overprovisioning_factor: 140

We see one is missing the endpoint, and one is duplicating them

Repro steps:

Include sample requests, environment, etc. All data and inputs required to reproduce the bug.

leosarra commented 4 months ago

I'm looking into it

nezdolik commented 4 months ago

cc @jmarantz

leosarra commented 4 months ago

The issue of the additional_addresses dump containing also the default address should be fixed by #35120 The bug was caused by the fact that I erronously considered _addresseslist to be the list of additional addresses instead of a list containing _default_address+additionaladdresses.

I'm still looking into reproducing the other issue that was reported where there is a missing entry.

leosarra commented 4 months ago

While trying to reproduce the issue I also realized that hosts from static clusters never had their addresses list set which led to incomplete additional_addresses in configdumps as well. I have addressed that one as well as part of the PR.

The dump from my side looks fine now. See output of istioctl configdump viewer istioctl_pc_endpoints.txt (I removed the workaround in istioctl that was applied to filter out the duplicated entries). @howardjohn can you check that it looks good for you as well with https://github.com/envoyproxy/envoy/pull/35120 applied?

howardjohn commented 4 months ago

nice!

On Thu, Jul 11, 2024, 5:38 AM Leonardo Sarra @.***> wrote:

While trying to reproduce the issue I also realized that hosts from static clusters never had their addresses list set which led to incomplete additional_addresses in configdumps as well. I have addressed that one as well as part of the PR.

The dump from my side looks fine now istioctl_pc_endpoints.txt https://github.com/user-attachments/files/16174415/istioctl_pc_endpoints.txt (I used istioctl configdump viewer without the workaround that was applied to filter out the duplicated entries that were there before). @howardjohn https://github.com/howardjohn can you check that it looks good for you as well with #35120 https://github.com/envoyproxy/envoy/pull/35120 applied?

— Reply to this email directly, view it on GitHub https://github.com/envoyproxy/envoy/issues/35100#issuecomment-2222480255, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYGXJ6WAXWF63BVBF7RFTZLZHAPAVCNFSM6AAAAABKRPGL5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGQ4DAMRVGU . You are receiving this because you were mentioned.Message ID: @.***>

leosarra commented 3 months ago

@nezdolik can this issue be closed now that the PR is merged? I don't the permissions to change the status of the issue on GitHub