[KMSv2] Use status key ID to determine staleness of encrypted data

[ritazh]

What type of PR is this?

/kind feature

What this PR does / why we need it:

KMSv2: Use status key ID to determine staleness of encrypted data

Which issue(s) this PR fixes:

Fixes #111922

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/issues/3299

/milestone v1.27
/sig auth
/triage accepted

[ritazh]

/assign @enj

[ritazh]

/kind feature

[ritazh]

/test pull-kubernetes-unit

[aramase]

/cc

[enj]

@liggitt this PR makes the API server coast (forever) on the last successful key ID it got from the plugin. Thus staleness checks for read calls will not fail if the plugin is down transiently. At a high level I believe we have three choices here:

1. Coast on the last key ID, no error, staleness based on last observed key ID
2. Error and fail the read request (would partially defeat the purpose of having a cache since we would no longer tolerate plugin downtime)
3. Mark the read request as stale=true - for no-op updates this would cause a write (which would presumably fail since the plugin is down)

What do you think is the best approach here?

[enj]

Maybe another option is to make stale a *bool to try and express the "I do not know" state?

[liggitt]

it's hard to imagine life being better for a cluster admin if we make the cluster start failing reads ... I wouldn't do that.

failing no-op writes doesn't seem useful to me either... it pushes errors to users unlikely to be able to do anything about them

I think I would expect to coast, and for an error here to increment a metric to make it clear something is not working as expected (either transiently or persistently)

[ritazh]

Opened #115188 to add metrics as a follow up.

[enj]

Trying to think if there is a way to have an integration test that proves that this loop is running. 🤔

[enj]

Running the tests locally I see that https://github.com/ritazh/kubernetes/blob/c5f62be64070e68ca8f7f8c38af0a52ce119107d/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go#L569 causes a panic every so often in the new go wait.PollImmediateUntilWithContext function. Here is a workaround for that:

diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go
index bd57f31ac77..4f93314e37a 100644
--- a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go
@@ -26,8 +26,10 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/wait"
 	apiserverconfig "k8s.io/apiserver/pkg/apis/config"
 	"k8s.io/apiserver/pkg/features"
 	"k8s.io/apiserver/pkg/storage/value"
@@ -243,7 +245,7 @@ func TestEncryptionProviderConfigCorrect(t *testing.T) {
 
 		for _, transformer := range transformers {
 			untransformedData, stale, err := transformer.Transformer.TransformFromStorage(ctx, transformedData, dataCtx)
-			if err != nil && err.Error() != "got unexpected empty keyID" {
+			if err != nil {
 				t.Fatalf("%s: error while reading using %s transformer: %s", testCase.Name, transformer.Name, err)
 			}
 			if stale != (transformer.Name != testCase.Name) {
@@ -551,7 +553,9 @@ func TestKMSPluginHealthz(t *testing.T) {
 				return
 			}
 
-			_, got, kmsUsed, err := getTransformerOverridesAndKMSPluginProbes(config, testContext(t).Done())
+			ctx, cancel := context.WithCancel(context.Background())
+			cancel() // cancel this upfront so the kms v2 healthz check poll only runs once
+			_, got, kmsUsed, err := getTransformerOverridesAndKMSPluginProbes(config, ctx.Done())
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -565,6 +569,7 @@ func TestKMSPluginHealthz(t *testing.T) {
 					p.l = nil
 					p.lastResponse = nil
 				case *kmsv2PluginProbe:
+					waitForOneKMSv2Check(t, p) // make sure the kms v2 healthz check poll is done
 					p.service = nil
 					p.l = nil
 					p.lastResponse = nil
@@ -595,6 +600,19 @@ func TestKMSPluginHealthz(t *testing.T) {
 	}
 }
 
+func waitForOneKMSv2Check(t *testing.T, p *kmsv2PluginProbe) {
+	t.Helper()
+
+	if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (done bool, err error) {
+		p.l.Lock()
+		defer p.l.Unlock()
+
+		return !p.lastResponse.received.IsZero(), nil
+	}); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestKMSPluginHealthzTTL(t *testing.T) {
 	ctx := testContext(t)
 

[ritazh]

@enj @aramase I've addressed your comments. When you get a chance, PTAL

[enj]

This is close.

[ritazh]

@enj @aramase I've addressed your comments. When you get a chance, PTAL

[enj]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f26e0356dab57c67edc5b38085ae2ea05b91fce0

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, ritazh

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:

• staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/OWNERS [enj]
• staging/src/k8s.io/apiserver/pkg/storage/value/OWNERS [enj]
• test/OWNERS [enj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment