Support OAuth Token for Kubernetes Authentication via Credential Service #1038
Conversation
Motivation: We decided to repurpose the `MirrorCredential` to manage all repository credentials, not just those specific to mirroring. To reflect this role, we should remove `Mirror` prefix from the class name. Caveat: This commit must be deployed to central dogma replicas after applying the changed from line#1030. Modifications: - Renamed `MirroringCredential` to `Credential` and moved its package. - Removed `hostnamePatterns` property in `Credential`. Result: - The renamed `Credential` class can now be used for managing various types of repository credentials, beyond just mirroring.
Motivation: The current approach stores the Kubernetes oauthToken in the Kubernetes configuration, which is not ideal for sensitive information. To improve security, we need a way to specify and manage the oauthToken through the credential service, allowing it to be securely retrieved and used by the Kubernetes service. Modifications: - Added functionality to use the `oauthToken` for Kubernetes authentication via the credential service if the token is specified with `credential:`. Result: - Kubernetes OAuth tokens can now be securely managed and retrieved through the credential service.
|
Let me wait until this is rebased over #1031 since it is difficult to review the changes related to OAuth Token. |
Yeah, let me ping you when it's ready. 😉 |
...src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java
Show resolved
Hide resolved
| if (endpoints.isEmpty()) { | ||
| return; | ||
| } | ||
| executorService.execute(updater::maybeSchedule); |
There was a problem hiding this comment.
How about passing kubernetesEndpointGroup as an argument to maybeSchedule to avoid calling kubernetesEndpointGroupFuture.join()?
There was a problem hiding this comment.
That's a good suggestion. 👍
xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
Show resolved
Hide resolved
| } | ||
|
|
||
| return configBuilder.build(); | ||
| if (!oauthToken.startsWith("credential:")) { |
There was a problem hiding this comment.
Question) Would it make more sense to enforce users to use credentials?
Since we're still free to make breaking changes, we could just rename oauthToken to credentialId or oauthCredentialId while we're at it.
There was a problem hiding this comment.
Yeah, that's a good idea. 👍
| if (scheduledFuture != null) { | ||
| return; | ||
| } | ||
| // Commit after 1 second so that it pushes with all the fetched endpoints in the duration | ||
| // instead of pushing one by one. | ||
| scheduledFuture = executorService.schedule(() -> { | ||
| scheduledFuture = null; | ||
| // maybeSchedule() is called after the future is completed. |
xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
Show resolved
Hide resolved
| Status.INTERNAL.withDescription( | ||
| "Failed to retrieve k8s endpoints within 5 seconds. watcherName: " + | ||
| watcher.getName()).asRuntimeException()); | ||
| }, 5, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Q) 5 seconds seems short compared to request timeouts. Why did you set 5 seconds as the deadline?
There was a problem hiding this comment.
I chose a value of less than 10 seconds, which is the request timeout.
This API has two phases: calling k8s and committing.
I estimated that the API would need at least 5 seconds for the committing phase so I allocated 5 seconds for the k8s.
I could potentially increase the default timeout and assign the larger value but I felt it wasn't necessary at this point.
This timeout will also be removed when I implement an API to fetch the information from k8s instead of using k8sEndpointGroup.
Do you have any recommendations?
|
Let me merge this to work on supporting merging multiple k8s clusters into one. Thanks! |
Motivation:
The current approach stores the Kubernetes oauthToken in the Kubernetes configuration, which is not ideal for sensitive information. To improve security, we need a way to specify and manage the oauthToken through the credential service, allowing it to be securely retrieved and used by the Kubernetes service.
Modifications:
oauthTokenfor Kubernetes authentication via the credential service if the token is specified withcredential:.Result: