Add xDS Kubernetes service #980

minwoox · 2024-07-15T07:06:51Z

Add xDS Kubernetes Service

Motivation:
To support integration with Kubernetes, we need APIs for creating, updating, and deleting Kubernetes watchers that retrieve pod endpoint information from the Kubernetes control plane.

Modifications:

Added xds_kubernetes.proto files that define gRPC and HTTP APIs for xDS resources.
Introduced XdsKubernetesService:
- The createWatcherRequest stores watcher information at /k8s/watchers/{watcher_id}.json.
- To validate the correctness of the watcher information, a KubernetesEndpointGroup is created and used.
  - The current implementation uses KubernetesEndpointGroup for simplicity, with plans to develop a custom implementation for improved error handling.
Added XdsKubernetesEndpointFetchingPlugin and XdsKubernetesEndpointFetchingService:
- The XdsKubernetesEndpointFetchingService, run by the ZooKeeper leader, creates ClusterLoadAssignment based on watcher information.
- The resulting ClusterLoadAssignment is stored at /k8s/endpoints/{watcher_id}.json and replicated via ZooKeeper log replay.
- The cluster name of the ClusterLoadAssignment is groups/{group}/k8s/clusters/{watcher_id}.json, and it is served as an xDS endpoint resource by the ControlPlaneService.

Result:

You can now add a watcher information that retrieves the pod's endpoint information from the Kubernetes control plane.

minwoox · 2024-08-12T12:00:53Z

This is ready. PTAL. 🙇

jrhee17

Left some questions but looks good to me 👍 👍 👍

jrhee17 · 2024-08-13T07:43:25Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+        kubernetesWatchers.values().forEach(map -> {
+            map.values().forEach(KubernetesEndpointGroup::closeAsync);
+            map.clear();
+        });
+        kubernetesWatchers.clear();


Question) Would this be called from plugins-for-* threads here, whereas map updates will be called from k8s-plugin-executor? i.e. the map doesn't seem to be thread-safe

I believe the stop should also be done cleanly since it is not uncommon for a node to lose leadership

Oops, let me fix that. 😓

jrhee17 · 2024-08-13T08:19:15Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+                return;
+            }
+            executor().execute(
+                    () ->  pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));


Suggested change

() -> pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));

() -> pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));

jrhee17 · 2024-08-13T08:20:19Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+        if (kubernetesEndpointGroup.isClosing()) {
+            return;
+        }
+        final LocalityLbEndpoints.Builder localityLbEndpointsBuilder = LocalityLbEndpoints.newBuilder();


Note) I understood that Locality is not set for this iteration.

That is correct. 👍

jrhee17 · 2024-08-13T08:26:25Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+            executor().execute(
+                    () ->  pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));


nit; there is probably a low chance this happens, but I understood that if listener invocation and delete occurs in quick succession, then it is possible that the endpoints remain even though it should be deleted.

I think it's fine to leave this as a known issue, but just want to check if this scenario is possible.

e.g.

handleXdsResource is called

onFileRemoved is called immediately

the pushK8sEndpoints invoked by step 1 is called

I don't think so because they are all executed by the executor().
1 handleXdsResource is called by the executor
2 onFileRemoved is called immediately
3 Thus, the created KubernetesEndpointGroup is closing
4 the pushK8sEndpoints invoked by step 1 is called by the executor but it doesn't push a commit because this condition:

centraldogma/xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

Line 163 in 37e4002

if (kubernetesEndpointGroup.isClosing()) {

I missed that line 😅 Thanks for the pointer 👍

jrhee17 · 2024-08-13T09:02:08Z

xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java

+                .withTrustCerts(kubernetesConfig.getTrustCerts());
+
+        if (!isNullOrEmpty(kubernetesConfig.getOauthToken())) {
+            configBuilder.withOauthToken(kubernetesConfig.getOauthToken());


Question) Am I understanding correctly that as of now users can access this value via APIs? I'm wondering if we're going need to prioritize access control for xDS related APIs.

Users cannot see this value. Only creating, updating and deleting (without seeing the value) is possible at the moment.

ikhoon · 2024-08-14T03:12:36Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

    @Override
    public void init(PluginInitContext pluginInitContext) {
        final InternalProjectInitializer projectInitializer = pluginInitContext.internalProjectInitializer();
+        pluginInitContext.commandExecutor();


nit: Could be removed?

ikhoon · 2024-08-14T03:14:33Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

-
-    public static final String DEFAULT_GROUP = "default_group";
+    @Nullable
+    private volatile ControlPlaneService controlPlaneService;

    public static final long BACKOFF_SECONDS = 30;


It seems unsed.

ikhoon · 2024-08-14T03:19:45Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

    @Override
    public CompletionStage<Void> start(PluginContext context) {
        return UnmodifiableFuture.completedFuture(null);
    }

    @Override
    public CompletionStage<Void> stop(PluginContext context) {
-        stop = true;
+        final ControlPlaneService controlPlaneService = this.controlPlaneService;
+        assert controlPlaneService != null;


I guess controlPlaneService could be null if projectInitializer.initialize(...) fails.

ikhoon · 2024-08-14T03:50:52Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+        executorService = ExecutorServiceMetrics.monitor(
+                meterRegistry, Executors.newSingleThreadScheduledExecutor(
+                        new DefaultThreadFactory("k8s-plugin-executor", true)), "k8sPluginExecutor");
+        executorService.execute(this::start);


Should we wait for the result of this::start? Otherwise, the exception raised in start won't be propagated to somewhere or logged.

Oops, yeah. We need to wait for that. 😉

ikhoon · 2024-08-14T03:56:18Z

...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java

+    private static final Pattern WATCHERS_PATTERN = Pattern.compile("(?<=/k8s)/watchers/");
+
+    private final CommandExecutor commandExecutor;
+    private final MeterRegistry meterRegistry;


nit: dead code?

ikhoon · 2024-08-14T05:12:42Z

xds/src/main/proto/centraldogma/xds/k8s/v1/xds_kubernetes.proto

+
+service XdsKubernetesService {
+
+  rpc CreateWatcher(CreateWatcherRequest) returns (Watcher) {


Optional) How about using service, which seems a simpler expression than watcher which is more abstract so we may need to explain what it means.

Meanwhile, we can simply say that service indicates Kubenetes Service, and this method can be used to create xDS resource for Kubenetes Service.
https://kubernetes.io/docs/concepts/services-networking/service/

Had a discussion and will use CreateServiceEndpointWatcher instead. 😉

ikhoon

Thanks, @minwoox! 🚀🚀

codecov · 2024-08-16T02:46:33Z

Codecov Report

Attention: Patch coverage is 77.87611% with 100 lines in your changes missing coverage. Please review.

Project coverage is 71.54%. Comparing base (c960313) to head (e9db3c6).
Report is 144 commits behind head on main.

Files	Patch %	Lines
...s/k8s/v1/XdsKubernetesEndpointFetchingService.java	68.86%	24 Missing and 9 partials ⚠️
...dogma/xds/internal/XdsResourceWatchingService.java	66.66%	25 Missing and 5 partials ⚠️
.../centraldogma/xds/k8s/v1/XdsKubernetesService.java	82.71%	8 Missing and 6 partials ⚠️
...ds/k8s/v1/XdsKubernetesEndpointFetchingPlugin.java	59.09%	8 Missing and 1 partial ⚠️
...traldogma/server/internal/ExecutorServiceUtil.java	40.00%	4 Missing and 2 partials ⚠️
...centraldogma/xds/internal/ControlPlaneService.java	94.04%	2 Missing and 3 partials ⚠️
.../centraldogma/xds/internal/ControlPlanePlugin.java	77.77%	1 Missing and 1 partial ⚠️
...orp/centraldogma/xds/group/v1/XdsGroupService.java	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #980      +/-   ##
============================================
+ Coverage     65.61%   71.54%   +5.92%     
- Complexity     3319     4113     +794     
============================================
  Files           355      402      +47     
  Lines         13865    16431    +2566     
  Branches       1498     1756     +258     
============================================
+ Hits           9098    11756    +2658     
+ Misses         3916     3642     -274     
- Partials        851     1033     +182

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[WIP] Add xDS Kubernetes service

4bd8f71

minwoox requested review from ikhoon, jrhee17 and trustin as code owners July 15, 2024 07:06

minwoox added the new feature label Jul 15, 2024

minwoox added this to the 0.68.0 milestone Jul 15, 2024

minwoox modified the milestones: 0.68.0, 0.69.0 Jul 23, 2024

minwoox added 5 commits August 8, 2024 22:44

Merge branch 'main' into k8s_service

d68fbe6

Update

85665ad

Update

37e4002

update test

427fa35

Remove log

2f57845

minwoox changed the title ~~[WIP] Add xDS Kubernetes service~~ Add xDS Kubernetes service Aug 12, 2024

Revert InternalProjectInitializer

56b43d4

jrhee17 approved these changes Aug 13, 2024

View reviewed changes

minwoox added 3 commits August 13, 2024 20:33

Address comments from jrhee17

b5dace4

Fix

eb23479

Remove started boolean

60c1888

ikhoon reviewed Aug 14, 2024

View reviewed changes

minwoox added 2 commits August 14, 2024 15:30

Address comments from @ikhoon

3eb7c78

Use group_id

7adbca1

ikhoon approved these changes Aug 16, 2024

View reviewed changes

Merge branch 'main' into k8s_service

e9db3c6

minwoox added 2 commits August 16, 2024 14:02

Precompute

0885bf2

lint

9acec7a

ikhoon merged commit 02701ae into line:main Aug 16, 2024
10 checks passed

minwoox deleted the k8s_service branch August 16, 2024 06:48

minwoox mentioned this pull request Aug 16, 2024

Test failure: XdsRouteServiceTest #1014

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add xDS Kubernetes service #980

Add xDS Kubernetes service #980

minwoox commented Jul 15, 2024 •

edited

Loading

minwoox commented Aug 12, 2024

jrhee17 left a comment

jrhee17 Aug 13, 2024

minwoox Aug 13, 2024

jrhee17 Aug 13, 2024

jrhee17 Aug 13, 2024

minwoox Aug 13, 2024

jrhee17 Aug 13, 2024

minwoox Aug 13, 2024

jrhee17 Aug 14, 2024

jrhee17 Aug 13, 2024

minwoox Aug 13, 2024 •

edited

Loading

ikhoon Aug 14, 2024

ikhoon Aug 14, 2024

ikhoon Aug 14, 2024

ikhoon Aug 14, 2024

minwoox Aug 14, 2024

ikhoon Aug 14, 2024

ikhoon Aug 14, 2024

minwoox Aug 14, 2024

ikhoon left a comment

codecov bot commented Aug 16, 2024

	() -> pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));
	() -> pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));

		executor().execute(
		() -> pushK8sEndpoints(kubernetesEndpointGroup, groupName, endpoints, watcherName));


		service XdsKubernetesService {

		rpc CreateWatcher(CreateWatcherRequest) returns (Watcher) {

Add xDS Kubernetes service #980

Add xDS Kubernetes service #980

Conversation

minwoox commented Jul 15, 2024 • edited Loading

minwoox commented Aug 12, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2024

Codecov Report

minwoox commented Jul 15, 2024 •

edited

Loading

minwoox Aug 13, 2024 •

edited

Loading