line / centraldogma

Highly-available version-controlled service configuration repository based on Git, ZooKeeper and HTTP/2
https://line.github.io/centraldogma/
Apache License 2.0
599 stars 117 forks source link

`XdsCentralDogmaBuilder` accepts a `localClusterName` instead of `serviceCluster` #1023

Closed jrhee17 closed 1 month ago

jrhee17 commented 1 month ago

Motivation

While verifying whether zone-aware routing works correctly, I found that the serviceCluster options wasn't being respected.

(string) Name of the local cluster (i.e., the cluster that owns the Envoy running this configuration). In order to enable zone aware routing this option must be set. If local_cluster_name is defined then clusters must be defined in the Bootstrap static cluster resources. This is unrelated to the --service-cluster option which do es not affect zone aware routing.

ref: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/bootstrap/v3/bootstrap.proto#envoy-v3-api-field-config-bootstrap-v3-clustermanager-local-cluster-name

I'm not sure why I thought using serviceCluster was a good idea, but it is probably a better idea to accept localClusterName if we want to take advantage of zone-aware routing

Modifications

Result

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.58%. Comparing base (f810f16) to head (543e762). Report is 6 commits behind head on main.

Files Patch % Lines
...gma/client/armeria/xds/XdsCentralDogmaBuilder.java 60.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1023 +/- ## ============================================ - Coverage 71.60% 71.58% -0.03% - Complexity 4121 4122 +1 ============================================ Files 402 402 Lines 16452 16474 +22 Branches 1762 1768 +6 ============================================ + Hits 11781 11793 +12 - Misses 3640 3646 +6 - Partials 1031 1035 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.