feat: Reuse provider grpc clients #394
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/retitle feat: Reuse provider grpc clients |
k8s-ci-robot
changed the title
| r.generateEvent(pod, v1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("failed to create provider client, err: %+v", err)) | ||
| return fmt.Errorf("failed to create provider client, err: %+v", err) | ||
| providerClient, exists := r.providerClients[providerName] | ||
| if !exists { |
There was a problem hiding this comment.
This logic is valid now as we support a mix of gRPC and old format for invoking the providers. The --grpc-supported-providers flag is the source of truth. When we make gRPC the default mode of communication between driver-provider, we'll need to have a fallback logic here to create the provider client on the fly if not found or we still need to continue setting the --grpc-supported-providers flag. WDYT?
There was a problem hiding this comment.
hmmm, good point, I need to think on that for a bit.
I'm generally a fan of explicit configuration like --grpc-supported-providers (I like a service's dependencies to not change during execution) but I'm also not a fan of having to edit the driver yaml for new/internal providers...
Another option would be to wrap the map in a structure with a mutex to create new clients on lookup misses. I think that would require a --deprecated-exec-providers flag to be added in the future.
There was a problem hiding this comment.
As a cluster operator, I can see benefits of setting the --grpc-supported-providers flag explicitly to control what driver-providers are allowed and rolled out in the clusters.
But I can also see benefits of creating a provider client on the fly reduces driver downtime.
Should we open an issue to discuss this more and get more user feedback on the desired experience?
There was a problem hiding this comment.
I've opened an issue to discuss this: #401.
This PR will change the current behavior where we create it on the fly. If we do go with this approach, then we should also add azure provider to the --grpc-supported-providers in all the manifests in the driver repo and vault when it's available.
I'm not sure if users are actually piggybacking on --grpc-supported-providers to control which providers are used in the cluster. They would just want to configure the SecretProviderClass and assume the driver knows how to communicate with that provider which in the future by default will just be grpc.
Create the provider GRPC clients during process initialization instead of on-demand. The Dial() call is non-blocking and if the connection is disrupted grpc should re-establish the connection. By configuring RPCs to retry through a service configuration, this will ensure that when a plugin is restarted the connection will be re-dialed and the RPC retried.
tam7t
force-pushed
the
reusable-connection
branch
from
9243df6
to
c34b0a4
Compare
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ritazh, tam7t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Create the provider GRPC clients during process initialization instead
of on-demand. The Dial() call is non-blocking and if the connection is
disrupted grpc should re-establish the connection. By configuring RPCs
to retry through a service configuration, this will ensure that when a
plugin is restarted the connection will be re-dialed and the RPC
retried.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #300
Special notes for your reviewer: