New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kms: add wiring to support automatic encryption config reload #113529
Conversation
/retest |
616a86f
to
43f941f
Compare
changes lgtm, would like an ack from the folks we know using this in beta state |
@bingosummer can you confirm this change works for AKS? |
43f941f
to
74261c8
Compare
/retest |
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Will this change be behind a feature flag, or only if KMSv2 is in use?
We depend on being able to exclude providers we know are not being referred to by etcd from health checks, and this would prevent us from doing that.
No, this proposes a change in the default of how the server works. It is not related to KMS v2 explicitly and actually benefits v1 implementations far more because the v1 API lacks any ability to inform the API server at runtime about changes to its config.
Sorry, can you be specific on what behavior you are trying to observe? If you know that a provider isn't being used, why not simply remove it from your encryption config? |
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
11efaf5
to
b7793eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic looks good, one request on the plumbing of the kmsv1/kmsv2 indicators to avoid accidents or future us losing our minds
@@ -419,7 +457,7 @@ var ( | |||
EnvelopeKMSv2ServiceFactory = envelopekmsv2.NewGRPCService | |||
) | |||
|
|||
func kmsPrefixTransformer(config *apiserverconfig.KMSConfiguration, stopCh <-chan struct{}) (value.PrefixTransformer, healthChecker, error) { | |||
func kmsPrefixTransformer(config *apiserverconfig.KMSConfiguration, stopCh <-chan struct{}) (value.PrefixTransformer, healthChecker, bool, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit, but name the return values or godoc it or something (and maybe order them as kmsv1, kmsv2?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comments up the call chain, sorry... two undocumented bools in a surprising order is asking for accidents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, make a struct to hold info about the loaded encryption config with KMSV1 and KMSV2 bools, since it looks like #112050 is going to add ~3 more return values that need plumbing as well, which is really unweildy (max kms timeout, content hash, and totalKMSPlugins (though I'm not sure why we need the total kms count)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though I'm not sure why we need the total kms count
Each call to /healthz
for KMS v1 can result in two gRPC calls (each of which have the KMS timeout)
With the kms-providers
endpoint, these calls all happen in a union
So I think the "safe" sleep duration is (which is obviously an overestimate):
2 * 2 * <num_kms> * <max timeout>
where the first 2
is simply a buffer, the second 2
is for the two gRPC calls for kms v1
This change adds a flag --encryption-provider-config-automatic-reload which will be used to drive automatic reloading of the encryption config at runtime. While this flag is set to true, or when KMS v2 plugins are used without KMS v1 plugins, the /healthz endpoints associated with said plugins are collapsed into a single endpoint at /healthz/kms-providers - in this state, it is not possible to configure exclusions for specific KMS providers while including the remaining ones - ex: using /readyz?exclude=kms-provider-1 to exclude a particular KMS is not possible. This single healthz check handles checking all configured KMS providers. When reloading is enabled but no KMS providers are configured, it is a no-op. k8s.io/apiserver does not support dynamic addition and removal of healthz checks at runtime. Reloading will instead have a single static healthz check and swap the underlying implementation at runtime when a config change occurs. Signed-off-by: Monis Khan <mok@microsoft.com>
b7793eb
to
22e540b
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt 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 |
This change adds a flag --encryption-provider-config-automatic-reload
which will be used to drive automatic reloading of the encryption
config at runtime. While this flag is set to true, or when KMS v2
plugins are used without KMS v1 plugins, the /healthz endpoints
associated with said plugins are collapsed into a single endpoint at
/healthz/kms-providers - in this state, it is not possible to
configure exclusions for specific KMS providers while including the
remaining ones - ex: using /readyz?exclude=kms-provider-1 to exclude
a particular KMS is not possible. This single healthz check handles
checking all configured KMS providers. When reloading is enabled
but no KMS providers are configured, it is a no-op.
k8s.io/apiserver does not support dynamic addition and removal of
healthz checks at runtime. Reloading will instead have a single
static healthz check and swap the underlying implementation at
runtime when a config change occurs.
Signed-off-by: Monis Khan mok@microsoft.com
/kind cleanup
/sig auth
/priority important-soon
/milestone v1.26
xref #112050