Merge pull request #116202 from ritazh/kmsv2-testcoverage

kmsv2: improve test coverage

Kubernetes-commit: 165829587a6fe398a1e473001f6b7dfa32ad9e37

go.mod

@@ -44,10 +44,10 @@ require (
 	gopkg.in/square/go-jose.v2 v2.6.0
 	k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4
 	k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d
-	k8s.io/client-go v0.0.0-20230303000145-3f4372de09cb
+	k8s.io/client-go v0.0.0-20230303065657-bfa72fd2d367
 	k8s.io/component-base v0.0.0-20230302013528-cdd3d3a94e73
 	k8s.io/klog/v2 v2.90.1
-	k8s.io/kms v0.0.0-20230302204122-57de6be5f4a6
+	k8s.io/kms v0.0.0-20230303123242-0b3f9dde5fcd
 	k8s.io/kube-openapi v0.0.0-20230123231816-1cb3ae25d79a
 	k8s.io/utils v0.0.0-20230209194617-a36077c30491
 	sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.1.1
@@ -126,7 +126,7 @@ require (
 replace (
 	k8s.io/api => k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4
 	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d
-	k8s.io/client-go => k8s.io/client-go v0.0.0-20230303000145-3f4372de09cb
+	k8s.io/client-go => k8s.io/client-go v0.0.0-20230303065657-bfa72fd2d367
 	k8s.io/component-base => k8s.io/component-base v0.0.0-20230302013528-cdd3d3a94e73
-	k8s.io/kms => k8s.io/kms v0.0.0-20230302204122-57de6be5f4a6
+	k8s.io/kms => k8s.io/kms v0.0.0-20230303123242-0b3f9dde5fcd
 )

go.sum

@@ -878,14 +878,14 @@ k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4 h1:qm5Xz7wiOrtVHPKAZsBByEd6k2WST6T
 k8s.io/api v0.0.0-20230302120942-f6c2559ad4f4/go.mod h1:cTD04/XhoraqP0GpFXtefYJYXBw6coqVSibz5Rzivkw=
 k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d h1:mridg1Zm6thnb5oTe+rOGnEUbPnjys9YHBFxlOf+GeA=
 k8s.io/apimachinery v0.0.0-20230302115847-76eb944e266d/go.mod h1:TO4higCGNMwebVSdb1XPJdXMU4kk+nmMY/cTMVCGa6M=
-k8s.io/client-go v0.0.0-20230303000145-3f4372de09cb h1:f+duaMC7mXtKgrzzFQYIHMdyR0436E1X/r4JDU9CYYk=
-k8s.io/client-go v0.0.0-20230303000145-3f4372de09cb/go.mod h1:4+y45xff2wn/bjSSXI+/SqdRZnYI4r8ndWJg7eOsyNg=
+k8s.io/client-go v0.0.0-20230303065657-bfa72fd2d367 h1:y1rUzE1VGpnWqpVBHXCOp8BlmTtZ3bU6I8L5xAQd3gg=
+k8s.io/client-go v0.0.0-20230303065657-bfa72fd2d367/go.mod h1:4+y45xff2wn/bjSSXI+/SqdRZnYI4r8ndWJg7eOsyNg=
 k8s.io/component-base v0.0.0-20230302013528-cdd3d3a94e73 h1:9aUdsRcbNOOttZL0FVKTuiZlVLCef4+aAKh7DAmArOw=
 k8s.io/component-base v0.0.0-20230302013528-cdd3d3a94e73/go.mod h1:OEU5Klnkrq4+Nh1Ir1vBDc2JL9FvvCcmjzrnR/eT7I4=
 k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw=
 k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
-k8s.io/kms v0.0.0-20230302204122-57de6be5f4a6 h1:dC5jw8zGznaPcsoInDJEs999or8eS21PNFC67HV2wdI=
-k8s.io/kms v0.0.0-20230302204122-57de6be5f4a6/go.mod h1:hp2A3BisYagCb7/4bWUf48fdBFflj12BBYAmH9XpWaI=
+k8s.io/kms v0.0.0-20230303123242-0b3f9dde5fcd h1:0vh05+hg06/s6B0rPgOZ4ej5AH7GNp4qX6GLEa+pjVw=
+k8s.io/kms v0.0.0-20230303123242-0b3f9dde5fcd/go.mod h1:BsJEqK1MWN8sYXv2fVg9sQ8DJcmCIGFo/WDDOokPOTQ=
 k8s.io/kube-openapi v0.0.0-20230123231816-1cb3ae25d79a h1:s6zvHjyDQX1NtVT88pvw2tddqhqY0Bz0Gbnn+yctsFU=
 k8s.io/kube-openapi v0.0.0-20230123231816-1cb3ae25d79a/go.mod h1:/BYxry62FuDzmI+i9B+X2pqfySRmSOW2ARmj5Zbqhj0=
 k8s.io/utils v0.0.0-20230209194617-a36077c30491 h1:r0BAOLElQnnFhE/ApUsg3iHdVYYPBjNSSOMowRZxxsY=

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 {

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))
+			}
+		})
+	}
+}

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:

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

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

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