Expose kms timeout value via encryption config.#72540
Expose kms timeout value via encryption config.#72540k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
/assign @liggitt |
There was a problem hiding this comment.
document the default if unspecified
There was a problem hiding this comment.
document the default if unspecified
There was a problem hiding this comment.
should be *metav1.Duration so we can tell the difference between "0" and unspecified
There was a problem hiding this comment.
should be +optional and omitempty, right?
|
@liggitt PTAL. |
|
/test pull-kubernetes-verify |
|
The failure is legitimate. The generated files need to be regenerated and checked in |
|
/test pull-kubernetes-e2e-kops-aws |
|
thanks @liggitt |
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
|
one comment on validation, then squash, lgtm otherwise |
f6bf95a to
a4dc53c
Compare
|
|
||
| timeout := kmsPluginConnectionTimeout | ||
| if provider.KMS.Timeout != nil { | ||
| if provider.KMS.Timeout.Duration < 0 { |
There was a problem hiding this comment.
@liggitt good catch - zero seems to make gRPC calls to kms-plugin fail no matter what - even when plugin is up and running before kube-apiserver makes its first call.
Based on that, changed the validation logic to prohibit zero.
Added unit test to cover timeout test cases; let me know if I should put them on a separate PR.
|
/test pull-kubernetes-e2e-gce-device-plugin-gpu |
|
/test pull-kubernetes-integration |
a2c1836 to
d1bf08d
Compare
add unit test to cover timeout behaviour.
d1bf08d to
39aca56
Compare
|
/lgtm please detail the config field in the release note and open a doc PR to update the encryption docs for 1.14 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: immutableT, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
|
Documentation PR 12158 submitted. |
|
@liggitt is this something that could be cherry-picked to 1.13? |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Expose, via EncryptionConfiguration, the timeout value that kube-apiserver will use when awaiting responses from kms-plugin.
This is a followup to 68585, were a timeout was added to calls from kube-apiserver to kms-plugin. This PR exposes the value of the timeout via EncryptionConfiguration. Providing the appropriate value of the timeout should be left to cluster managers who are familiar with the environment in which the cluster and the plugin operate. For example, KMS rooted in in a hardware device like (HSM/TPM) may have fairly long delays (seconds), while in memory KMS implementations would have timeouts in the range of milliseconds.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: