From 39aca564749cd92ed1cfec7129eb3f6593549137 Mon Sep 17 00:00:00 2001 From: immutableT Date: Fri, 4 Jan 2019 17:06:07 -0800 Subject: [PATCH] require timeout to be greater than zero. add unit test to cover timeout behaviour. --- .../server/options/encryptionconfig/config.go | 4 +- .../options/encryptionconfig/config_test.go | 8 +- .../pkg/storage/value/encrypt/envelope/BUILD | 10 ++ .../envelope/grpc_service_unix_test.go | 139 ++++++++++++++++-- 4 files changed, 142 insertions(+), 19 deletions(-) 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 ed5d32d8b17..de0201534d5 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 @@ -175,8 +175,8 @@ func GetPrefixTransformers(config *apiserverconfig.ResourceConfiguration) ([]val timeout := kmsPluginConnectionTimeout if provider.KMS.Timeout != nil { - if provider.KMS.Timeout.Duration < 0 { - return nil, fmt.Errorf("could not configure KMS plugin %q, timeout should be positive value", provider.KMS.Name) + if provider.KMS.Timeout.Duration <= 0 { + return nil, fmt.Errorf("could not configure KMS plugin %q, timeout should be a positive value", provider.KMS.Name) } timeout = provider.KMS.Timeout.Duration } 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 7c03344668b..39f01fbb3a5 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 @@ -443,7 +443,7 @@ resources: want: 15 * time.Second, }, { - desc: "duration explicitly provided as 0", + desc: "duration explicitly provided as 0 which is an invalid value, error should be returned", config: `kind: EncryptionConfiguration apiVersion: apiserver.config.k8s.io/v1 resources: @@ -453,9 +453,9 @@ resources: - kms: name: foo endpoint: unix:///tmp/testprovider.sock - timeout: 0 + timeout: 0s `, - want: 0, + wantErr: "timeout should be a positive value", }, { desc: "duration is not provided, default will be supplied", @@ -485,7 +485,7 @@ resources: timeout: -15s `, - wantErr: "timeout should be positive value", + wantErr: "timeout should be a positive value", }, } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/BUILD b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/BUILD index bd42719d88d..68a3cdceda9 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/BUILD @@ -36,42 +36,52 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/aes:go_default_library", ] + select({ "@io_bazel_rules_go//go/platform:android": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:darwin": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:dragonfly": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:freebsd": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:linux": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:nacl": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:netbsd": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:openbsd": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:plan9": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], "@io_bazel_rules_go//go/platform:solaris": [ + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1:go_default_library", "//vendor/google.golang.org/grpc:go_default_library", ], diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service_unix_test.go b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service_unix_test.go index ca124770183..430ef2a1a0e 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service_unix_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service_unix_test.go @@ -31,18 +31,17 @@ import ( "google.golang.org/grpc" + "k8s.io/apimachinery/pkg/util/uuid" kmsapi "k8s.io/apiserver/pkg/storage/value/encrypt/envelope/v1beta1" ) -const ( - endpoint = "unix:///@kms-socket.sock" -) - // TestKMSPluginLateStart tests the scenario where kms-plugin pod/container starts after kube-apiserver pod/container. // Since the Dial to kms-plugin is non-blocking we expect the construction of gRPC service to succeed even when // kms-plugin is not yet up - dialing happens in the background. func TestKMSPluginLateStart(t *testing.T) { + t.Parallel() callTimeout := 3 * time.Second + endpoint := getSocketName() service, err := NewGRPCService(endpoint, callTimeout) if err != nil { @@ -51,7 +50,7 @@ func TestKMSPluginLateStart(t *testing.T) { defer destroyService(service) time.Sleep(callTimeout / 2) - f, err := startFakeKMSProvider(kmsapiVersion) + f, err := startFakeKMSProvider(kmsapiVersion, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -64,17 +63,119 @@ func TestKMSPluginLateStart(t *testing.T) { } } +// TestTimeout tests behaviour of the kube-apiserver based on the supplied timeout and delayed start of kms-plugin. +func TestTimeouts(t *testing.T) { + t.Parallel() + var testCases = []struct { + desc string + callTimeout time.Duration + pluginDelay time.Duration + kubeAPIServerDelay time.Duration + wantErr string + }{ + { + desc: "timeout zero - expect failure when call from kube-apiserver arrives before plugin starts", + callTimeout: 0 * time.Second, + pluginDelay: 3 * time.Second, + wantErr: "rpc error: code = DeadlineExceeded desc = context deadline exceeded", + }, + { + desc: "timeout zero but kms-plugin already up - still failure - zero timeout is an invalid value", + callTimeout: 0 * time.Second, + pluginDelay: 0 * time.Second, + kubeAPIServerDelay: 2 * time.Second, + wantErr: "rpc error: code = DeadlineExceeded desc = context deadline exceeded", + }, + { + desc: "timeout greater than kms-plugin delay - expect success", + callTimeout: 6 * time.Second, + pluginDelay: 3 * time.Second, + }, + { + desc: "timeout less than kms-plugin delay - expect failure", + callTimeout: 3 * time.Second, + pluginDelay: 6 * time.Second, + wantErr: "rpc error: code = DeadlineExceeded desc = context deadline exceeded", + }, + } + + for _, tt := range testCases { + tt := tt + t.Run(tt.desc, func(t *testing.T) { + t.Parallel() + var ( + service Service + err error + data = []byte("test data") + kubeAPIServerWG sync.WaitGroup + kmsPluginWG sync.WaitGroup + testCompletedWG sync.WaitGroup + socketName = getSocketName() + ) + + testCompletedWG.Add(1) + defer testCompletedWG.Done() + + kubeAPIServerWG.Add(1) + go func() { + // Simulating late start of kube-apiserver - plugin is up before kube-apiserver, if requested by the testcase. + time.Sleep(tt.kubeAPIServerDelay) + + service, err = NewGRPCService(socketName, tt.callTimeout) + if err != nil { + t.Fatalf("failed to create envelope service, error: %v", err) + } + defer destroyService(service) + kubeAPIServerWG.Done() + // Keeping kube-apiserver up to process requests. + testCompletedWG.Wait() + }() + + kmsPluginWG.Add(1) + go func() { + // Simulating delayed start of kms-plugin, kube-apiserver is up before the plugin, if requested by the testcase. + time.Sleep(tt.pluginDelay) + + f, err := startFakeKMSProvider(kmsapiVersion, socketName) + if err != nil { + t.Fatalf("failed to start test KMS provider server, error: %v", err) + } + defer f.server.Stop() + kmsPluginWG.Done() + // Keeping plugin up to process requests. + testCompletedWG.Wait() + }() + + kubeAPIServerWG.Wait() + _, err = service.Encrypt(data) + + if err == nil && tt.wantErr != "" { + t.Fatalf("got nil, want %s", tt.wantErr) + } + + if err != nil && tt.wantErr == "" { + t.Fatalf("got %q, want nil", err.Error()) + } + + // Collecting kms-plugin - allowing plugin to clean-up. + kmsPluginWG.Wait() + }) + } +} + // TestIntermittentConnectionLoss tests the scenario where the connection with kms-plugin is intermittently lost. func TestIntermittentConnectionLoss(t *testing.T) { + t.Parallel() var ( wg1 sync.WaitGroup wg2 sync.WaitGroup timeout = 30 * time.Second blackOut = 1 * time.Second data = []byte("test data") + endpoint = getSocketName() ) // Start KMS Plugin - f, err := startFakeKMSProvider(kmsapiVersion) + f, err := startFakeKMSProvider(kmsapiVersion, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -93,8 +194,9 @@ func TestIntermittentConnectionLoss(t *testing.T) { t.Log("Connected to KMSPlugin") // Stop KMS Plugin - simulating connection loss + t.Log("KMS Plugin is stopping") f.server.Stop() - t.Log("KMS Plugin is stopped") + time.Sleep(2 * time.Second) wg1.Add(1) wg2.Add(1) @@ -112,7 +214,7 @@ func TestIntermittentConnectionLoss(t *testing.T) { wg1.Wait() time.Sleep(blackOut) // Start KMS Plugin - f, err = startFakeKMSProvider(kmsapiVersion) + f, err = startFakeKMSProvider(kmsapiVersion, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -123,11 +225,13 @@ func TestIntermittentConnectionLoss(t *testing.T) { } func TestUnsupportedVersion(t *testing.T) { + t.Parallel() ver := "invalid" data := []byte("test data") wantErr := fmt.Errorf(versionErrorf, ver, kmsapiVersion) + endpoint := getSocketName() - f, err := startFakeKMSProvider(ver) + f, err := startFakeKMSProvider(ver, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %ver", err) } @@ -162,8 +266,10 @@ func TestUnsupportedVersion(t *testing.T) { // Normal encryption and decryption operation. func TestGRPCService(t *testing.T) { + t.Parallel() // Start a test gRPC server. - f, err := startFakeKMSProvider(kmsapiVersion) + endpoint := getSocketName() + f, err := startFakeKMSProvider(kmsapiVersion, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -196,8 +302,10 @@ func TestGRPCService(t *testing.T) { // Normal encryption and decryption operation by multiple go-routines. func TestGRPCServiceConcurrentAccess(t *testing.T) { + t.Parallel() // Start a test gRPC server. - f, err := startFakeKMSProvider(kmsapiVersion) + endpoint := getSocketName() + f, err := startFakeKMSProvider(kmsapiVersion, endpoint) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -245,10 +353,15 @@ func destroyService(service Service) { } } +func getSocketName() string { + return fmt.Sprintf("unix:///@%s.sock", uuid.NewUUID()) +} + // Test all those invalid configuration for KMS provider. func TestInvalidConfiguration(t *testing.T) { + t.Parallel() // Start a test gRPC server. - f, err := startFakeKMSProvider(kmsapiVersion) + f, err := startFakeKMSProvider(kmsapiVersion, getSocketName()) if err != nil { t.Fatalf("failed to start test KMS provider server, error: %v", err) } @@ -275,7 +388,7 @@ func TestInvalidConfiguration(t *testing.T) { } // Start the gRPC server that listens on unix socket. -func startFakeKMSProvider(version string) (*fakeKMSPlugin, error) { +func startFakeKMSProvider(version, endpoint string) (*fakeKMSPlugin, error) { sockFile, err := parseEndpoint(endpoint) if err != nil { return nil, fmt.Errorf("failed to parse endpoint:%q, error %v", endpoint, err)