From 51db940dcc485a82303dc0b9b664ab564d462914 Mon Sep 17 00:00:00 2001 From: Rita Zhang Date: Wed, 1 Mar 2023 19:05:50 -0800 Subject: [PATCH] kmsv2: improve test coverage Signed-off-by: Rita Zhang --- .../server/options/encryptionconfig/config.go | 2 +- .../options/encryptionconfig/config_test.go | 126 ++++++++++++++++-- .../invalid-configs/invalid-aes-gcm.yaml | 8 ++ .../kms/invalid-config-type.yaml | 11 ++ .../invalid-configs/kms/invalid-content.yaml | 0 .../invalid-configs/kms/invalid-gvk.yaml | 11 ++ .../encrypt/envelope/metrics/metrics_test.go | 45 +++++++ .../k8s.io/kms/internal/plugins/mock/go.mod | 2 +- .../k8s.io/kms/internal/plugins/mock/go.sum | 4 +- 9 files changed, 195 insertions(+), 14 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/invalid-aes-gcm.yaml create mode 100644 staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-config-type.yaml create mode 100644 staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-content.yaml create mode 100644 staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-gvk.yaml diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go index 3f73a3d5426..631aa64dff1 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go +++ b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go @@ -347,7 +347,7 @@ func loadConfig(filepath string, reload bool) (*apiserverconfig.EncryptionConfig configObj, gvk, err := codecs.UniversalDecoder().Decode(data, nil, nil) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("error decoding encryption provider configuration file %q: %w", filepath, err) } config, ok := configObj.(*apiserverconfig.EncryptionConfiguration) if !ok { 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 d0f5ab38102..3cbb8c411e8 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 @@ -214,6 +214,12 @@ func TestEncryptionProviderConfigCorrect(t *testing.T) { t.Fatalf("KMSCloseGracePeriod mismatch (-want +got):\n%s", cmp.Diff(expectedKMSCloseGracePeriod, aesGcmFirstEncryptionConfiguration.KMSCloseGracePeriod)) } + invalidConfigWithAesGcm := "testdata/invalid-configs/invalid-aes-gcm.yaml" + _, err = LoadEncryptionConfig(ctx, invalidConfigWithAesGcm, false) + if !strings.Contains(errString(err), "error while parsing file") { + t.Fatalf("should result in error while parsing configuration file: %s.\nThe file was:\n%s", err, invalidConfigWithAesGcm) + } + // Math for GracePeriod is explained at - https://github.com/kubernetes/kubernetes/blob/c9ed04762f94a319d7b1fb718dc345491a32bea6/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go#L159-L163 expectedKMSCloseGracePeriod = 26 * time.Second correctConfigWithAesCbcFirst := "testdata/valid-configs/aes-cbc-first.yaml" @@ -307,9 +313,27 @@ func TestKMSMaxTimeout(t *testing.T) { testCases := []struct { name string + expectedErr string expectedTimeout time.Duration config apiserverconfig.EncryptionConfiguration }{ + { + name: "config with bad provider", + config: apiserverconfig.EncryptionConfiguration{ + Resources: []apiserverconfig.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []apiserverconfig.ProviderConfiguration{ + { + KMS: nil, + }, + }, + }, + }, + }, + expectedErr: "provider does not contain any of the expected providers: KMS, AESGCM, AESCBC, Secretbox, Identity", + expectedTimeout: 6 * time.Second, + }, { name: "default timeout", config: apiserverconfig.EncryptionConfiguration{ @@ -333,6 +357,7 @@ func TestKMSMaxTimeout(t *testing.T) { }, }, }, + expectedErr: "", expectedTimeout: 6 * time.Second, }, { @@ -375,6 +400,7 @@ func TestKMSMaxTimeout(t *testing.T) { }, }, }, + expectedErr: "", expectedTimeout: 12 * time.Second, }, { @@ -433,6 +459,7 @@ func TestKMSMaxTimeout(t *testing.T) { }, }, }, + expectedErr: "", expectedTimeout: 32 * time.Second, }, { @@ -491,6 +518,7 @@ func TestKMSMaxTimeout(t *testing.T) { }, }, }, + expectedErr: "", expectedTimeout: 15 * time.Second, }, } @@ -509,14 +537,21 @@ func TestKMSMaxTimeout(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // cancel this upfront so the kms v2 checks do not block - _, _, kmsUsed, _ := getTransformerOverridesAndKMSPluginHealthzCheckers(ctx, &testCase.config) - if kmsUsed == nil { - t.Fatal("kmsUsed should not be nil") + _, _, kmsUsed, err := getTransformerOverridesAndKMSPluginHealthzCheckers(ctx, &testCase.config) + + if !strings.Contains(errString(err), testCase.expectedErr) { + t.Fatalf("expecting error calling prefixTransformersAndProbes, expected: %s, got: %s", testCase.expectedErr, errString(err)) + } + if len(testCase.expectedErr) == 0 { + if kmsUsed == nil { + t.Fatal("kmsUsed should not be nil") + } + + if kmsUsed.kmsTimeoutSum != testCase.expectedTimeout { + t.Fatalf("expected timeout %v, got %v", testCase.expectedTimeout, kmsUsed.kmsTimeoutSum) + } } - if kmsUsed.kmsTimeoutSum != testCase.expectedTimeout { - t.Fatalf("expected timeout %v, got %v", testCase.expectedTimeout, kmsUsed.kmsTimeoutSum) - } }) } } @@ -539,6 +574,30 @@ func TestKMSPluginHealthz(t *testing.T) { kmsv2 bool kmsv1 bool }{ + { + desc: "Invalid config file path", + config: "invalid/path", + want: nil, + wantErr: `error opening encryption provider configuration file "invalid/path"`, + }, + { + desc: "Empty config file content", + config: "testdata/invalid-configs/kms/invalid-content.yaml", + want: nil, + wantErr: `encryption provider configuration file "testdata/invalid-configs/kms/invalid-content.yaml" is empty`, + }, + { + desc: "Unable to decode", + config: "testdata/invalid-configs/kms/invalid-gvk.yaml", + want: nil, + wantErr: `error decoding encryption provider configuration file`, + }, + { + desc: "Unexpected config type", + config: "testdata/invalid-configs/kms/invalid-config-type.yaml", + want: nil, + wantErr: `no kind "EncryptionConfigurations" is registered for version "apiserver.config.k8s.io/v1"`, + }, { desc: "Install Healthz", config: "testdata/valid-configs/kms/default-timeout.yaml", @@ -593,7 +652,7 @@ func TestKMSPluginHealthz(t *testing.T) { for _, tt := range testCases { t.Run(tt.desc, func(t *testing.T) { config, _, err := loadConfig(tt.config, false) - if errStr := errString(err); errStr != tt.wantErr { + if errStr := errString(err); !strings.Contains(errStr, tt.wantErr) { t.Fatalf("unexpected error state got=%s want=%s", errStr, tt.wantErr) } if len(tt.wantErr) > 0 { @@ -837,7 +896,7 @@ func TestCBCKeyRotationWithoutOverlappingProviders(t *testing.T) { func testCBCKeyRotationWithProviders(t *testing.T, firstEncryptionConfig, firstPrefix, secondEncryptionConfig, secondPrefix string) { p := getTransformerFromEncryptionConfig(t, firstEncryptionConfig) - ctx := context.Background() + ctx := testContext(t) dataCtx := value.DefaultContext([]byte("authenticated_data")) out, err := p.TransformToStorage(ctx, []byte("firstvalue"), dataCtx) @@ -907,6 +966,7 @@ func getTransformerFromEncryptionConfig(t *testing.T, encryptionConfigPath strin func TestIsKMSv2ProviderHealthyError(t *testing.T) { testCases := []struct { desc string + expectedErr string statusResponse *kmsservice.StatusResponse }{ { @@ -914,12 +974,14 @@ func TestIsKMSv2ProviderHealthyError(t *testing.T) { statusResponse: &kmsservice.StatusResponse{ Healthz: "unhealthy", }, + expectedErr: "got unexpected healthz status: unhealthy, expected KMSv2 API version v2alpha1, got , expected KMSv2 KeyID to be set, got ", }, { desc: "version is not v2alpha1", statusResponse: &kmsservice.StatusResponse{ Version: "v1beta1", }, + expectedErr: "got unexpected healthz status: , expected KMSv2 API version v2alpha1, got v1beta1, expected KMSv2 KeyID to be set, got ", }, { desc: "missing keyID", @@ -927,13 +989,24 @@ func TestIsKMSv2ProviderHealthyError(t *testing.T) { Healthz: "ok", Version: "v2alpha1", }, + expectedErr: "expected KMSv2 KeyID to be set, got ", + }, + { + desc: "invalid long keyID", + statusResponse: &kmsservice.StatusResponse{ + Healthz: "ok", + Version: "v2alpha1", + KeyID: sampleInvalidKeyID, + }, + expectedErr: "expected KMSv2 KeyID to be set, got ", }, } for _, tt := range testCases { t.Run(tt.desc, func(t *testing.T) { - if err := isKMSv2ProviderHealthy("testplugin", tt.statusResponse); err == nil { - t.Fatalf("isKMSv2ProviderHealthy() should have returned an error") + err := isKMSv2ProviderHealthy("testplugin", tt.statusResponse) + if !strings.Contains(errString(err), tt.expectedErr) { + t.Errorf("expected err %q, got %q", tt.expectedErr, errString(err)) } }) } @@ -961,3 +1034,36 @@ func TestComputeEncryptionConfigHash(t *testing.T) { t.Errorf("expected hash %q but got %q", expect, sum) } } + +func TestGetCurrentKeyID(t *testing.T) { + ctx := testContext(t) + kmsv2Probe := &kmsv2PluginProbe{ + name: "foo", + ttl: 3 * time.Second, + } + testCases := []struct { + desc string + keyID string + expectedErr string + }{ + { + desc: "empty keyID", + keyID: "", + expectedErr: "got unexpected empty keyID", + }, + { + desc: "valid keyID", + keyID: "1", + expectedErr: "", + }, + } + for _, tt := range testCases { + t.Run(tt.desc, func(t *testing.T) { + kmsv2Probe.keyID.Store(&tt.keyID) + _, err := kmsv2Probe.getCurrentKeyID(ctx) + if errString(err) != tt.expectedErr { + t.Errorf("expected err %q, got %q", tt.expectedErr, errString(err)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/invalid-aes-gcm.yaml b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/invalid-aes-gcm.yaml new file mode 100644 index 00000000000..75bb54e580b --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/invalid-aes-gcm.yaml @@ -0,0 +1,8 @@ +kind: EncryptionConfiguration +apiVersion: apiserver.config.k8s.io/v1 +resources: + - resources: + - secrets + providers: + - aesgcm: + keys: diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-config-type.yaml b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-config-type.yaml new file mode 100644 index 00000000000..b4b7f44b421 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-config-type.yaml @@ -0,0 +1,11 @@ +kind: EncryptionConfigurations +apiVersion: apiserver.config.k8s.io/v1 +resources: + - resources: + - secrets + providers: + - kms: + apiVersion: v2 + name: testproviderv2 + endpoint: unix:///tmp/testprovider.sock + timeout: 15s diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-content.yaml b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-content.yaml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-gvk.yaml b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-gvk.yaml new file mode 100644 index 00000000000..d1a2cc1c3d2 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/testdata/invalid-configs/kms/invalid-gvk.yaml @@ -0,0 +1,11 @@ +kinds: EncryptionConfiguration +apiVersion: apiserver.config.k8s.io/v1 +resources: + - resources: + - secrets + providers: + - kms: + apiVersion: v2 + name: testproviderv2 + endpoint: unix:///tmp/testprovider.sock + timeout: 15s diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/metrics/metrics_test.go index d9cbeeaebf8..b72099c77b0 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/metrics/metrics_test.go @@ -345,6 +345,51 @@ func TestRecordKeyIDLRUKey(t *testing.T) { } } +func TestRecordKeyIDFromStatus(t *testing.T) { + RegisterMetrics() + + cacheSize = 3 + registerLRUMetrics() + KeyIDHashStatusLastTimestampSeconds.Reset() + defer KeyIDHashStatusLastTimestampSeconds.Reset() + + var wg sync.WaitGroup + for i := 1; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + keyID := rand.String(32) + key := metricLabels{ + providerName: rand.String(32), + keyIDHash: getHash(keyID), + } + RecordKeyIDFromStatus(key.providerName, keyID) + }() + } + wg.Wait() + + validMetrics := 0 + metricFamilies, err := legacyregistry.DefaultGatherer.Gather() + if err != nil { + t.Fatal(err) + } + for _, family := range metricFamilies { + if family.GetName() != "apiserver_envelope_encryption_key_id_hash_status_last_timestamp_seconds" { + continue + } + for _, metric := range family.GetMetric() { + if metric.Gauge.GetValue() == 0 { + t.Errorf("invalid metric seen: %s", metric.String()) + } else { + validMetrics++ + } + } + } + if validMetrics != cacheSize { + t.Fatalf("expected total valid metrics to be the same as cacheSize %d, got %d", cacheSize, validMetrics) + } +} + func TestRecordInvalidKeyIDFromStatus(t *testing.T) { testCases := []struct { desc string diff --git a/staging/src/k8s.io/kms/internal/plugins/mock/go.mod b/staging/src/k8s.io/kms/internal/plugins/mock/go.mod index 0952b21025e..95f3899f1fb 100644 --- a/staging/src/k8s.io/kms/internal/plugins/mock/go.mod +++ b/staging/src/k8s.io/kms/internal/plugins/mock/go.mod @@ -3,7 +3,7 @@ module k8s.io/kms/plugins/mock go 1.19 require ( - k8s.io/klog/v2 v2.90.0 + k8s.io/klog/v2 v2.90.1 k8s.io/kms v0.0.0-00010101000000-000000000000 ) diff --git a/staging/src/k8s.io/kms/internal/plugins/mock/go.sum b/staging/src/k8s.io/kms/internal/plugins/mock/go.sum index 44c3fada722..2ba7a32ce29 100644 --- a/staging/src/k8s.io/kms/internal/plugins/mock/go.sum +++ b/staging/src/k8s.io/kms/internal/plugins/mock/go.sum @@ -162,7 +162,7 @@ gopkg.in/yaml.v2 v2.2.3/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= -k8s.io/klog/v2 v2.90.0 h1:VkTxIV/FjRXn1fgNNcKGM8cfmL1Z33ZjXRTVxKCoF5M= -k8s.io/klog/v2 v2.90.0/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= +k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw= +k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= k8s.io/utils v0.0.0-20230209194617-a36077c30491 h1:r0BAOLElQnnFhE/ApUsg3iHdVYYPBjNSSOMowRZxxsY= k8s.io/utils v0.0.0-20230209194617-a36077c30491/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=