From eaa0906e15936f6f2d3cd95d6d4ba9ab87729203 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 26 Jan 2022 18:55:59 +0100 Subject: [PATCH 01/49] Create certSecretsRepo type to store certificates in k8s secrets --- .../localscanner/secrets_repository.go | 103 +++++++++++ .../localscanner/secrets_repository_test.go | 168 ++++++++++++++++++ 2 files changed, 271 insertions(+) create mode 100644 sensor/kubernetes/localscanner/secrets_repository.go create mode 100644 sensor/kubernetes/localscanner/secrets_repository_test.go diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go new file mode 100644 index 0000000000000..508468cf413aa --- /dev/null +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -0,0 +1,103 @@ +package localscanner + +import ( + "context" + + "github.com/hashicorp/go-multierror" + "github.com/pkg/errors" + "github.com/stackrox/rox/generated/storage" + v1 "k8s.io/api/core/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/client-go/util/retry" +) + +var ( + _ certSecretsRepo = (*certSecretsRepoImpl)(nil) +) + +type certSecretsRepo interface { + getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) + putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error +} + +type certSecretsRepoImpl struct { + secretNames map[storage.ServiceType]string + backoff wait.Backoff + secretsClient corev1.SecretInterface + +} + +func NewCertSecretsRepo(secretNames map[storage.ServiceType]string, + backoff wait.Backoff, secretsClient corev1.SecretInterface) certSecretsRepo { + return &certSecretsRepoImpl{ + secretNames: secretNames, + backoff: backoff, + secretsClient: secretsClient, + } +} + +func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) { + secretsMap := make(map[storage.ServiceType]*v1.Secret, len(r.secretNames)) + var getErr error + for serviceType, secretName := range r.secretNames { + var ( + secret *v1.Secret + err error + ) + retryErr := retry.OnError(r.backoff, + func(err error) bool { + return (ctx.Err() == nil) && !k8sErrors.IsNotFound(err) + }, + func() error { + secret, err = r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) + return err + }, + ) + if retryErr != nil { + getErr = multierror.Append(getErr, errors.Wrapf(retryErr,"for secret %s", secretName)) + } else { + secretsMap[serviceType] = secret + } + // on context cancellation abort getting other secrets. + if ctx.Err() != nil { + return nil, ctx.Err() + } + } + if getErr != nil { + return nil, getErr + } + return secretsMap, nil +} + +func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error { + var putErr error + for serviceType, secretName := range r.secretNames { + secret := secrets[serviceType] + if secret == nil { + putErr = + multierror.Append(putErr, errors.Errorf("no secret found for service type %s", serviceType)) + } else { + retryErr := retry.OnError(r.backoff, + func(err error) bool { + return ctx.Err() == nil + }, + func() error { + _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) + return err + }, + ) + if retryErr != nil{ + putErr = multierror.Append(putErr, errors.Wrapf(retryErr,"for secret %s", secretName)) + } + } + // on context cancellation abort putting other secrets. + if ctx.Err() != nil { + return ctx.Err() + } + } + + return putErr +} \ No newline at end of file diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go new file mode 100644 index 0000000000000..5f5766aea55ac --- /dev/null +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -0,0 +1,168 @@ +package localscanner + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stackrox/rox/generated/storage" + "github.com/stackrox/rox/pkg/concurrency" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" + corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + + "github.com/stretchr/testify/suite" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + fakecorev1 "k8s.io/client-go/kubernetes/typed/core/v1/fake" + k8sTesting "k8s.io/client-go/testing" +) + +const ( + namespace = "namespace" +) +var ( + forcedErr = errors.New("forced error") + serviceTypes = []storage.ServiceType{ + storage.ServiceType_SENSOR_SERVICE, + storage.ServiceType_SCANNER_SERVICE, + storage.ServiceType_SCANNER_DB_SERVICE, + storage.ServiceType_CENTRAL_SERVICE, + } + capTime = 100 * time.Millisecond + shortBackoff = wait.Backoff{ + Duration: capTime, + Factor: 1, + Jitter: 0, + Steps: 2, + Cap: capTime, + } + longBackoff = wait.Backoff{ + Duration: 2 * time.Second, + Factor: 10, + Steps: 20, + } +) +func TestCertSecretsRepo(t *testing.T) { + suite.Run(t, new(certSecretsRepoSuite)) +} + +type certSecretsRepoSuite struct { + suite.Suite +} + +func (s *certSecretsRepoSuite) TestGet() { + testCases := map[string]struct{ + expectedErr error + f *certSecretsRepoFixture + }{ + "successful get": {nil, s.newFixture("", shortBackoff,"foo")}, + "failed get": {forcedErr, s.newFixture("get", shortBackoff, "foo")}, + "cancelled get": {context.Canceled, s.newFixture("get", longBackoff, "foo")}, + } + for tcName, tc := range testCases { + s.Run(tcName, func() { + getCtx, cancelGetCtx := context.WithCancel(context.Background()) + defer cancelGetCtx() + doneErrSig := concurrency.NewErrorSignal() + + go func() { + secrets, err := tc.f.repo.getSecrets(getCtx) + if tc.expectedErr == nil { + s.Equal(len(tc.f.secretsMap), len(secrets)) + for k, v := range tc.f.secretsMap { + s.Equal(v, secrets[k]) + } + } + doneErrSig.SignalWithError(err) + }() + if tc.expectedErr == context.Canceled { + go cancelGetCtx() + } + + timeoutCtx, cancelTimeoutCtx := context.WithTimeout(context.Background(), time.Second) + defer cancelTimeoutCtx() + err, ok := doneErrSig.WaitUntil(timeoutCtx) + s.Require().True(ok) + s.checkExpectedError(tc.expectedErr, err) + }) + } +} + +func (s *certSecretsRepoSuite) TestPut() { + testCases := map[string]struct{ + expectedErr error + f *certSecretsRepoFixture + }{ + "successful put": {nil, s.newFixture("", shortBackoff,"foo")}, + "failed put": {forcedErr, s.newFixture("update", shortBackoff, "foo")}, + "cancelled put": {context.Canceled, s.newFixture("update", longBackoff, "foo")}, + } + for tcName, tc := range testCases { + s.Run(tcName, func() { + putCtx, cancelPutCtx := context.WithCancel(context.Background()) + defer cancelPutCtx() + doneErrSig := concurrency.NewErrorSignal() + + go func() { + err := tc.f.repo.putSecrets(putCtx, tc.f.secretsMap) + doneErrSig.SignalWithError(err) + }() + if tc.expectedErr == context.Canceled { + go cancelPutCtx() + } + + timeoutCtx, cancelTimeoutCtx := context.WithTimeout(context.Background(), time.Second) + defer cancelTimeoutCtx() + err, ok := doneErrSig.WaitUntil(timeoutCtx) + s.Require().True(ok) + s.checkExpectedError(tc.expectedErr, err) + }) + } +} + +func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { + if expectedErr != forcedErr { + s.Equal(expectedErr, err) + } else { + // multierror wraps forcedErr + s.NotNil(err) + } +} + +type certSecretsRepoFixture struct { + repo certSecretsRepo + secretClient corev1.SecretInterface + secretsMap map[storage.ServiceType]*v1.Secret +} + +func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backoff, secretNames ...string) *certSecretsRepoFixture { + s.Require().LessOrEqual(len(secretNames), len(serviceTypes)) + + secretsNamesMap := make(map[storage.ServiceType]string, len(secretNames)) + secretsMap := make(map[storage.ServiceType]*v1.Secret, len(secretNames)) + objects := make([]runtime.Object, len(secretNames)) + for i, secretName := range secretNames { + secret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + }, + } + secretsNamesMap[serviceTypes[i]] = secretName + secretsMap[serviceTypes[i]] = secret + objects[i] = secret + } + clientSet := fake.NewSimpleClientset(objects...) + secretsClient := clientSet.CoreV1().Secrets(namespace) + clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, &v1.Secret{}, forcedErr + }) + return &certSecretsRepoFixture{ + repo: NewCertSecretsRepo(secretsNamesMap, backoff, secretsClient), + secretClient: secretsClient, + secretsMap: secretsMap, + } +} From 6cb9a1c90905066b66bfae3417a389b684029249 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 26 Jan 2022 18:59:19 +0100 Subject: [PATCH 02/49] fix style --- .../localscanner/secrets_repository.go | 23 ++++---- .../localscanner/secrets_repository_test.go | 59 ++++++++++--------- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 508468cf413aa..bd0c773a0f949 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -24,17 +24,18 @@ type certSecretsRepo interface { } type certSecretsRepoImpl struct { - secretNames map[storage.ServiceType]string - backoff wait.Backoff - secretsClient corev1.SecretInterface - + secretNames map[storage.ServiceType]string + backoff wait.Backoff + secretsClient corev1.SecretInterface } +// NewCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and +// for the specified service types. func NewCertSecretsRepo(secretNames map[storage.ServiceType]string, backoff wait.Backoff, secretsClient corev1.SecretInterface) certSecretsRepo { return &certSecretsRepoImpl{ - secretNames: secretNames, - backoff: backoff, + secretNames: secretNames, + backoff: backoff, secretsClient: secretsClient, } } @@ -45,7 +46,7 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi for serviceType, secretName := range r.secretNames { var ( secret *v1.Secret - err error + err error ) retryErr := retry.OnError(r.backoff, func(err error) bool { @@ -57,7 +58,7 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi }, ) if retryErr != nil { - getErr = multierror.Append(getErr, errors.Wrapf(retryErr,"for secret %s", secretName)) + getErr = multierror.Append(getErr, errors.Wrapf(retryErr, "for secret %s", secretName)) } else { secretsMap[serviceType] = secret } @@ -89,8 +90,8 @@ func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storag return err }, ) - if retryErr != nil{ - putErr = multierror.Append(putErr, errors.Wrapf(retryErr,"for secret %s", secretName)) + if retryErr != nil { + putErr = multierror.Append(putErr, errors.Wrapf(retryErr, "for secret %s", secretName)) } } // on context cancellation abort putting other secrets. @@ -100,4 +101,4 @@ func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storag } return putErr -} \ No newline at end of file +} diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 5f5766aea55ac..e8a7352490262 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -8,14 +8,13 @@ import ( "github.com/stackrox/rox/generated/storage" "github.com/stackrox/rox/pkg/concurrency" + "github.com/stretchr/testify/suite" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" - corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - - "github.com/stretchr/testify/suite" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" + corev1 "k8s.io/client-go/kubernetes/typed/core/v1" fakecorev1 "k8s.io/client-go/kubernetes/typed/core/v1/fake" k8sTesting "k8s.io/client-go/testing" ) @@ -23,16 +22,17 @@ import ( const ( namespace = "namespace" ) + var ( - forcedErr = errors.New("forced error") - serviceTypes = []storage.ServiceType{ + errForced = errors.New("forced error") + serviceTypes = []storage.ServiceType{ storage.ServiceType_SENSOR_SERVICE, storage.ServiceType_SCANNER_SERVICE, storage.ServiceType_SCANNER_DB_SERVICE, storage.ServiceType_CENTRAL_SERVICE, } - capTime = 100 * time.Millisecond - shortBackoff = wait.Backoff{ + capTime = 100 * time.Millisecond + shortBackoff = wait.Backoff{ Duration: capTime, Factor: 1, Jitter: 0, @@ -41,10 +41,11 @@ var ( } longBackoff = wait.Backoff{ Duration: 2 * time.Second, - Factor: 10, - Steps: 20, + Factor: 10, + Steps: 20, } ) + func TestCertSecretsRepo(t *testing.T) { suite.Run(t, new(certSecretsRepoSuite)) } @@ -54,13 +55,13 @@ type certSecretsRepoSuite struct { } func (s *certSecretsRepoSuite) TestGet() { - testCases := map[string]struct{ + testCases := map[string]struct { expectedErr error - f *certSecretsRepoFixture + f *certSecretsRepoFixture }{ - "successful get": {nil, s.newFixture("", shortBackoff,"foo")}, - "failed get": {forcedErr, s.newFixture("get", shortBackoff, "foo")}, - "cancelled get": {context.Canceled, s.newFixture("get", longBackoff, "foo")}, + "successful get": {nil, s.newFixture("", shortBackoff, "foo")}, + "failed get": {errForced, s.newFixture("get", shortBackoff, "foo")}, + "cancelled get": {context.Canceled, s.newFixture("get", longBackoff, "foo")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -92,13 +93,13 @@ func (s *certSecretsRepoSuite) TestGet() { } func (s *certSecretsRepoSuite) TestPut() { - testCases := map[string]struct{ + testCases := map[string]struct { expectedErr error - f *certSecretsRepoFixture + f *certSecretsRepoFixture }{ - "successful put": {nil, s.newFixture("", shortBackoff,"foo")}, - "failed put": {forcedErr, s.newFixture("update", shortBackoff, "foo")}, - "cancelled put": {context.Canceled, s.newFixture("update", longBackoff, "foo")}, + "successful put": {nil, s.newFixture("", shortBackoff, "foo")}, + "failed put": {errForced, s.newFixture("update", shortBackoff, "foo")}, + "cancelled put": {context.Canceled, s.newFixture("update", longBackoff, "foo")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -123,19 +124,19 @@ func (s *certSecretsRepoSuite) TestPut() { } } -func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { - if expectedErr != forcedErr { +func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { + if expectedErr != errForced { s.Equal(expectedErr, err) } else { - // multierror wraps forcedErr + // multierror wraps errForced s.NotNil(err) } } type certSecretsRepoFixture struct { - repo certSecretsRepo + repo certSecretsRepo secretClient corev1.SecretInterface - secretsMap map[storage.ServiceType]*v1.Secret + secretsMap map[storage.ServiceType]*v1.Secret } func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backoff, secretNames ...string) *certSecretsRepoFixture { @@ -147,7 +148,7 @@ func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backo for i, secretName := range secretNames { secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretName, + Name: secretName, Namespace: namespace, }, } @@ -158,11 +159,11 @@ func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backo clientSet := fake.NewSimpleClientset(objects...) secretsClient := clientSet.CoreV1().Secrets(namespace) clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { - return true, &v1.Secret{}, forcedErr + return true, &v1.Secret{}, errForced }) return &certSecretsRepoFixture{ - repo: NewCertSecretsRepo(secretsNamesMap, backoff, secretsClient), + repo: NewCertSecretsRepo(secretsNamesMap, backoff, secretsClient), secretClient: secretsClient, - secretsMap: secretsMap, + secretsMap: secretsMap, } } From 86c98e1680a9c031e2336acc11277b0326f0371a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 27 Jan 2022 18:02:56 +0100 Subject: [PATCH 03/49] fix flaky test --- .../localscanner/secrets_repository_test.go | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index e8a7352490262..34e2fddb6a613 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -31,7 +31,7 @@ var ( storage.ServiceType_SCANNER_DB_SERVICE, storage.ServiceType_CENTRAL_SERVICE, } - capTime = 100 * time.Millisecond + capTime = 10 * time.Millisecond shortBackoff = wait.Backoff{ Duration: capTime, Factor: 1, @@ -40,9 +40,11 @@ var ( Cap: capTime, } longBackoff = wait.Backoff{ - Duration: 2 * time.Second, - Factor: 10, - Steps: 20, + Duration: capTime, + Factor: 1, + Steps: 10, + Jitter: 0, + Cap: capTime, } ) @@ -80,12 +82,10 @@ func (s *certSecretsRepoSuite) TestGet() { doneErrSig.SignalWithError(err) }() if tc.expectedErr == context.Canceled { - go cancelGetCtx() + cancelGetCtx() } - timeoutCtx, cancelTimeoutCtx := context.WithTimeout(context.Background(), time.Second) - defer cancelTimeoutCtx() - err, ok := doneErrSig.WaitUntil(timeoutCtx) + err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) s.Require().True(ok) s.checkExpectedError(tc.expectedErr, err) }) @@ -112,12 +112,10 @@ func (s *certSecretsRepoSuite) TestPut() { doneErrSig.SignalWithError(err) }() if tc.expectedErr == context.Canceled { - go cancelPutCtx() + cancelPutCtx() } - timeoutCtx, cancelTimeoutCtx := context.WithTimeout(context.Background(), time.Second) - defer cancelTimeoutCtx() - err, ok := doneErrSig.WaitUntil(timeoutCtx) + err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) s.Require().True(ok) s.checkExpectedError(tc.expectedErr, err) }) @@ -129,7 +127,7 @@ func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { s.Equal(expectedErr, err) } else { // multierror wraps errForced - s.NotNil(err) + s.Error(err) } } From d756b9ed471b0c2b145f02319a54e63212a4048a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 31 Jan 2022 14:51:13 +0100 Subject: [PATCH 04/49] add comments to clarify responsibilities of this code --- sensor/kubernetes/localscanner/secrets_repository.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index bd0c773a0f949..6ff5df25f3687 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -18,8 +18,12 @@ var ( _ certSecretsRepo = (*certSecretsRepoImpl)(nil) ) +// certSecretsRepo is in charge of persisting and retrieving a set of secrets corresponding to service types +// into some permanent storage system. type certSecretsRepo interface { + // getSecrets retrieves the secrets from permanent storage. getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) + // putSecrets persists the secrets on permanent storage. putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error } @@ -30,7 +34,7 @@ type certSecretsRepoImpl struct { } // NewCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and -// for the specified service types. +// for the specified service types, and uses the k8s API for persistence. func NewCertSecretsRepo(secretNames map[storage.ServiceType]string, backoff wait.Backoff, secretsClient corev1.SecretInterface) certSecretsRepo { return &certSecretsRepoImpl{ From a033701305e16c2f5fad5cef839fcbd1f267c826 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 31 Jan 2022 15:07:45 +0100 Subject: [PATCH 05/49] remove retries, leave it to caller --- .../localscanner/secrets_repository.go | 35 +++------------ .../localscanner/secrets_repository_test.go | 44 ++++++------------- 2 files changed, 21 insertions(+), 58 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 6ff5df25f3687..59ae00d05ac3b 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -7,11 +7,8 @@ import ( "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" v1 "k8s.io/api/core/v1" - k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/wait" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/client-go/util/retry" ) var ( @@ -29,17 +26,15 @@ type certSecretsRepo interface { type certSecretsRepoImpl struct { secretNames map[storage.ServiceType]string - backoff wait.Backoff secretsClient corev1.SecretInterface } // NewCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and // for the specified service types, and uses the k8s API for persistence. func NewCertSecretsRepo(secretNames map[storage.ServiceType]string, - backoff wait.Backoff, secretsClient corev1.SecretInterface) certSecretsRepo { + secretsClient corev1.SecretInterface) certSecretsRepo { return &certSecretsRepoImpl{ secretNames: secretNames, - backoff: backoff, secretsClient: secretsClient, } } @@ -52,17 +47,9 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi secret *v1.Secret err error ) - retryErr := retry.OnError(r.backoff, - func(err error) bool { - return (ctx.Err() == nil) && !k8sErrors.IsNotFound(err) - }, - func() error { - secret, err = r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) - return err - }, - ) - if retryErr != nil { - getErr = multierror.Append(getErr, errors.Wrapf(retryErr, "for secret %s", secretName)) + secret, err = r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) + if err != nil { + getErr = multierror.Append(getErr, errors.Wrapf(err, "for secret %s", secretName)) } else { secretsMap[serviceType] = secret } @@ -85,17 +72,9 @@ func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storag putErr = multierror.Append(putErr, errors.Errorf("no secret found for service type %s", serviceType)) } else { - retryErr := retry.OnError(r.backoff, - func(err error) bool { - return ctx.Err() == nil - }, - func() error { - _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) - return err - }, - ) - if retryErr != nil { - putErr = multierror.Append(putErr, errors.Wrapf(retryErr, "for secret %s", secretName)) + _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) + if err != nil { + putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secretName)) } } // on context cancellation abort putting other secrets. diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 34e2fddb6a613..31ec91a5c7998 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -12,7 +12,6 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/fake" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" fakecorev1 "k8s.io/client-go/kubernetes/typed/core/v1/fake" @@ -31,21 +30,6 @@ var ( storage.ServiceType_SCANNER_DB_SERVICE, storage.ServiceType_CENTRAL_SERVICE, } - capTime = 10 * time.Millisecond - shortBackoff = wait.Backoff{ - Duration: capTime, - Factor: 1, - Jitter: 0, - Steps: 2, - Cap: capTime, - } - longBackoff = wait.Backoff{ - Duration: capTime, - Factor: 1, - Steps: 10, - Jitter: 0, - Cap: capTime, - } ) func TestCertSecretsRepo(t *testing.T) { @@ -61,9 +45,9 @@ func (s *certSecretsRepoSuite) TestGet() { expectedErr error f *certSecretsRepoFixture }{ - "successful get": {nil, s.newFixture("", shortBackoff, "foo")}, - "failed get": {errForced, s.newFixture("get", shortBackoff, "foo")}, - "cancelled get": {context.Canceled, s.newFixture("get", longBackoff, "foo")}, + "successful get": {nil, s.newFixture("", "foo")}, + "failed get": {errForced, s.newFixture("get", "foo")}, + "cancelled get": {context.Canceled, s.newFixture("get", "foo")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -72,6 +56,9 @@ func (s *certSecretsRepoSuite) TestGet() { doneErrSig := concurrency.NewErrorSignal() go func() { + if tc.expectedErr == context.Canceled { + cancelGetCtx() + } secrets, err := tc.f.repo.getSecrets(getCtx) if tc.expectedErr == nil { s.Equal(len(tc.f.secretsMap), len(secrets)) @@ -81,9 +68,6 @@ func (s *certSecretsRepoSuite) TestGet() { } doneErrSig.SignalWithError(err) }() - if tc.expectedErr == context.Canceled { - cancelGetCtx() - } err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) s.Require().True(ok) @@ -97,9 +81,9 @@ func (s *certSecretsRepoSuite) TestPut() { expectedErr error f *certSecretsRepoFixture }{ - "successful put": {nil, s.newFixture("", shortBackoff, "foo")}, - "failed put": {errForced, s.newFixture("update", shortBackoff, "foo")}, - "cancelled put": {context.Canceled, s.newFixture("update", longBackoff, "foo")}, + "successful put": {nil, s.newFixture("", "foo")}, + "failed put": {errForced, s.newFixture("update", "foo")}, + "cancelled put": {context.Canceled, s.newFixture("update", "foo")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -108,12 +92,12 @@ func (s *certSecretsRepoSuite) TestPut() { doneErrSig := concurrency.NewErrorSignal() go func() { + if tc.expectedErr == context.Canceled { + cancelPutCtx() + } err := tc.f.repo.putSecrets(putCtx, tc.f.secretsMap) doneErrSig.SignalWithError(err) }() - if tc.expectedErr == context.Canceled { - cancelPutCtx() - } err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) s.Require().True(ok) @@ -137,7 +121,7 @@ type certSecretsRepoFixture struct { secretsMap map[storage.ServiceType]*v1.Secret } -func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backoff, secretNames ...string) *certSecretsRepoFixture { +func (s *certSecretsRepoSuite) newFixture(verbToError string, secretNames ...string) *certSecretsRepoFixture { s.Require().LessOrEqual(len(secretNames), len(serviceTypes)) secretsNamesMap := make(map[storage.ServiceType]string, len(secretNames)) @@ -160,7 +144,7 @@ func (s *certSecretsRepoSuite) newFixture(verbToError string, backoff wait.Backo return true, &v1.Secret{}, errForced }) return &certSecretsRepoFixture{ - repo: NewCertSecretsRepo(secretsNamesMap, backoff, secretsClient), + repo: NewCertSecretsRepo(secretsNamesMap, secretsClient), secretClient: secretsClient, secretsMap: secretsMap, } From 16a5e18beee713e293009204115897b0e8bf1c0a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 31 Jan 2022 15:08:57 +0100 Subject: [PATCH 06/49] use longer test timeout for increased stability on CI --- sensor/kubernetes/localscanner/secrets_repository_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 31ec91a5c7998..afd91e5ae26e2 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -20,6 +20,7 @@ import ( const ( namespace = "namespace" + testTimeout = time.Second ) var ( @@ -69,7 +70,7 @@ func (s *certSecretsRepoSuite) TestGet() { doneErrSig.SignalWithError(err) }() - err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) + err, ok := doneErrSig.WaitWithTimeout(testTimeout) s.Require().True(ok) s.checkExpectedError(tc.expectedErr, err) }) @@ -99,7 +100,7 @@ func (s *certSecretsRepoSuite) TestPut() { doneErrSig.SignalWithError(err) }() - err, ok := doneErrSig.WaitWithTimeout(100 * time.Millisecond) + err, ok := doneErrSig.WaitWithTimeout(testTimeout) s.Require().True(ok) s.checkExpectedError(tc.expectedErr, err) }) From e6e708687ed24c4b3ae7e24a90bbe70102098978 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 31 Jan 2022 15:24:47 +0100 Subject: [PATCH 07/49] only accept a single secret name in `newFixture` --- .../localscanner/secrets_repository_test.go | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index afd91e5ae26e2..6cc3ca1ffdde3 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -19,18 +19,12 @@ import ( ) const ( - namespace = "namespace" + namespace = "namespace" testTimeout = time.Second ) var ( - errForced = errors.New("forced error") - serviceTypes = []storage.ServiceType{ - storage.ServiceType_SENSOR_SERVICE, - storage.ServiceType_SCANNER_SERVICE, - storage.ServiceType_SCANNER_DB_SERVICE, - storage.ServiceType_CENTRAL_SERVICE, - } + errForced = errors.New("forced error") ) func TestCertSecretsRepo(t *testing.T) { @@ -122,24 +116,17 @@ type certSecretsRepoFixture struct { secretsMap map[storage.ServiceType]*v1.Secret } -func (s *certSecretsRepoSuite) newFixture(verbToError string, secretNames ...string) *certSecretsRepoFixture { - s.Require().LessOrEqual(len(secretNames), len(serviceTypes)) - - secretsNamesMap := make(map[storage.ServiceType]string, len(secretNames)) - secretsMap := make(map[storage.ServiceType]*v1.Secret, len(secretNames)) - objects := make([]runtime.Object, len(secretNames)) - for i, secretName := range secretNames { - secret := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: namespace, - }, - } - secretsNamesMap[serviceTypes[i]] = secretName - secretsMap[serviceTypes[i]] = secret - objects[i] = secret +func (s *certSecretsRepoSuite) newFixture(verbToError string, secretName string) *certSecretsRepoFixture { + serviceType := storage.ServiceType_SCANNER_SERVICE + secretsNamesMap := map[storage.ServiceType]string{serviceType: secretName} + secret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + }, } - clientSet := fake.NewSimpleClientset(objects...) + secretsMap := map[storage.ServiceType]*v1.Secret{serviceType: secret} + clientSet := fake.NewSimpleClientset(secret) secretsClient := clientSet.CoreV1().Secrets(namespace) clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced From 7811e3a9f80339ded27baac361760a9d4700418d Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 31 Jan 2022 15:25:55 +0100 Subject: [PATCH 08/49] use "stackrox-ns" as test namespace --- sensor/kubernetes/localscanner/secrets_repository_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 6cc3ca1ffdde3..d8fb18dd036b9 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -19,7 +19,7 @@ import ( ) const ( - namespace = "namespace" + namespace = "stackrox-ns" testTimeout = time.Second ) From 6434c941909cb563c929991cc32f3ce6d8fef423 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 1 Feb 2022 18:16:54 +0100 Subject: [PATCH 09/49] code cleanup --- sensor/kubernetes/localscanner/secrets_repository.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 59ae00d05ac3b..71b5513f659e2 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -43,11 +43,7 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi secretsMap := make(map[storage.ServiceType]*v1.Secret, len(r.secretNames)) var getErr error for serviceType, secretName := range r.secretNames { - var ( - secret *v1.Secret - err error - ) - secret, err = r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) + secret, err := r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) if err != nil { getErr = multierror.Append(getErr, errors.Wrapf(err, "for secret %s", secretName)) } else { @@ -69,8 +65,7 @@ func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storag for serviceType, secretName := range r.secretNames { secret := secrets[serviceType] if secret == nil { - putErr = - multierror.Append(putErr, errors.Errorf("no secret found for service type %s", serviceType)) + putErr = multierror.Append(putErr, errors.Errorf("no secret found for service type %s", serviceType)) } else { _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) if err != nil { From 42d64a54fc0418cc52b07ff913c8b7796dbdceb2 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 1 Feb 2022 18:26:26 +0100 Subject: [PATCH 10/49] improve comments --- sensor/kubernetes/localscanner/secrets_repository.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 71b5513f659e2..ab1926f6b0248 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -16,7 +16,9 @@ var ( ) // certSecretsRepo is in charge of persisting and retrieving a set of secrets corresponding to service types -// into some permanent storage system. +// into some permanent storage system, thus implementing the +// [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for a map from service types +// to secrets. type certSecretsRepo interface { // getSecrets retrieves the secrets from permanent storage. getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) @@ -29,9 +31,9 @@ type certSecretsRepoImpl struct { secretsClient corev1.SecretInterface } -// NewCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and +// newCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and // for the specified service types, and uses the k8s API for persistence. -func NewCertSecretsRepo(secretNames map[storage.ServiceType]string, +func newCertSecretsRepo(secretNames map[storage.ServiceType]string, secretsClient corev1.SecretInterface) certSecretsRepo { return &certSecretsRepoImpl{ secretNames: secretNames, From 2fc10132c2119b6131a7489778898baffb7e369a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 1 Feb 2022 18:26:59 +0100 Subject: [PATCH 11/49] simplify tests delegating test timeout to CI task --- .../localscanner/secrets_repository_test.go | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index d8fb18dd036b9..474aad98e5214 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -4,10 +4,8 @@ import ( "context" "errors" "testing" - "time" "github.com/stackrox/rox/generated/storage" - "github.com/stackrox/rox/pkg/concurrency" "github.com/stretchr/testify/suite" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -20,7 +18,6 @@ import ( const ( namespace = "stackrox-ns" - testTimeout = time.Second ) var ( @@ -48,24 +45,18 @@ func (s *certSecretsRepoSuite) TestGet() { s.Run(tcName, func() { getCtx, cancelGetCtx := context.WithCancel(context.Background()) defer cancelGetCtx() - doneErrSig := concurrency.NewErrorSignal() + if tc.expectedErr == context.Canceled { + cancelGetCtx() + } - go func() { - if tc.expectedErr == context.Canceled { - cancelGetCtx() - } - secrets, err := tc.f.repo.getSecrets(getCtx) - if tc.expectedErr == nil { - s.Equal(len(tc.f.secretsMap), len(secrets)) - for k, v := range tc.f.secretsMap { - s.Equal(v, secrets[k]) - } - } - doneErrSig.SignalWithError(err) - }() + secrets, err := tc.f.repo.getSecrets(getCtx) - err, ok := doneErrSig.WaitWithTimeout(testTimeout) - s.Require().True(ok) + if tc.expectedErr == nil { + s.Equal(len(tc.f.secretsMap), len(secrets)) + for k, v := range tc.f.secretsMap { + s.Equal(v, secrets[k]) + } + } s.checkExpectedError(tc.expectedErr, err) }) } @@ -84,18 +75,12 @@ func (s *certSecretsRepoSuite) TestPut() { s.Run(tcName, func() { putCtx, cancelPutCtx := context.WithCancel(context.Background()) defer cancelPutCtx() - doneErrSig := concurrency.NewErrorSignal() + if tc.expectedErr == context.Canceled { + cancelPutCtx() + } - go func() { - if tc.expectedErr == context.Canceled { - cancelPutCtx() - } - err := tc.f.repo.putSecrets(putCtx, tc.f.secretsMap) - doneErrSig.SignalWithError(err) - }() + err := tc.f.repo.putSecrets(putCtx, tc.f.secretsMap) - err, ok := doneErrSig.WaitWithTimeout(testTimeout) - s.Require().True(ok) s.checkExpectedError(tc.expectedErr, err) }) } @@ -132,7 +117,7 @@ func (s *certSecretsRepoSuite) newFixture(verbToError string, secretName string) return true, &v1.Secret{}, errForced }) return &certSecretsRepoFixture{ - repo: NewCertSecretsRepo(secretsNamesMap, secretsClient), + repo: newCertSecretsRepo(secretsNamesMap, secretsClient), secretClient: secretsClient, secretsMap: secretsMap, } From 91f3db983fcdef7837b899799375fb3622205461 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 1 Feb 2022 19:15:32 +0100 Subject: [PATCH 12/49] put fails on unknown secrets and it's ok on missing secrets --- .../localscanner/secrets_repository.go | 26 ++++++++++------- .../localscanner/secrets_repository_test.go | 29 +++++++++++++++++-- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index ab1926f6b0248..5d840b7e874ff 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -15,14 +15,17 @@ var ( _ certSecretsRepo = (*certSecretsRepoImpl)(nil) ) -// certSecretsRepo is in charge of persisting and retrieving a set of secrets corresponding to service types -// into some permanent storage system, thus implementing the +// certSecretsRepo is in charge of persisting and retrieving a set of secrets corresponding to a fixed +// set of service types into k8s, thus implementing the // [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for a map from service types -// to secrets. +// to secrets and using the k8s API as persistence. type certSecretsRepo interface { // getSecrets retrieves the secrets from permanent storage. getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) // putSecrets persists the secrets on permanent storage. + // - Returns an error in case some service in `secret` is not in the set of service types handled by the repository. + // - `secrets` may miss an entry for some service type handled by the repository, in that case this only updates + // the secrets for the service types in `secrets`. putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error } @@ -64,14 +67,17 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error { var putErr error - for serviceType, secretName := range r.secretNames { - secret := secrets[serviceType] - if secret == nil { - putErr = multierror.Append(putErr, errors.Errorf("no secret found for service type %s", serviceType)) + + for serviceType, secret := range secrets { + secretName, ok := r.secretNames[serviceType] + if !ok { + putErr = multierror.Append(putErr, errors.Errorf("unkown service type %s", serviceType)) } else { - _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) - if err != nil { - putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secretName)) + if secret != nil { + _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) + if err != nil { + putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secretName)) + } } } // on context cancellation abort putting other secrets. diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 474aad98e5214..b1c891e969958 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -17,11 +17,13 @@ import ( ) const ( - namespace = "stackrox-ns" + namespace = "stackrox-ns" ) var ( - errForced = errors.New("forced error") + errForced = errors.New("forced error") + serviceType = storage.ServiceType_SCANNER_SERVICE + anotherServiceType = storage.ServiceType_SENSOR_SERVICE ) func TestCertSecretsRepo(t *testing.T) { @@ -86,6 +88,28 @@ func (s *certSecretsRepoSuite) TestPut() { } } +func (s *certSecretsRepoSuite) TestPutNilSecretSuccess() { + f := s.newFixture("", "foo") + f.secretsMap[serviceType] = nil + err := f.repo.putSecrets(context.Background(), f.secretsMap) + s.NoError(err) +} + +func (s *certSecretsRepoSuite) TestPutNoSecretSuccess() { + f := s.newFixture("", "foo") + f.secretsMap = make(map[storage.ServiceType]*v1.Secret) + err := f.repo.putSecrets(context.Background(), f.secretsMap) + s.NoError(err) +} + +func (s *certSecretsRepoSuite) TestPutUnknownSecretFail() { + f := s.newFixture("", "foo") + f.secretsMap[anotherServiceType] = f.secretsMap[serviceType] + delete(f.secretsMap, serviceType) + err := f.repo.putSecrets(context.Background(), f.secretsMap) + s.Error(err) +} + func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { if expectedErr != errForced { s.Equal(expectedErr, err) @@ -102,7 +126,6 @@ type certSecretsRepoFixture struct { } func (s *certSecretsRepoSuite) newFixture(verbToError string, secretName string) *certSecretsRepoFixture { - serviceType := storage.ServiceType_SCANNER_SERVICE secretsNamesMap := map[storage.ServiceType]string{serviceType: secretName} secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ From 4786c64a62c2ff9a2dd704489d776dac047145ca Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 2 Feb 2022 13:02:05 +0100 Subject: [PATCH 13/49] comment that put is idempotent but not atomic --- sensor/kubernetes/localscanner/secrets_repository.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 5d840b7e874ff..b6eaf86a8ae16 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -26,6 +26,8 @@ type certSecretsRepo interface { // - Returns an error in case some service in `secret` is not in the set of service types handled by the repository. // - `secrets` may miss an entry for some service type handled by the repository, in that case this only updates // the secrets for the service types in `secrets`. + // - This operation is idempotent but not atomic in sense that on error some of the secrets might be persisted + // while others are not. putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error } From 2f5772c71a005c4e2e9038d076f413dcc94084bb Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 2 Feb 2022 16:35:49 +0100 Subject: [PATCH 14/49] wip convert secrets repo into certificates repo --- .../localscanner/secrets_repository.go | 113 ++++++++++++------ .../localscanner/secrets_repository_test.go | 109 ++++++++++------- 2 files changed, 143 insertions(+), 79 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index b6eaf86a8ae16..3de4cbb827273 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -6,56 +6,91 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" + "github.com/stackrox/rox/pkg/mtls" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" ) var ( - _ certSecretsRepo = (*certSecretsRepoImpl)(nil) + _ serviceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) ) -// certSecretsRepo is in charge of persisting and retrieving a set of secrets corresponding to a fixed +// serviceCertificatesRepo is in charge of persisting and retrieving a set of secrets corresponding to a fixed // set of service types into k8s, thus implementing the // [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for a map from service types // to secrets and using the k8s API as persistence. -type certSecretsRepo interface { +type serviceCertificatesRepo interface { // getSecrets retrieves the secrets from permanent storage. - getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) + // getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) // FIXME update comments and delete + getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) // putSecrets persists the secrets on permanent storage. // - Returns an error in case some service in `secret` is not in the set of service types handled by the repository. // - `secrets` may miss an entry for some service type handled by the repository, in that case this only updates // the secrets for the service types in `secrets`. // - This operation is idempotent but not atomic in sense that on error some of the secrets might be persisted // while others are not. - putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error + // putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error // FIXME update comments and delete + putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error } -type certSecretsRepoImpl struct { - secretNames map[storage.ServiceType]string +// serviceCertificatesRepoSecretsImpl is a serviceCertificatesRepo that uses k8s secrets for persistence. +// Invariants: +// - secrets and secretsClient are read-only, but the elements of secrets are read-write. +// - All secrets store the same CA PEM. +// - No secret in secrets is nil. +type serviceCertificatesRepoSecretsImpl struct { + secrets map[storage.ServiceType]*v1.Secret secretsClient corev1.SecretInterface } -// newCertSecretsRepo creates a new certSecretsRepo that handles secrets with the specified names and +// newServiceCertificatesRepoWithSecretsPersistence creates a new serviceCertificatesRepo that handles secrets with the specified names and // for the specified service types, and uses the k8s API for persistence. -func newCertSecretsRepo(secretNames map[storage.ServiceType]string, - secretsClient corev1.SecretInterface) certSecretsRepo { - return &certSecretsRepoImpl{ - secretNames: secretNames, - secretsClient: secretsClient, +func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.ServiceType]*v1.Secret, + secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { + for serviceType, secret := range secrets { + if secret == nil { + return nil, errors.Errorf("nil secrets for service type %q", serviceType) + } } + return &serviceCertificatesRepoSecretsImpl{ + secrets: secrets, + secretsClient: secretsClient, + }, nil } -func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) { - secretsMap := make(map[storage.ServiceType]*v1.Secret, len(r.secretNames)) +// getServiceCertificates behaves as follows in case of missing data in the secrets: +// - if a secret has no data then the certificates won't contain a TypedServiceCertificate for the corresponding +// service type. +// - if the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate +// for that secret will contain a zero value. +func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { + certificates := &storage.TypedServiceCertificateSet{} + certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) var getErr error - for serviceType, secretName := range r.secretNames { - secret, err := r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) + for serviceType, secret := range r.secrets { + // Invariant: no secret in r.secrets is nil. + retrievedSecret, err := r.secretsClient.Get(ctx, secret.Name, metav1.GetOptions{}) if err != nil { - getErr = multierror.Append(getErr, errors.Wrapf(err, "for secret %s", secretName)) - } else { - secretsMap[serviceType] = secret + getErr = multierror.Append(getErr, errors.Wrapf(err, "for secret %s", secret.Name)) + continue } + secretData := retrievedSecret.Data + if secretData == nil { + continue + } + if certificates.GetCaPem() == nil { + // all secrets store the same CA PEM. + certificates.CaPem = secretData[mtls.CACertFileName] + } + certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ + ServiceType: serviceType, + Cert: &storage.ServiceCertificate{ + CertPem: secretData[mtls.ServiceCertFileName], + KeyPem: secretData[mtls.ServiceKeyFileName], + }, + }) + // on context cancellation abort getting other secrets. if ctx.Err() != nil { return nil, ctx.Err() @@ -64,24 +99,34 @@ func (r *certSecretsRepoImpl) getSecrets(ctx context.Context) (map[storage.Servi if getErr != nil { return nil, getErr } - return secretsMap, nil + + return certificates, nil } -func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error { +// putServiceCertificates edge cases: +// - Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. +// - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. +func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { var putErr error - - for serviceType, secret := range secrets { - secretName, ok := r.secretNames[serviceType] + caPem := certificates.GetCaPem() + for _, cert := range certificates.GetServiceCerts() { + secret, ok := r.secrets[cert.GetServiceType()] if !ok { - putErr = multierror.Append(putErr, errors.Errorf("unkown service type %s", serviceType)) - } else { - if secret != nil { - _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) - if err != nil { - putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secretName)) - } - } + // we don't know where to persist this. + putErr = multierror.Append(putErr, errors.Errorf("unkown service type %s", cert.GetServiceType())) + continue } + // Invariant: no secret in r.secrets is nil. + secret.Data = map[string][]byte{ + mtls.CACertFileName: caPem, + mtls.ServiceCertFileName: cert.GetCert().GetCertPem(), + mtls.ServiceKeyFileName: cert.GetCert().GetKeyPem(), + } + _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) + if err != nil { + putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secret.Name)) + } + // on context cancellation abort putting other secrets. if ctx.Err() != nil { return ctx.Err() @@ -89,4 +134,4 @@ func (r *certSecretsRepoImpl) putSecrets(ctx context.Context, secrets map[storag } return putErr -} +} \ No newline at end of file diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index b1c891e969958..8aac6ad2f27e6 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -3,9 +3,11 @@ package localscanner import ( "context" "errors" + "fmt" "testing" "github.com/stackrox/rox/generated/storage" + "github.com/stackrox/rox/pkg/mtls" "github.com/stretchr/testify/suite" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,22 +28,22 @@ var ( anotherServiceType = storage.ServiceType_SENSOR_SERVICE ) -func TestCertSecretsRepo(t *testing.T) { - suite.Run(t, new(certSecretsRepoSuite)) +func TestServiceCertificatesRepoSecretsImpl(t *testing.T) { + suite.Run(t, new(serviceCertificatesRepoSecretsImplSuite)) } -type certSecretsRepoSuite struct { +type serviceCertificatesRepoSecretsImplSuite struct { suite.Suite } -func (s *certSecretsRepoSuite) TestGet() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { testCases := map[string]struct { expectedErr error f *certSecretsRepoFixture }{ - "successful get": {nil, s.newFixture("", "foo")}, - "failed get": {errForced, s.newFixture("get", "foo")}, - "cancelled get": {context.Canceled, s.newFixture("get", "foo")}, + "successful get": {nil, s.newFixture("")}, + "failed get": {errForced, s.newFixture("get")}, + "cancelled get": {context.Canceled, s.newFixture("")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -51,27 +53,23 @@ func (s *certSecretsRepoSuite) TestGet() { cancelGetCtx() } - secrets, err := tc.f.repo.getSecrets(getCtx) - + certificates, err := tc.f.repo.getServiceCertificates(getCtx) if tc.expectedErr == nil { - s.Equal(len(tc.f.secretsMap), len(secrets)) - for k, v := range tc.f.secretsMap { - s.Equal(v, secrets[k]) - } + s.Equal(tc.f.certificates, certificates) } s.checkExpectedError(tc.expectedErr, err) }) } } -func (s *certSecretsRepoSuite) TestPut() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { testCases := map[string]struct { expectedErr error f *certSecretsRepoFixture }{ - "successful put": {nil, s.newFixture("", "foo")}, - "failed put": {errForced, s.newFixture("update", "foo")}, - "cancelled put": {context.Canceled, s.newFixture("update", "foo")}, + "successful put": {nil, s.newFixture("")}, + "failed put": {errForced, s.newFixture("update")}, + "cancelled put": {context.Canceled, s.newFixture("")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -81,36 +79,38 @@ func (s *certSecretsRepoSuite) TestPut() { cancelPutCtx() } - err := tc.f.repo.putSecrets(putCtx, tc.f.secretsMap) + err := tc.f.repo.putServiceCertificates(putCtx, tc.f.certificates) s.checkExpectedError(tc.expectedErr, err) }) } } -func (s *certSecretsRepoSuite) TestPutNilSecretSuccess() { - f := s.newFixture("", "foo") - f.secretsMap[serviceType] = nil - err := f.repo.putSecrets(context.Background(), f.secretsMap) - s.NoError(err) +func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailure() { + f := s.newFixture("") + var secret *v1.Secret + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} + _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) + s.Error(err) } -func (s *certSecretsRepoSuite) TestPutNoSecretSuccess() { - f := s.newFixture("", "foo") - f.secretsMap = make(map[storage.ServiceType]*v1.Secret) - err := f.repo.putSecrets(context.Background(), f.secretsMap) - s.NoError(err) +func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { + f := s.newFixture("") + certificates := f.certificates.GetServiceCerts() + s.Require().Len(certificates, 1) + certificates[0].ServiceType = anotherServiceType + err := f.repo.putServiceCertificates(context.Background(), f.certificates) + s.Error(err) } -func (s *certSecretsRepoSuite) TestPutUnknownSecretFail() { - f := s.newFixture("", "foo") - f.secretsMap[anotherServiceType] = f.secretsMap[serviceType] - delete(f.secretsMap, serviceType) - err := f.repo.putSecrets(context.Background(), f.secretsMap) - s.Error(err) +func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { + f := s.newFixture("") + f.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + err := f.repo.putServiceCertificates(context.Background(), f.certificates) + s.NoError(err) } -func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { +func (s *serviceCertificatesRepoSecretsImplSuite) checkExpectedError(expectedErr, err error) { if expectedErr != errForced { s.Equal(expectedErr, err) } else { @@ -120,28 +120,47 @@ func (s *certSecretsRepoSuite) checkExpectedError(expectedErr, err error) { } type certSecretsRepoFixture struct { - repo certSecretsRepo - secretClient corev1.SecretInterface - secretsMap map[storage.ServiceType]*v1.Secret + repo serviceCertificatesRepo + secretsClient corev1.SecretInterface + certificates *storage.TypedServiceCertificateSet } -func (s *certSecretsRepoSuite) newFixture(verbToError string, secretName string) *certSecretsRepoFixture { - secretsNamesMap := map[storage.ServiceType]string{serviceType: secretName} +func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(verbToError string) *certSecretsRepoFixture { + serviceCertificate := &storage.TypedServiceCertificate{ + ServiceType: serviceType, + Cert: &storage.ServiceCertificate{ + CertPem: make([]byte, 0), + KeyPem: make([]byte, 1), + }, + } + certificates := &storage.TypedServiceCertificateSet{ + CaPem: make([]byte, 2), + ServiceCerts: []*storage.TypedServiceCertificate{ + serviceCertificate, + }, + } secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretName, + Name: fmt.Sprintf("%s-secret", serviceType), Namespace: namespace, }, + Data: map[string][]byte{ + mtls.CACertFileName: certificates.GetCaPem(), + mtls.ServiceCertFileName: serviceCertificate.GetCert().GetCertPem(), + mtls.ServiceKeyFileName: serviceCertificate.GetCert().GetKeyPem(), + }, } - secretsMap := map[storage.ServiceType]*v1.Secret{serviceType: secret} + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} clientSet := fake.NewSimpleClientset(secret) secretsClient := clientSet.CoreV1().Secrets(namespace) clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) + repo, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, secretsClient) + s.Require().NoError(err) return &certSecretsRepoFixture{ - repo: newCertSecretsRepo(secretsNamesMap, secretsClient), - secretClient: secretsClient, - secretsMap: secretsMap, + repo: repo, + secretsClient: secretsClient, + certificates: certificates, } } From 8f8417671a1d33ff2b581274a08c0aab9762d877 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 2 Feb 2022 17:09:08 +0100 Subject: [PATCH 15/49] add unit tests --- .../localscanner/secrets_repository.go | 8 +- .../localscanner/secrets_repository_test.go | 77 +++++++++++++++++-- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/secrets_repository.go index 3de4cbb827273..3f79f2fcd7243 100644 --- a/sensor/kubernetes/localscanner/secrets_repository.go +++ b/sensor/kubernetes/localscanner/secrets_repository.go @@ -40,7 +40,7 @@ type serviceCertificatesRepo interface { // - All secrets store the same CA PEM. // - No secret in secrets is nil. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]*v1.Secret + secrets map[storage.ServiceType]*v1.Secret secretsClient corev1.SecretInterface } @@ -54,7 +54,7 @@ func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.Servic } } return &serviceCertificatesRepoSecretsImpl{ - secrets: secrets, + secrets: secrets, secretsClient: secretsClient, }, nil } @@ -87,7 +87,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. ServiceType: serviceType, Cert: &storage.ServiceCertificate{ CertPem: secretData[mtls.ServiceCertFileName], - KeyPem: secretData[mtls.ServiceKeyFileName], + KeyPem: secretData[mtls.ServiceKeyFileName], }, }) @@ -134,4 +134,4 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. } return putErr -} \ No newline at end of file +} diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/secrets_repository_test.go index 8aac6ad2f27e6..1e946f27c643e 100644 --- a/sensor/kubernetes/localscanner/secrets_repository_test.go +++ b/sensor/kubernetes/localscanner/secrets_repository_test.go @@ -94,11 +94,55 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailur s.Error(err) } +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { + f := s.newFixtureAdvancedOpts("", true) + expectedCertificates := &storage.TypedServiceCertificateSet{} + expectedCertificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + + certificates, err := f.repo.getServiceCertificates(context.Background()) + + s.NoError(err) + s.Equal(expectedCertificates, certificates) +} + +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSuccess() { + testCases := map[string]struct { + missingSecretDataKey string + setExpectedCertsFunc func(certificates *storage.TypedServiceCertificateSet) + }{ + "missing CA": { + missingSecretDataKey: mtls.CACertFileName, + setExpectedCertsFunc: func(certificates *storage.TypedServiceCertificateSet) { + certificates.CaPem = nil + }}, + "missing Cert": { + missingSecretDataKey: mtls.ServiceCertFileName, + setExpectedCertsFunc: func(certificates *storage.TypedServiceCertificateSet) { + s.getFirstServiceCertificate(certificates).Cert.CertPem = nil + }, + }, + "missing Key": { + missingSecretDataKey: mtls.ServiceKeyFileName, + setExpectedCertsFunc: func(certificates *storage.TypedServiceCertificateSet) { + s.getFirstServiceCertificate(certificates).Cert.KeyPem = nil + }, + }, + } + for tcName, tc := range testCases { + s.Run(tcName, func() { + f := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) + certificates, err := f.repo.getServiceCertificates(context.Background()) + tc.setExpectedCertsFunc(f.certificates) + + s.NoError(err) + s.Equal(f.certificates, certificates) + }) + } +} + func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { f := s.newFixture("") - certificates := f.certificates.GetServiceCerts() - s.Require().Len(certificates, 1) - certificates[0].ServiceType = anotherServiceType + s.getFirstServiceCertificate(f.certificates).ServiceType = anotherServiceType err := f.repo.putServiceCertificates(context.Background(), f.certificates) s.Error(err) } @@ -119,18 +163,30 @@ func (s *serviceCertificatesRepoSecretsImplSuite) checkExpectedError(expectedErr } } +func (s *serviceCertificatesRepoSecretsImplSuite) getFirstServiceCertificate( + certificates *storage.TypedServiceCertificateSet) *storage.TypedServiceCertificate { + serviceCerts := certificates.GetServiceCerts() + s.Require().Len(serviceCerts, 1) + return serviceCerts[0] +} + type certSecretsRepoFixture struct { repo serviceCertificatesRepo secretsClient corev1.SecretInterface - certificates *storage.TypedServiceCertificateSet + certificates *storage.TypedServiceCertificateSet } func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(verbToError string) *certSecretsRepoFixture { - serviceCertificate := &storage.TypedServiceCertificate{ + return s.newFixtureAdvancedOpts(verbToError, false) +} + +func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToError string, emptySecretData bool, + missingSecretDataKeys ...string) *certSecretsRepoFixture { + serviceCertificate := &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ CertPem: make([]byte, 0), - KeyPem: make([]byte, 1), + KeyPem: make([]byte, 1), }, } certificates := &storage.TypedServiceCertificateSet{ @@ -144,11 +200,16 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(verbToError string) Name: fmt.Sprintf("%s-secret", serviceType), Namespace: namespace, }, - Data: map[string][]byte{ + } + if !emptySecretData { + secret.Data = map[string][]byte{ mtls.CACertFileName: certificates.GetCaPem(), mtls.ServiceCertFileName: serviceCertificate.GetCert().GetCertPem(), mtls.ServiceKeyFileName: serviceCertificate.GetCert().GetKeyPem(), - }, + } + } + for _, secretDataKey := range missingSecretDataKeys { + delete(secret.Data, secretDataKey) } secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} clientSet := fake.NewSimpleClientset(secret) From f411d162511fa41a190445c3b31d56aa1a30edf9 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 2 Feb 2022 17:14:20 +0100 Subject: [PATCH 16/49] rename repository files accordingly to refactor --- .../{secrets_repository.go => service_certificates_repository.go} | 0 ...repository_test.go => service_certificates_repository_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename sensor/kubernetes/localscanner/{secrets_repository.go => service_certificates_repository.go} (100%) rename sensor/kubernetes/localscanner/{secrets_repository_test.go => service_certificates_repository_test.go} (100%) diff --git a/sensor/kubernetes/localscanner/secrets_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go similarity index 100% rename from sensor/kubernetes/localscanner/secrets_repository.go rename to sensor/kubernetes/localscanner/service_certificates_repository.go diff --git a/sensor/kubernetes/localscanner/secrets_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go similarity index 100% rename from sensor/kubernetes/localscanner/secrets_repository_test.go rename to sensor/kubernetes/localscanner/service_certificates_repository_test.go From 6de90b69d7fd01f0ce75da6bc3f2922a15df3eb3 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 2 Feb 2022 17:49:31 +0100 Subject: [PATCH 17/49] improve comments and additional input validation --- .../service_certificates_repository.go | 46 +++++++++++-------- .../service_certificates_repository_test.go | 30 ++++++++++++ 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 3f79f2fcd7243..ab0d76b07b62f 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -2,6 +2,7 @@ package localscanner import ( "context" + "strings" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" @@ -16,42 +17,47 @@ var ( _ serviceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) ) -// serviceCertificatesRepo is in charge of persisting and retrieving a set of secrets corresponding to a fixed -// set of service types into k8s, thus implementing the -// [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for a map from service types -// to secrets and using the k8s API as persistence. +// serviceCertificatesRepo is in charge of persisting and retrieving a set of service certificates, thus implementing +// the [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for *storage.TypedServiceCertificateSet. type serviceCertificatesRepo interface { - // getSecrets retrieves the secrets from permanent storage. - // getSecrets(ctx context.Context) (map[storage.ServiceType]*v1.Secret, error) // FIXME update comments and delete + // getServiceCertificates retrieves the certificates from permanent storage. getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) - // putSecrets persists the secrets on permanent storage. - // - Returns an error in case some service in `secret` is not in the set of service types handled by the repository. - // - `secrets` may miss an entry for some service type handled by the repository, in that case this only updates - // the secrets for the service types in `secrets`. - // - This operation is idempotent but not atomic in sense that on error some of the secrets might be persisted - // while others are not. - // putSecrets(ctx context.Context, secrets map[storage.ServiceType]*v1.Secret) error // FIXME update comments and delete + // putServiceCertificates persists the certificates on permanent storage. putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error } // serviceCertificatesRepoSecretsImpl is a serviceCertificatesRepo that uses k8s secrets for persistence. +// All fields except Data are respected when persisting the secrets. // Invariants: -// - secrets and secretsClient are read-only, but the elements of secrets are read-write. -// - All secrets store the same CA PEM. +// - secrets and secretsClient are read-only, except the field Data of the entries in secrets. // - No secret in secrets is nil. +// - All secrets with non nil Data store the same CA PEM. type serviceCertificatesRepoSecretsImpl struct { secrets map[storage.ServiceType]*v1.Secret secretsClient corev1.SecretInterface } -// newServiceCertificatesRepoWithSecretsPersistence creates a new serviceCertificatesRepo that handles secrets with the specified names and -// for the specified service types, and uses the k8s API for persistence. +// newServiceCertificatesRepoWithSecretsPersistence creates a new serviceCertificatesRepoSecretsImpl that persists +// certificates for the specified services in the corresponding k8s secrets. func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { + caMap := make(map[string]storage.ServiceType) for serviceType, secret := range secrets { if secret == nil { return nil, errors.Errorf("nil secrets for service type %q", serviceType) } + if secretData := secret.Data; secretData != nil { + caPem := secretData[mtls.CACertFileName] + caMap[string(caPem)] = serviceType + } + } + if len(caMap) > 1 { + serviceTypes := make([]string, 0) + for _, serviceType := range caMap { + serviceTypes = append(serviceTypes, serviceType.String()) + } + return nil, errors.Errorf("found different CA PEM in secret Data for service types %q", + strings.Join(serviceTypes, ",")) } return &serviceCertificatesRepoSecretsImpl{ secrets: secrets, @@ -80,7 +86,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. continue } if certificates.GetCaPem() == nil { - // all secrets store the same CA PEM. + // all secrets with non nil Data store the same CA PEM. certificates.CaPem = secretData[mtls.CACertFileName] } certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ @@ -103,7 +109,9 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. return certificates, nil } -// putServiceCertificates edge cases: +// putServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted +// while others are not. +// Edge cases: // - Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. // - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 1e946f27c643e..af22242789a91 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -94,6 +94,36 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailur s.Error(err) } +func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithDifferentCASecretDataFailure() { + f := s.newFixture("") + secret1 := &v1.Secret{ + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 0), + }, + } + secret2 := &v1.Secret{ + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 1), + }, + } + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} + _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) + s.Error(err) +} + +func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCASecretDataSuccess() { + f := s.newFixture("") + secret1 := &v1.Secret{ + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 0), + }, + } + secret2 := &v1.Secret{} + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} + _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) + s.NoError(err) +} + func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { f := s.newFixtureAdvancedOpts("", true) expectedCertificates := &storage.TypedServiceCertificateSet{} From f1b6e8c47ed2b1794dd767088ad26e5737ad9103 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 3 Feb 2022 16:52:51 +0100 Subject: [PATCH 18/49] check same CA is retrieved on get --- .../service_certificates_repository.go | 24 ++++------ .../service_certificates_repository_test.go | 45 ++++++++++++------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index ab0d76b07b62f..cee6ac960e2f6 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -1,8 +1,8 @@ package localscanner import ( + "bytes" "context" - "strings" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" @@ -31,7 +31,6 @@ type serviceCertificatesRepo interface { // Invariants: // - secrets and secretsClient are read-only, except the field Data of the entries in secrets. // - No secret in secrets is nil. -// - All secrets with non nil Data store the same CA PEM. type serviceCertificatesRepoSecretsImpl struct { secrets map[storage.ServiceType]*v1.Secret secretsClient corev1.SecretInterface @@ -41,23 +40,10 @@ type serviceCertificatesRepoSecretsImpl struct { // certificates for the specified services in the corresponding k8s secrets. func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { - caMap := make(map[string]storage.ServiceType) for serviceType, secret := range secrets { if secret == nil { return nil, errors.Errorf("nil secrets for service type %q", serviceType) } - if secretData := secret.Data; secretData != nil { - caPem := secretData[mtls.CACertFileName] - caMap[string(caPem)] = serviceType - } - } - if len(caMap) > 1 { - serviceTypes := make([]string, 0) - for _, serviceType := range caMap { - serviceTypes = append(serviceTypes, serviceType.String()) - } - return nil, errors.Errorf("found different CA PEM in secret Data for service types %q", - strings.Join(serviceTypes, ",")) } return &serviceCertificatesRepoSecretsImpl{ secrets: secrets, @@ -73,6 +59,7 @@ func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.Servic func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + var firstServiceTypeWithCA storage.ServiceType var getErr error for serviceType, secret := range r.secrets { // Invariant: no secret in r.secrets is nil. @@ -86,8 +73,13 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. continue } if certificates.GetCaPem() == nil { - // all secrets with non nil Data store the same CA PEM. certificates.CaPem = secretData[mtls.CACertFileName] + firstServiceTypeWithCA = serviceType + } else { + if !bytes.Equal(certificates.GetCaPem(), secretData[mtls.CACertFileName]) { + return nil, errors.Errorf("found different CA PEM in secret Data for service types %q and %q", + firstServiceTypeWithCA, serviceType) + } } certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ ServiceType: serviceType, diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index af22242789a91..ac92038e3e392 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -62,6 +62,34 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { } } +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferenteCAsFailure() { + secret1 := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret1", + Namespace: namespace, + }, + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 0), + }, + } + secret2 := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret2", + Namespace: namespace, + }, + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 1), + }, + } + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} + clientSet := fake.NewSimpleClientset(secret1, secret2) + secretsClient := clientSet.CoreV1().Secrets(namespace) + repo, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, secretsClient) + s.Require().NoError(err) + _, err = repo.getServiceCertificates(context.Background()) + s.Require().Error(err) +} + func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { testCases := map[string]struct { expectedErr error @@ -94,23 +122,6 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailur s.Error(err) } -func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithDifferentCASecretDataFailure() { - f := s.newFixture("") - secret1 := &v1.Secret{ - Data: map[string][]byte{ - mtls.CACertFileName: make([]byte, 0), - }, - } - secret2 := &v1.Secret{ - Data: map[string][]byte{ - mtls.CACertFileName: make([]byte, 1), - }, - } - secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) - s.Error(err) -} - func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCASecretDataSuccess() { f := s.newFixture("") secret1 := &v1.Secret{ From 9b6a6dc2390e86223c72a410a5ef92b619fd9335 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 3 Feb 2022 16:55:23 +0100 Subject: [PATCH 19/49] simplify factory function for repo --- .../localscanner/service_certificates_repository.go | 4 ++-- .../localscanner/service_certificates_repository_test.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index cee6ac960e2f6..286b921d78801 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -36,9 +36,9 @@ type serviceCertificatesRepoSecretsImpl struct { secretsClient corev1.SecretInterface } -// newServiceCertificatesRepoWithSecretsPersistence creates a new serviceCertificatesRepoSecretsImpl that persists +// newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists // certificates for the specified services in the corresponding k8s secrets. -func newServiceCertificatesRepoWithSecretsPersistence(secrets map[storage.ServiceType]*v1.Secret, +func newServiceCertificatesRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { for serviceType, secret := range secrets { if secret == nil { diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index ac92038e3e392..1a0dde758fb07 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -84,7 +84,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferenteCAsFailure() secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) - repo, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, secretsClient) + repo, err := newServiceCertificatesRepo(secrets, secretsClient) s.Require().NoError(err) _, err = repo.getServiceCertificates(context.Background()) s.Require().Error(err) @@ -118,7 +118,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailur f := s.newFixture("") var secret *v1.Secret secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} - _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) + _, err := newServiceCertificatesRepo(secrets, f.secretsClient) s.Error(err) } @@ -131,7 +131,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCAS } secret2 := &v1.Secret{} secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - _, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, f.secretsClient) + _, err := newServiceCertificatesRepo(secrets, f.secretsClient) s.NoError(err) } @@ -258,7 +258,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) - repo, err := newServiceCertificatesRepoWithSecretsPersistence(secrets, secretsClient) + repo, err := newServiceCertificatesRepo(secrets, secretsClient) s.Require().NoError(err) return &certSecretsRepoFixture{ repo: repo, From 7bb589edd5e0436e7adce1d21ac32551ad9433e3 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 3 Feb 2022 17:05:22 +0100 Subject: [PATCH 20/49] add parameters for secrets file paths --- .../service_certificates_repository.go | 32 +++++++++++-------- .../service_certificates_repository_test.go | 13 +++++--- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 286b921d78801..0aca92e6dedb4 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -7,7 +7,6 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" - "github.com/stackrox/rox/pkg/mtls" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -32,13 +31,17 @@ type serviceCertificatesRepo interface { // - secrets and secretsClient are read-only, except the field Data of the entries in secrets. // - No secret in secrets is nil. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]*v1.Secret - secretsClient corev1.SecretInterface + secrets map[storage.ServiceType]*v1.Secret + secretsClient corev1.SecretInterface + caCertFileName string + serviceCertFileName string + serviceKeyFileName string } // newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists // certificates for the specified services in the corresponding k8s secrets. -func newServiceCertificatesRepo(secrets map[storage.ServiceType]*v1.Secret, +func newServiceCertificatesRepo(caCertFileName, serviceCertFileName, serviceKeyFileName string, + secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { for serviceType, secret := range secrets { if secret == nil { @@ -46,8 +49,11 @@ func newServiceCertificatesRepo(secrets map[storage.ServiceType]*v1.Secret, } } return &serviceCertificatesRepoSecretsImpl{ - secrets: secrets, - secretsClient: secretsClient, + secrets: secrets, + secretsClient: secretsClient, + caCertFileName: caCertFileName, + serviceCertFileName: serviceCertFileName, + serviceKeyFileName: serviceKeyFileName, }, nil } @@ -73,10 +79,10 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. continue } if certificates.GetCaPem() == nil { - certificates.CaPem = secretData[mtls.CACertFileName] + certificates.CaPem = secretData[r.caCertFileName] firstServiceTypeWithCA = serviceType } else { - if !bytes.Equal(certificates.GetCaPem(), secretData[mtls.CACertFileName]) { + if !bytes.Equal(certificates.GetCaPem(), secretData[r.caCertFileName]) { return nil, errors.Errorf("found different CA PEM in secret Data for service types %q and %q", firstServiceTypeWithCA, serviceType) } @@ -84,8 +90,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ - CertPem: secretData[mtls.ServiceCertFileName], - KeyPem: secretData[mtls.ServiceKeyFileName], + CertPem: secretData[r.serviceCertFileName], + KeyPem: secretData[r.serviceKeyFileName], }, }) @@ -118,9 +124,9 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. } // Invariant: no secret in r.secrets is nil. secret.Data = map[string][]byte{ - mtls.CACertFileName: caPem, - mtls.ServiceCertFileName: cert.GetCert().GetCertPem(), - mtls.ServiceKeyFileName: cert.GetCert().GetKeyPem(), + r.caCertFileName: caPem, + r.serviceCertFileName: cert.GetCert().GetCertPem(), + r.serviceKeyFileName: cert.GetCert().GetKeyPem(), } _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) if err != nil { diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 1a0dde758fb07..a75efb7eae9c6 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -84,7 +84,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferenteCAsFailure() secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) - repo, err := newServiceCertificatesRepo(secrets, secretsClient) + repo, err := newTestRepo(secrets, secretsClient) s.Require().NoError(err) _, err = repo.getServiceCertificates(context.Background()) s.Require().Error(err) @@ -118,7 +118,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailur f := s.newFixture("") var secret *v1.Secret secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} - _, err := newServiceCertificatesRepo(secrets, f.secretsClient) + _, err := newTestRepo(secrets, f.secretsClient) s.Error(err) } @@ -131,7 +131,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCAS } secret2 := &v1.Secret{} secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - _, err := newServiceCertificatesRepo(secrets, f.secretsClient) + _, err := newTestRepo(secrets, f.secretsClient) s.NoError(err) } @@ -258,7 +258,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) - repo, err := newServiceCertificatesRepo(secrets, secretsClient) + repo, err := newTestRepo(secrets, secretsClient) s.Require().NoError(err) return &certSecretsRepoFixture{ repo: repo, @@ -266,3 +266,8 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE certificates: certificates, } } + +func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { + return newServiceCertificatesRepo(mtls.CACertFileName, mtls.ServiceCertFileName, mtls.ServiceKeyFileName, + secrets, secretsClient) +} From d0747afdbbf10d4fcb5e2a87b665e1905adee9c1 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 3 Feb 2022 17:35:26 +0100 Subject: [PATCH 21/49] move body of put and get certificates loop to separte methods --- .../service_certificates_repository.go | 99 +++++++++++-------- .../service_certificates_repository_test.go | 63 +++++++----- 2 files changed, 97 insertions(+), 65 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 0aca92e6dedb4..e76bdd56d5b67 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -68,33 +68,9 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. var firstServiceTypeWithCA storage.ServiceType var getErr error for serviceType, secret := range r.secrets { - // Invariant: no secret in r.secrets is nil. - retrievedSecret, err := r.secretsClient.Get(ctx, secret.Name, metav1.GetOptions{}) - if err != nil { - getErr = multierror.Append(getErr, errors.Wrapf(err, "for secret %s", secret.Name)) - continue + if err := r.getServiceCertificate(ctx, serviceType, secret, certificates, &firstServiceTypeWithCA); err != nil { + getErr = multierror.Append(getErr, err) } - secretData := retrievedSecret.Data - if secretData == nil { - continue - } - if certificates.GetCaPem() == nil { - certificates.CaPem = secretData[r.caCertFileName] - firstServiceTypeWithCA = serviceType - } else { - if !bytes.Equal(certificates.GetCaPem(), secretData[r.caCertFileName]) { - return nil, errors.Errorf("found different CA PEM in secret Data for service types %q and %q", - firstServiceTypeWithCA, serviceType) - } - } - certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ - ServiceType: serviceType, - Cert: &storage.ServiceCertificate{ - CertPem: secretData[r.serviceCertFileName], - KeyPem: secretData[r.serviceKeyFileName], - }, - }) - // on context cancellation abort getting other secrets. if ctx.Err() != nil { return nil, ctx.Err() @@ -107,6 +83,39 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. return certificates, nil } +func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, + serviceType storage.ServiceType, secret *v1.Secret, + certificates *storage.TypedServiceCertificateSet, + firstServiceTypeWithCA *storage.ServiceType) error { + // Invariant: no secret in r.secrets is nil. + retrievedSecret, err := r.secretsClient.Get(ctx, secret.Name, metav1.GetOptions{}) + if err != nil { + return errors.Wrapf(err, "for service type %q", serviceType) + } + secretData := retrievedSecret.Data + if secretData == nil { + return errors.Wrapf(err, "missing for secret data for service type %q", serviceType) + } + if certificates.GetCaPem() == nil { + certificates.CaPem = secretData[r.caCertFileName] + *firstServiceTypeWithCA = serviceType + } else { + if !bytes.Equal(certificates.GetCaPem(), secretData[r.caCertFileName]) { + return errors.Errorf("found different CA PEM in secret Data for service types %q and %q", + firstServiceTypeWithCA, serviceType) + } + } + certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ + ServiceType: serviceType, + Cert: &storage.ServiceCertificate{ + CertPem: secretData[r.serviceCertFileName], + KeyPem: secretData[r.serviceKeyFileName], + }, + }) + + return nil +} + // putServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted // while others are not. // Edge cases: @@ -116,21 +125,8 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. var putErr error caPem := certificates.GetCaPem() for _, cert := range certificates.GetServiceCerts() { - secret, ok := r.secrets[cert.GetServiceType()] - if !ok { - // we don't know where to persist this. - putErr = multierror.Append(putErr, errors.Errorf("unkown service type %s", cert.GetServiceType())) - continue - } - // Invariant: no secret in r.secrets is nil. - secret.Data = map[string][]byte{ - r.caCertFileName: caPem, - r.serviceCertFileName: cert.GetCert().GetCertPem(), - r.serviceKeyFileName: cert.GetCert().GetKeyPem(), - } - _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}) - if err != nil { - putErr = multierror.Append(putErr, errors.Wrapf(err, "for secret %s", secret.Name)) + if err := r.putServiceCertificate(ctx, caPem, cert); err != nil { + putErr = multierror.Append(putErr, err) } // on context cancellation abort putting other secrets. @@ -141,3 +137,24 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. return putErr } + +func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, + cert *storage.TypedServiceCertificate) error { + + secret, ok := r.secrets[cert.GetServiceType()] + if !ok { + // we don't know where to persist this. + return errors.Errorf("unkown service type %q", cert.GetServiceType()) + } + // Invariant: no secret in r.secrets is nil. + secret.Data = map[string][]byte{ + r.caCertFileName: caPem, + r.serviceCertFileName: cert.GetCert().GetCertPem(), + r.serviceKeyFileName: cert.GetCert().GetKeyPem(), + } + if _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}); err != nil { + return errors.Wrapf(err, "for service type %q", cert.GetServiceType()) + } + + return nil +} diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index a75efb7eae9c6..379c370f75c35 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -62,32 +62,47 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { } } -func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferenteCAsFailure() { - secret1 := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "secret1", - Namespace: namespace, - }, - Data: map[string][]byte{ - mtls.CACertFileName: make([]byte, 0), - }, +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { + testCases := map[string]struct { + expectError bool + secondCASize int + }{ + "same CAs successful get": {false, 0}, + "different CAs failed get": {true, 1}, } - secret2 := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "secret2", - Namespace: namespace, - }, - Data: map[string][]byte{ - mtls.CACertFileName: make([]byte, 1), - }, + for tcName, tc := range testCases { + s.Run(tcName, func() { + secret1 := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret1", + Namespace: namespace, + }, + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, 0), + }, + } + secret2 := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret2", + Namespace: namespace, + }, + Data: map[string][]byte{ + mtls.CACertFileName: make([]byte, tc.secondCASize), + }, + } + secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} + clientSet := fake.NewSimpleClientset(secret1, secret2) + secretsClient := clientSet.CoreV1().Secrets(namespace) + repo, err := newTestRepo(secrets, secretsClient) + s.Require().NoError(err) + _, err = repo.getServiceCertificates(context.Background()) + if tc.expectError { + s.Error(err) + } else { + s.NoError(err) + } + }) } - secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - clientSet := fake.NewSimpleClientset(secret1, secret2) - secretsClient := clientSet.CoreV1().Secrets(namespace) - repo, err := newTestRepo(secrets, secretsClient) - s.Require().NoError(err) - _, err = repo.getServiceCertificates(context.Background()) - s.Require().Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { From 28547245189857c74dbd386996d55098d28bf24f Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 3 Feb 2022 17:40:56 +0100 Subject: [PATCH 22/49] rename `f` to `fixture` to improve readability --- .../service_certificates_repository_test.go | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 379c370f75c35..f8e9519e99975 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -39,7 +39,7 @@ type serviceCertificatesRepoSecretsImplSuite struct { func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { testCases := map[string]struct { expectedErr error - f *certSecretsRepoFixture + fixture *certSecretsRepoFixture }{ "successful get": {nil, s.newFixture("")}, "failed get": {errForced, s.newFixture("get")}, @@ -53,9 +53,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { cancelGetCtx() } - certificates, err := tc.f.repo.getServiceCertificates(getCtx) + certificates, err := tc.fixture.repo.getServiceCertificates(getCtx) if tc.expectedErr == nil { - s.Equal(tc.f.certificates, certificates) + s.Equal(tc.fixture.certificates, certificates) } s.checkExpectedError(tc.expectedErr, err) }) @@ -108,7 +108,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { testCases := map[string]struct { expectedErr error - f *certSecretsRepoFixture + fixture *certSecretsRepoFixture }{ "successful put": {nil, s.newFixture("")}, "failed put": {errForced, s.newFixture("update")}, @@ -122,7 +122,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { cancelPutCtx() } - err := tc.f.repo.putServiceCertificates(putCtx, tc.f.certificates) + err := tc.fixture.repo.putServiceCertificates(putCtx, tc.fixture.certificates) s.checkExpectedError(tc.expectedErr, err) }) @@ -130,15 +130,15 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { } func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailure() { - f := s.newFixture("") + fixture := s.newFixture("") var secret *v1.Secret secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} - _, err := newTestRepo(secrets, f.secretsClient) + _, err := newTestRepo(secrets, fixture.secretsClient) s.Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCASecretDataSuccess() { - f := s.newFixture("") + fixture := s.newFixture("") secret1 := &v1.Secret{ Data: map[string][]byte{ mtls.CACertFileName: make([]byte, 0), @@ -146,16 +146,16 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCAS } secret2 := &v1.Secret{} secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - _, err := newTestRepo(secrets, f.secretsClient) + _, err := newTestRepo(secrets, fixture.secretsClient) s.NoError(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { - f := s.newFixtureAdvancedOpts("", true) + fixture := s.newFixtureAdvancedOpts("", true) expectedCertificates := &storage.TypedServiceCertificateSet{} expectedCertificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - certificates, err := f.repo.getServiceCertificates(context.Background()) + certificates, err := fixture.repo.getServiceCertificates(context.Background()) s.NoError(err) s.Equal(expectedCertificates, certificates) @@ -186,27 +186,27 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } for tcName, tc := range testCases { s.Run(tcName, func() { - f := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) - certificates, err := f.repo.getServiceCertificates(context.Background()) - tc.setExpectedCertsFunc(f.certificates) + fixture := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) + certificates, err := fixture.repo.getServiceCertificates(context.Background()) + tc.setExpectedCertsFunc(fixture.certificates) s.NoError(err) - s.Equal(f.certificates, certificates) + s.Equal(fixture.certificates, certificates) }) } } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { - f := s.newFixture("") - s.getFirstServiceCertificate(f.certificates).ServiceType = anotherServiceType - err := f.repo.putServiceCertificates(context.Background(), f.certificates) + fixture := s.newFixture("") + s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) s.Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { - f := s.newFixture("") - f.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - err := f.repo.putServiceCertificates(context.Background(), f.certificates) + fixture := s.newFixture("") + fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) s.NoError(err) } From ccdfe68633d67263c9d0f45f5b5dffbb0522cf7a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 4 Feb 2022 17:00:11 +0100 Subject: [PATCH 23/49] allow different certificate file path per service type --- .../service_certificates_repository.go | 48 +++++++++---------- .../service_certificates_repository_test.go | 13 ++++- 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index e76bdd56d5b67..b6543c4da2de9 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -31,8 +31,12 @@ type serviceCertificatesRepo interface { // - secrets and secretsClient are read-only, except the field Data of the entries in secrets. // - No secret in secrets is nil. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]*v1.Secret - secretsClient corev1.SecretInterface + secrets map[storage.ServiceType]serviceCertificateSecret + secretsClient corev1.SecretInterface +} + +type serviceCertificateSecret struct { + secret *v1.Secret caCertFileName string serviceCertFileName string serviceKeyFileName string @@ -40,20 +44,16 @@ type serviceCertificatesRepoSecretsImpl struct { // newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists // certificates for the specified services in the corresponding k8s secrets. -func newServiceCertificatesRepo(caCertFileName, serviceCertFileName, serviceKeyFileName string, - secrets map[storage.ServiceType]*v1.Secret, +func newServiceCertificatesRepo(secrets map[storage.ServiceType]serviceCertificateSecret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { for serviceType, secret := range secrets { - if secret == nil { + if secret.secret == nil { return nil, errors.Errorf("nil secrets for service type %q", serviceType) } } return &serviceCertificatesRepoSecretsImpl{ - secrets: secrets, - secretsClient: secretsClient, - caCertFileName: caCertFileName, - serviceCertFileName: serviceCertFileName, - serviceKeyFileName: serviceKeyFileName, + secrets: secrets, + secretsClient: secretsClient, }, nil } @@ -67,8 +67,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) var firstServiceTypeWithCA storage.ServiceType var getErr error - for serviceType, secret := range r.secrets { - if err := r.getServiceCertificate(ctx, serviceType, secret, certificates, &firstServiceTypeWithCA); err != nil { + for serviceType, secretInfo := range r.secrets { + if err := r.getServiceCertificate(ctx, serviceType, secretInfo, certificates, &firstServiceTypeWithCA); err != nil { getErr = multierror.Append(getErr, err) } // on context cancellation abort getting other secrets. @@ -84,11 +84,11 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. } func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, - serviceType storage.ServiceType, secret *v1.Secret, + serviceType storage.ServiceType, secretInfo serviceCertificateSecret, certificates *storage.TypedServiceCertificateSet, firstServiceTypeWithCA *storage.ServiceType) error { // Invariant: no secret in r.secrets is nil. - retrievedSecret, err := r.secretsClient.Get(ctx, secret.Name, metav1.GetOptions{}) + retrievedSecret, err := r.secretsClient.Get(ctx, secretInfo.secret.Name, metav1.GetOptions{}) if err != nil { return errors.Wrapf(err, "for service type %q", serviceType) } @@ -97,10 +97,10 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C return errors.Wrapf(err, "missing for secret data for service type %q", serviceType) } if certificates.GetCaPem() == nil { - certificates.CaPem = secretData[r.caCertFileName] + certificates.CaPem = secretData[secretInfo.caCertFileName] *firstServiceTypeWithCA = serviceType } else { - if !bytes.Equal(certificates.GetCaPem(), secretData[r.caCertFileName]) { + if !bytes.Equal(certificates.GetCaPem(), secretData[secretInfo.caCertFileName]) { return errors.Errorf("found different CA PEM in secret Data for service types %q and %q", firstServiceTypeWithCA, serviceType) } @@ -108,8 +108,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ - CertPem: secretData[r.serviceCertFileName], - KeyPem: secretData[r.serviceKeyFileName], + CertPem: secretData[secretInfo.serviceCertFileName], + KeyPem: secretData[secretInfo.serviceKeyFileName], }, }) @@ -141,18 +141,18 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, cert *storage.TypedServiceCertificate) error { - secret, ok := r.secrets[cert.GetServiceType()] + secretInfo, ok := r.secrets[cert.GetServiceType()] if !ok { // we don't know where to persist this. return errors.Errorf("unkown service type %q", cert.GetServiceType()) } // Invariant: no secret in r.secrets is nil. - secret.Data = map[string][]byte{ - r.caCertFileName: caPem, - r.serviceCertFileName: cert.GetCert().GetCertPem(), - r.serviceKeyFileName: cert.GetCert().GetKeyPem(), + secretInfo.secret.Data = map[string][]byte{ + secretInfo.caCertFileName: caPem, + secretInfo.serviceCertFileName: cert.GetCert().GetCertPem(), + secretInfo.serviceKeyFileName: cert.GetCert().GetKeyPem(), } - if _, err := r.secretsClient.Update(ctx, secret, metav1.UpdateOptions{}); err != nil { + if _, err := r.secretsClient.Update(ctx, secretInfo.secret, metav1.UpdateOptions{}); err != nil { return errors.Wrapf(err, "for service type %q", cert.GetServiceType()) } diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index f8e9519e99975..b6f798e8398d2 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -283,6 +283,15 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { - return newServiceCertificatesRepo(mtls.CACertFileName, mtls.ServiceCertFileName, mtls.ServiceKeyFileName, - secrets, secretsClient) + secretsInfo := make(map[storage.ServiceType]serviceCertificateSecret) + for serviceType, secret := range secrets { + secretsInfo[serviceType] = serviceCertificateSecret{ + secret: secret, + caCertFileName: mtls.CACertFileName, + serviceCertFileName: mtls.ServiceCertFileName, + serviceKeyFileName: mtls.ServiceKeyFileName, + } + } + + return newServiceCertificatesRepo(secretsInfo, secretsClient) } From ec2c8f75b2286a64445c335aac54dd6abdc3c8d2 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 7 Feb 2022 17:22:40 +0100 Subject: [PATCH 24/49] check secret ownership and create secrets on the repository --- .../service_certificates_repository.go | 195 ++++++++++++++---- .../service_certificates_repository_test.go | 41 +--- 2 files changed, 166 insertions(+), 70 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index b6543c4da2de9..704b23444a600 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -3,17 +3,26 @@ package localscanner import ( "bytes" "context" + "encoding/json" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" + appsApiv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sTypes "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" ) var ( - _ serviceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) + // ErrSensorDoesNotOwnCertSecrets indicates that this component won't be updating the certificates in + // the secrets because the owner of the secrets is not the deployment for sensor. + ErrSensorDoesNotOwnCertSecrets = errors.New("sensor deployment does not own certificate secrets") + + errForServiceFormat = "for service type %q" + _ serviceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) ) // serviceCertificatesRepo is in charge of persisting and retrieving a set of service certificates, thus implementing @@ -26,35 +35,42 @@ type serviceCertificatesRepo interface { } // serviceCertificatesRepoSecretsImpl is a serviceCertificatesRepo that uses k8s secrets for persistence. -// All fields except Data are respected when persisting the secrets. -// Invariants: -// - secrets and secretsClient are read-only, except the field Data of the entries in secrets. -// - No secret in secrets is nil. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]serviceCertificateSecret + secrets map[storage.ServiceType]ServiceCertSecretSpec secretsClient corev1.SecretInterface } -type serviceCertificateSecret struct { - secret *v1.Secret +// ServiceCertSecretSpec species the name of the secret where certificates for a service are stored, and +// the secret data keys where each certificate file is stored. +type ServiceCertSecretSpec struct { + secretName string caCertFileName string serviceCertFileName string serviceKeyFileName string } -// newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists -// certificates for the specified services in the corresponding k8s secrets. -func newServiceCertificatesRepo(secrets map[storage.ServiceType]serviceCertificateSecret, +// NewServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists +// certificates for the specified services in k8s secrets with the secret name and secret data +// path specified in ServiceCertSecretSpec. +// Returns ErrSensorDoesNotOwnCertSecrets in case some secret doesn't have sensorDeployment as owner. +// If some secret does not exist then it creates it in same namespace as sensorDeployment, and with +// sensorDeployment as owner, populating the data of the new secrets with the corresponding certificates +// in initialCerts. +func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec ServiceCertSecretSpec, + sensorDeployment *appsApiv1.Deployment, initialCerts *storage.TypedServiceCertificateSet, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { - for serviceType, secret := range secrets { - if secret.secret == nil { - return nil, errors.Errorf("nil secrets for service type %q", serviceType) - } - } - return &serviceCertificatesRepoSecretsImpl{ - secrets: secrets, + repo := &serviceCertificatesRepoSecretsImpl{ + secrets: map[storage.ServiceType]ServiceCertSecretSpec{ + storage.ServiceType_SCANNER_SERVICE: scannerSpec, + storage.ServiceType_SCANNER_DB_SERVICE: scannerDBSpec, + }, secretsClient: secretsClient, - }, nil + } + if err := repo.setupSecrets(ctx, sensorDeployment, initialCerts); err != nil { + return nil, errors.Wrap(err, "setting up secrets") + } + + return repo, nil } // getServiceCertificates behaves as follows in case of missing data in the secrets: @@ -67,8 +83,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) var firstServiceTypeWithCA storage.ServiceType var getErr error - for serviceType, secretInfo := range r.secrets { - if err := r.getServiceCertificate(ctx, serviceType, secretInfo, certificates, &firstServiceTypeWithCA); err != nil { + for serviceType, secretSpec := range r.secrets { + if err := r.getServiceCertificate(ctx, serviceType, secretSpec, certificates, &firstServiceTypeWithCA); err != nil { getErr = multierror.Append(getErr, err) } // on context cancellation abort getting other secrets. @@ -84,23 +100,22 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. } func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, - serviceType storage.ServiceType, secretInfo serviceCertificateSecret, + serviceType storage.ServiceType, secretSpec ServiceCertSecretSpec, certificates *storage.TypedServiceCertificateSet, firstServiceTypeWithCA *storage.ServiceType) error { - // Invariant: no secret in r.secrets is nil. - retrievedSecret, err := r.secretsClient.Get(ctx, secretInfo.secret.Name, metav1.GetOptions{}) + retrievedSecret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if err != nil { - return errors.Wrapf(err, "for service type %q", serviceType) + return errors.Wrapf(err, errForServiceFormat, serviceType) } secretData := retrievedSecret.Data if secretData == nil { return errors.Wrapf(err, "missing for secret data for service type %q", serviceType) } if certificates.GetCaPem() == nil { - certificates.CaPem = secretData[secretInfo.caCertFileName] + certificates.CaPem = secretData[secretSpec.caCertFileName] *firstServiceTypeWithCA = serviceType } else { - if !bytes.Equal(certificates.GetCaPem(), secretData[secretInfo.caCertFileName]) { + if !bytes.Equal(certificates.GetCaPem(), secretData[secretSpec.caCertFileName]) { return errors.Errorf("found different CA PEM in secret Data for service types %q and %q", firstServiceTypeWithCA, serviceType) } @@ -108,8 +123,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ - CertPem: secretData[secretInfo.serviceCertFileName], - KeyPem: secretData[secretInfo.serviceKeyFileName], + CertPem: secretData[secretSpec.serviceCertFileName], + KeyPem: secretData[secretSpec.serviceKeyFileName], }, }) @@ -141,20 +156,124 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, cert *storage.TypedServiceCertificate) error { - secretInfo, ok := r.secrets[cert.GetServiceType()] + secretSpec, ok := r.secrets[cert.GetServiceType()] if !ok { - // we don't know where to persist this. + // we don't know how to persist this. return errors.Errorf("unkown service type %q", cert.GetServiceType()) } - // Invariant: no secret in r.secrets is nil. - secretInfo.secret.Data = map[string][]byte{ - secretInfo.caCertFileName: caPem, - secretInfo.serviceCertFileName: cert.GetCert().GetCertPem(), - secretInfo.serviceKeyFileName: cert.GetCert().GetKeyPem(), + secretData, err := r.secretDataForCertificate(caPem, cert) + if err != nil { + return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) + } + patch := []patchByteMap{{ + Op: "replace", + Path: "/data", + Value: secretData, + }} + patchBytes, err := json.Marshal(patch) + if err != nil { + return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) } - if _, err := r.secretsClient.Update(ctx, secretInfo.secret, metav1.UpdateOptions{}); err != nil { - return errors.Wrapf(err, "for service type %q", cert.GetServiceType()) + if _, err = r.secretsClient.Patch(ctx, secretSpec.secretName, k8sTypes.JSONPatchType, patchBytes, + metav1.PatchOptions{}); err != nil { + return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) } return nil } + +type patchByteMap struct { + Op string `json:"op"` + Path string `json:"path"` + Value map[string][]byte `json:"value"` +} + +// setupSecrets setups the k8s secrets where we store the certificates. +// - In case the secret doesn't have sensorDeployment as owner, this returns ErrSensorDoesNotOwnCertSecrets. +// - In case the secret doesn't exist this creates it setting sensorDeployment as owner, with cert stored +// in the secret data. +func (r *serviceCertificatesRepoSecretsImpl) setupSecrets(ctx context.Context, sensorDeployment *appsApiv1.Deployment, + initialCerts *storage.TypedServiceCertificateSet) error { + for serviceType, secretSpec := range r.secrets { + serviceCert, err := r.certificateForService(initialCerts, serviceType) + if err != nil { + return errors.Wrapf(err, errForServiceFormat, serviceType) + } + _, err = r.setupSecret(ctx, initialCerts.GetCaPem(), serviceCert, sensorDeployment, secretSpec.secretName) + if err != nil { + return errors.Wrapf(err, errForServiceFormat, serviceType) + } + } + + return nil +} + +func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, + caPem []byte, cert *storage.TypedServiceCertificate, + sensorDeployment *appsApiv1.Deployment, secretName string) (*v1.Secret, error) { + secret, err := r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) + + if k8sErrors.IsNotFound(err) { + secretData, err := r.secretDataForCertificate(caPem, cert) + if err != nil { + return nil, err + } + newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: sensorDeployment.GetNamespace(), + OwnerReferences: ownerReferenceFor(sensorDeployment), + }, + Data: secretData, + }, metav1.CreateOptions{}) + if createErr != nil { + return nil, createErr + } + return newSecret, nil + } + + if err != nil { + return nil, err + } + + ownerReferences := secret.GetOwnerReferences() + if len(ownerReferences) != 1 { + return nil, ErrSensorDoesNotOwnCertSecrets + } + + ownerRef := ownerReferences[0] + if ownerRef.UID != sensorDeployment.GetUID() { + return nil, ErrSensorDoesNotOwnCertSecrets + } + + return secret, nil +} + +func ownerReferenceFor(sensorDeployment *appsApiv1.Deployment) []metav1.OwnerReference { + return []metav1.OwnerReference{ + *metav1.NewControllerRef(sensorDeployment, sensorDeployment.GroupVersionKind()), + } +} + +func (r *serviceCertificatesRepoSecretsImpl) certificateForService(certs *storage.TypedServiceCertificateSet, + serviceType storage.ServiceType) (*storage.TypedServiceCertificate, error) { + for _, cert := range certs.GetServiceCerts() { + if cert.GetServiceType() == serviceType { + return cert, nil + } + } + return nil, errors.Errorf("no certificate found for service type %q", serviceType) +} + +func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(caPem []byte, + cert *storage.TypedServiceCertificate) (map[string][]byte, error) { + secretSpec, ok := r.secrets[cert.GetServiceType()] + if !ok { + return nil, errors.Errorf("unkown service type %q", cert.GetServiceType()) + } + return map[string][]byte{ + secretSpec.caCertFileName: caPem, + secretSpec.serviceCertFileName: cert.GetCert().GetCertPem(), + secretSpec.serviceKeyFileName: cert.GetCert().GetKeyPem(), + }, nil +} diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index b6f798e8398d2..0773f99af9687 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -93,9 +93,8 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) - repo, err := newTestRepo(secrets, secretsClient) - s.Require().NoError(err) - _, err = repo.getServiceCertificates(context.Background()) + repo := newTestRepo(secrets, secretsClient) + _, err := repo.getServiceCertificates(context.Background()) if tc.expectError { s.Error(err) } else { @@ -111,7 +110,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { fixture *certSecretsRepoFixture }{ "successful put": {nil, s.newFixture("")}, - "failed put": {errForced, s.newFixture("update")}, + "failed put": {errForced, s.newFixture("patch")}, "cancelled put": {context.Canceled, s.newFixture("")}, } for tcName, tc := range testCases { @@ -129,27 +128,6 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { } } -func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithNilSecretFailure() { - fixture := s.newFixture("") - var secret *v1.Secret - secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} - _, err := newTestRepo(secrets, fixture.secretsClient) - s.Error(err) -} - -func (s *serviceCertificatesRepoSecretsImplSuite) TestNewRepoWithJustOneNoNilCASecretDataSuccess() { - fixture := s.newFixture("") - secret1 := &v1.Secret{ - Data: map[string][]byte{ - mtls.CACertFileName: make([]byte, 0), - }, - } - secret2 := &v1.Secret{} - secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} - _, err := newTestRepo(secrets, fixture.secretsClient) - s.NoError(err) -} - func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { fixture := s.newFixtureAdvancedOpts("", true) expectedCertificates := &storage.TypedServiceCertificateSet{} @@ -273,8 +251,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) - repo, err := newTestRepo(secrets, secretsClient) - s.Require().NoError(err) + repo := newTestRepo(secrets, secretsClient) return &certSecretsRepoFixture{ repo: repo, secretsClient: secretsClient, @@ -282,16 +259,16 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } } -func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { - secretsInfo := make(map[storage.ServiceType]serviceCertificateSecret) +func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) serviceCertificatesRepo { + secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) for serviceType, secret := range secrets { - secretsInfo[serviceType] = serviceCertificateSecret{ - secret: secret, + secretsSpec[serviceType] = ServiceCertSecretSpec{ + secretName: secret.Name, caCertFileName: mtls.CACertFileName, serviceCertFileName: mtls.ServiceCertFileName, serviceKeyFileName: mtls.ServiceKeyFileName, } } - return newServiceCertificatesRepo(secretsInfo, secretsClient) + return &serviceCertificatesRepoSecretsImpl{secrets: secretsSpec, secretsClient: secretsClient} } From 4f1bad1ddf4db860ee519be21411e1b55687a039 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 7 Feb 2022 17:32:15 +0100 Subject: [PATCH 25/49] comments improvements --- .../service_certificates_repository.go | 77 +++++++++---------- .../service_certificates_repository_test.go | 18 ++--- 2 files changed, 45 insertions(+), 50 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 704b23444a600..3ab46137d13ae 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -17,24 +17,24 @@ import ( ) var ( - // ErrSensorDoesNotOwnCertSecrets indicates that this component won't be updating the certificates in + // ErrSensorDoesNotOwnCertSecrets indicates that this repository should not be updating the certificates in // the secrets because the owner of the secrets is not the deployment for sensor. ErrSensorDoesNotOwnCertSecrets = errors.New("sensor deployment does not own certificate secrets") errForServiceFormat = "for service type %q" - _ serviceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) + _ ServiceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) ) -// serviceCertificatesRepo is in charge of persisting and retrieving a set of service certificates, thus implementing +// ServiceCertificatesRepo is in charge of persisting and retrieving a set of service certificates, thus implementing // the [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for *storage.TypedServiceCertificateSet. -type serviceCertificatesRepo interface { - // getServiceCertificates retrieves the certificates from permanent storage. - getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) - // putServiceCertificates persists the certificates on permanent storage. - putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error +type ServiceCertificatesRepo interface { + // GetServiceCertificates retrieves the certificates from permanent storage. + GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) + // PutServiceCertificates persists the certificates on permanent storage. + PutServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error } -// serviceCertificatesRepoSecretsImpl is a serviceCertificatesRepo that uses k8s secrets for persistence. +// serviceCertificatesRepoSecretsImpl is a ServiceCertificatesRepo that uses k8s secrets for persistence. type serviceCertificatesRepoSecretsImpl struct { secrets map[storage.ServiceType]ServiceCertSecretSpec secretsClient corev1.SecretInterface @@ -49,16 +49,14 @@ type ServiceCertSecretSpec struct { serviceKeyFileName string } -// NewServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists -// certificates for the specified services in k8s secrets with the secret name and secret data -// path specified in ServiceCertSecretSpec. +// NewServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists certificates for +// scanner and scanner DB in k8s secrets with the secret name and secret data path specified in ServiceCertSecretSpec. // Returns ErrSensorDoesNotOwnCertSecrets in case some secret doesn't have sensorDeployment as owner. -// If some secret does not exist then it creates it in same namespace as sensorDeployment, and with -// sensorDeployment as owner, populating the data of the new secrets with the corresponding certificates -// in initialCerts. +// In case some secret does not exist then it creates it in same namespace as sensorDeployment, and with +// sensorDeployment as owner, populating the secret data with the corresponding certificates in initialCerts. func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec ServiceCertSecretSpec, sensorDeployment *appsApiv1.Deployment, initialCerts *storage.TypedServiceCertificateSet, - secretsClient corev1.SecretInterface) (serviceCertificatesRepo, error) { + secretsClient corev1.SecretInterface) (ServiceCertificatesRepo, error) { repo := &serviceCertificatesRepoSecretsImpl{ secrets: map[storage.ServiceType]ServiceCertSecretSpec{ storage.ServiceType_SCANNER_SERVICE: scannerSpec, @@ -73,12 +71,12 @@ func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec return repo, nil } -// getServiceCertificates behaves as follows in case of missing data in the secrets: +// GetServiceCertificates behaves as follows in case of missing data in the secrets: // - if a secret has no data then the certificates won't contain a TypedServiceCertificate for the corresponding // service type. // - if the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate // for that secret will contain a zero value. -func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { +func (r *serviceCertificatesRepoSecretsImpl) GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) var firstServiceTypeWithCA storage.ServiceType @@ -103,11 +101,11 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C serviceType storage.ServiceType, secretSpec ServiceCertSecretSpec, certificates *storage.TypedServiceCertificateSet, firstServiceTypeWithCA *storage.ServiceType) error { - retrievedSecret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) + secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if err != nil { return errors.Wrapf(err, errForServiceFormat, serviceType) } - secretData := retrievedSecret.Data + secretData := secret.Data if secretData == nil { return errors.Wrapf(err, "missing for secret data for service type %q", serviceType) } @@ -131,12 +129,13 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C return nil } -// putServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted +// PutServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted // while others are not. // Edge cases: // - Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. // - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. -func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { +func (r *serviceCertificatesRepoSecretsImpl) PutServiceCertificates(ctx context.Context, + certificates *storage.TypedServiceCertificateSet) error { var putErr error caPem := certificates.GetCaPem() for _, cert := range certificates.GetServiceCerts() { @@ -165,32 +164,32 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.C if err != nil { return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) } - patch := []patchByteMap{{ + patch := []patchSecretDataByteMap{{ Op: "replace", Path: "/data", Value: secretData, }} - patchBytes, err := json.Marshal(patch) - if err != nil { - return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) + patchBytes, marshallingErr := json.Marshal(patch) + if marshallingErr != nil { + return errors.Wrapf(marshallingErr, errForServiceFormat, cert.GetServiceType()) } - if _, err = r.secretsClient.Patch(ctx, secretSpec.secretName, k8sTypes.JSONPatchType, patchBytes, - metav1.PatchOptions{}); err != nil { - return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) + if _, patchErr := r.secretsClient.Patch(ctx, secretSpec.secretName, k8sTypes.JSONPatchType, patchBytes, + metav1.PatchOptions{}); patchErr != nil { + return errors.Wrapf(patchErr, errForServiceFormat, cert.GetServiceType()) } return nil } -type patchByteMap struct { +type patchSecretDataByteMap struct { Op string `json:"op"` Path string `json:"path"` Value map[string][]byte `json:"value"` } // setupSecrets setups the k8s secrets where we store the certificates. -// - In case the secret doesn't have sensorDeployment as owner, this returns ErrSensorDoesNotOwnCertSecrets. -// - In case the secret doesn't exist this creates it setting sensorDeployment as owner, with cert stored +// - In case a secret doesn't have sensorDeployment as owner, this returns ErrSensorDoesNotOwnCertSecrets. +// - In case a secret doesn't exist this creates it setting sensorDeployment as owner, with cert stored // in the secret data. func (r *serviceCertificatesRepoSecretsImpl) setupSecrets(ctx context.Context, sensorDeployment *appsApiv1.Deployment, initialCerts *storage.TypedServiceCertificateSet) error { @@ -220,9 +219,11 @@ func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, } newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: sensorDeployment.GetNamespace(), - OwnerReferences: ownerReferenceFor(sensorDeployment), + Name: secretName, + Namespace: sensorDeployment.GetNamespace(), + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(sensorDeployment, sensorDeployment.GroupVersionKind()), + }, }, Data: secretData, }, metav1.CreateOptions{}) @@ -249,12 +250,6 @@ func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, return secret, nil } -func ownerReferenceFor(sensorDeployment *appsApiv1.Deployment) []metav1.OwnerReference { - return []metav1.OwnerReference{ - *metav1.NewControllerRef(sensorDeployment, sensorDeployment.GroupVersionKind()), - } -} - func (r *serviceCertificatesRepoSecretsImpl) certificateForService(certs *storage.TypedServiceCertificateSet, serviceType storage.ServiceType) (*storage.TypedServiceCertificate, error) { for _, cert := range certs.GetServiceCerts() { diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 0773f99af9687..2f833d06524a2 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -53,7 +53,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { cancelGetCtx() } - certificates, err := tc.fixture.repo.getServiceCertificates(getCtx) + certificates, err := tc.fixture.repo.GetServiceCertificates(getCtx) if tc.expectedErr == nil { s.Equal(tc.fixture.certificates, certificates) } @@ -94,7 +94,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) repo := newTestRepo(secrets, secretsClient) - _, err := repo.getServiceCertificates(context.Background()) + _, err := repo.GetServiceCertificates(context.Background()) if tc.expectError { s.Error(err) } else { @@ -121,7 +121,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { cancelPutCtx() } - err := tc.fixture.repo.putServiceCertificates(putCtx, tc.fixture.certificates) + err := tc.fixture.repo.PutServiceCertificates(putCtx, tc.fixture.certificates) s.checkExpectedError(tc.expectedErr, err) }) @@ -133,7 +133,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { expectedCertificates := &storage.TypedServiceCertificateSet{} expectedCertificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - certificates, err := fixture.repo.getServiceCertificates(context.Background()) + certificates, err := fixture.repo.GetServiceCertificates(context.Background()) s.NoError(err) s.Equal(expectedCertificates, certificates) @@ -165,7 +165,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu for tcName, tc := range testCases { s.Run(tcName, func() { fixture := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) - certificates, err := fixture.repo.getServiceCertificates(context.Background()) + certificates, err := fixture.repo.GetServiceCertificates(context.Background()) tc.setExpectedCertsFunc(fixture.certificates) s.NoError(err) @@ -177,14 +177,14 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { fixture := s.newFixture("") s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType - err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.PutServiceCertificates(context.Background(), fixture.certificates) s.Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { fixture := s.newFixture("") fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.PutServiceCertificates(context.Background(), fixture.certificates) s.NoError(err) } @@ -205,7 +205,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) getFirstServiceCertificate( } type certSecretsRepoFixture struct { - repo serviceCertificatesRepo + repo ServiceCertificatesRepo secretsClient corev1.SecretInterface certificates *storage.TypedServiceCertificateSet } @@ -259,7 +259,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } } -func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) serviceCertificatesRepo { +func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) ServiceCertificatesRepo { secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) for serviceType, secret := range secrets { secretsSpec[serviceType] = ServiceCertSecretSpec{ From a3111f9fca6312fbf9712680c7dd01ba85cf3b02 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 8 Feb 2022 11:10:38 +0100 Subject: [PATCH 26/49] request certificates on demand during secret setup --- .../service_certificates_repository.go | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 3ab46137d13ae..a40cd45848cec 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -55,7 +55,7 @@ type ServiceCertSecretSpec struct { // In case some secret does not exist then it creates it in same namespace as sensorDeployment, and with // sensorDeployment as owner, populating the secret data with the corresponding certificates in initialCerts. func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec ServiceCertSecretSpec, - sensorDeployment *appsApiv1.Deployment, initialCerts *storage.TypedServiceCertificateSet, + sensorDeployment *appsApiv1.Deployment, initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error), secretsClient corev1.SecretInterface) (ServiceCertificatesRepo, error) { repo := &serviceCertificatesRepoSecretsImpl{ secrets: map[storage.ServiceType]ServiceCertSecretSpec{ @@ -64,7 +64,7 @@ func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec }, secretsClient: secretsClient, } - if err := repo.setupSecrets(ctx, sensorDeployment, initialCerts); err != nil { + if err := repo.setupSecrets(ctx, sensorDeployment, initialCertsSupplier); err != nil { return nil, errors.Wrap(err, "setting up secrets") } @@ -192,13 +192,9 @@ type patchSecretDataByteMap struct { // - In case a secret doesn't exist this creates it setting sensorDeployment as owner, with cert stored // in the secret data. func (r *serviceCertificatesRepoSecretsImpl) setupSecrets(ctx context.Context, sensorDeployment *appsApiv1.Deployment, - initialCerts *storage.TypedServiceCertificateSet) error { + initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error)) error { for serviceType, secretSpec := range r.secrets { - serviceCert, err := r.certificateForService(initialCerts, serviceType) - if err != nil { - return errors.Wrapf(err, errForServiceFormat, serviceType) - } - _, err = r.setupSecret(ctx, initialCerts.GetCaPem(), serviceCert, sensorDeployment, secretSpec.secretName) + _, err := r.setupSecret(ctx, serviceType, initialCertsSupplier, sensorDeployment, secretSpec.secretName) if err != nil { return errors.Wrapf(err, errForServiceFormat, serviceType) } @@ -208,14 +204,23 @@ func (r *serviceCertificatesRepoSecretsImpl) setupSecrets(ctx context.Context, s } func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, - caPem []byte, cert *storage.TypedServiceCertificate, + serviceType storage.ServiceType, + initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error), sensorDeployment *appsApiv1.Deployment, secretName string) (*v1.Secret, error) { secret, err := r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { - secretData, err := r.secretDataForCertificate(caPem, cert) - if err != nil { - return nil, err + initialCerts, getInitialCertsErr := initialCertsSupplier(ctx) + if getInitialCertsErr != nil { + return nil, getInitialCertsErr + } + cert, getCertErr := r.certificateForService(initialCerts, serviceType) + if getCertErr != nil { + return nil, getCertErr + } + secretData, dataForCertErr := r.secretDataForCertificate(initialCerts.GetCaPem(), cert) + if dataForCertErr != nil { + return nil, dataForCertErr } newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ From 358f02d1b97b8f9cbb8db0307578cc31611055ef Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Tue, 8 Feb 2022 14:17:58 +0100 Subject: [PATCH 27/49] move repo interface definition to client code it'll moved to the sensor component that uses the repo --- .../service_certificates_repository.go | 14 ++------------ .../service_certificates_repository_test.go | 4 ++-- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index a40cd45848cec..436ad45332958 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -21,19 +21,9 @@ var ( // the secrets because the owner of the secrets is not the deployment for sensor. ErrSensorDoesNotOwnCertSecrets = errors.New("sensor deployment does not own certificate secrets") - errForServiceFormat = "for service type %q" - _ ServiceCertificatesRepo = (*serviceCertificatesRepoSecretsImpl)(nil) + errForServiceFormat = "for service type %q" ) -// ServiceCertificatesRepo is in charge of persisting and retrieving a set of service certificates, thus implementing -// the [repository pattern](https://martinfowler.com/eaaCatalog/repository.html) for *storage.TypedServiceCertificateSet. -type ServiceCertificatesRepo interface { - // GetServiceCertificates retrieves the certificates from permanent storage. - GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) - // PutServiceCertificates persists the certificates on permanent storage. - PutServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error -} - // serviceCertificatesRepoSecretsImpl is a ServiceCertificatesRepo that uses k8s secrets for persistence. type serviceCertificatesRepoSecretsImpl struct { secrets map[storage.ServiceType]ServiceCertSecretSpec @@ -56,7 +46,7 @@ type ServiceCertSecretSpec struct { // sensorDeployment as owner, populating the secret data with the corresponding certificates in initialCerts. func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec ServiceCertSecretSpec, sensorDeployment *appsApiv1.Deployment, initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error), - secretsClient corev1.SecretInterface) (ServiceCertificatesRepo, error) { + secretsClient corev1.SecretInterface) (*serviceCertificatesRepoSecretsImpl, error) { repo := &serviceCertificatesRepoSecretsImpl{ secrets: map[storage.ServiceType]ServiceCertSecretSpec{ storage.ServiceType_SCANNER_SERVICE: scannerSpec, diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 2f833d06524a2..cc6239e10ff2b 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -205,7 +205,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) getFirstServiceCertificate( } type certSecretsRepoFixture struct { - repo ServiceCertificatesRepo + repo *serviceCertificatesRepoSecretsImpl secretsClient corev1.SecretInterface certificates *storage.TypedServiceCertificateSet } @@ -259,7 +259,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } } -func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) ServiceCertificatesRepo { +func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) for serviceType, secret := range secrets { secretsSpec[serviceType] = ServiceCertSecretSpec{ From 369837e1045a4b529a7cd90443c8e32e36a920c4 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 9 Feb 2022 10:29:17 +0100 Subject: [PATCH 28/49] fix secret owner reference --- .../localscanner/service_certificates_repository.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 436ad45332958..fa35d2395acbd 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -212,12 +212,22 @@ func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, if dataForCertErr != nil { return nil, dataForCertErr } + sensorDeploymentGVK := sensorDeployment.GroupVersionKind() + blockOwnerDeletion := false + isController := false newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, Namespace: sensorDeployment.GetNamespace(), OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(sensorDeployment, sensorDeployment.GroupVersionKind()), + { + APIVersion: sensorDeploymentGVK.GroupVersion().String(), + Kind: sensorDeploymentGVK.Kind, + Name: sensorDeployment.GetName(), + UID: sensorDeployment.GetUID(), + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, + }, }, }, Data: secretData, From 744cdabd054b18ee1623253f82e89f8db46d412f Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Wed, 9 Feb 2022 12:58:38 +0100 Subject: [PATCH 29/49] make GetServiceCertificates functional instead of mutating a param --- .../service_certificates_repository.go | 66 +++++++++---------- .../service_certificates_repository_test.go | 9 +-- 2 files changed, 34 insertions(+), 41 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index fa35d2395acbd..2e1f14250bdf1 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" - "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" appsApiv1 "k8s.io/api/apps/v1" @@ -21,6 +20,13 @@ var ( // the secrets because the owner of the secrets is not the deployment for sensor. ErrSensorDoesNotOwnCertSecrets = errors.New("sensor deployment does not own certificate secrets") + // ErrDifferentCAForDifferentServiceTypes indicates that different service types have different values + // for CA stored in their secrets. + ErrDifferentCAForDifferentServiceTypes = errors.New("found different CA PEM in secret Data for different service types") + + // ErrMissingSecretData indicates some secret has no data. + ErrMissingSecretData = errors.New("missing secret data") + errForServiceFormat = "for service type %q" ) @@ -62,61 +68,52 @@ func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec } // GetServiceCertificates behaves as follows in case of missing data in the secrets: -// - if a secret has no data then the certificates won't contain a TypedServiceCertificate for the corresponding -// service type. -// - if the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate +// - Fails with ErrMissingSecretData in case any secret has no data. +// - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. +// - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. func (r *serviceCertificatesRepoSecretsImpl) GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - var firstServiceTypeWithCA storage.ServiceType - var getErr error for serviceType, secretSpec := range r.secrets { - if err := r.getServiceCertificate(ctx, serviceType, secretSpec, certificates, &firstServiceTypeWithCA); err != nil { - getErr = multierror.Append(getErr, err) + certificate, ca, err := r.getServiceCertificate(ctx, serviceType, secretSpec) + if err != nil { + return nil, err + } + if certificates.GetCaPem() == nil { + certificates.CaPem = ca + } else { + if !bytes.Equal(certificates.GetCaPem(), ca) { + return nil, ErrDifferentCAForDifferentServiceTypes + } } + certificates.ServiceCerts = append(certificates.ServiceCerts, certificate) // on context cancellation abort getting other secrets. if ctx.Err() != nil { return nil, ctx.Err() } } - if getErr != nil { - return nil, getErr - } return certificates, nil } -func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, - serviceType storage.ServiceType, secretSpec ServiceCertSecretSpec, - certificates *storage.TypedServiceCertificateSet, - firstServiceTypeWithCA *storage.ServiceType) error { - secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) - if err != nil { - return errors.Wrapf(err, errForServiceFormat, serviceType) +func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, serviceType storage.ServiceType, + secretSpec ServiceCertSecretSpec) (cert *storage.TypedServiceCertificate, ca []byte, err error) { + secret, getErr := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) + if getErr != nil { + return nil, nil, getErr } secretData := secret.Data if secretData == nil { - return errors.Wrapf(err, "missing for secret data for service type %q", serviceType) - } - if certificates.GetCaPem() == nil { - certificates.CaPem = secretData[secretSpec.caCertFileName] - *firstServiceTypeWithCA = serviceType - } else { - if !bytes.Equal(certificates.GetCaPem(), secretData[secretSpec.caCertFileName]) { - return errors.Errorf("found different CA PEM in secret Data for service types %q and %q", - firstServiceTypeWithCA, serviceType) - } + return nil, nil, ErrMissingSecretData } - certificates.ServiceCerts = append(certificates.ServiceCerts, &storage.TypedServiceCertificate{ + return &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ CertPem: secretData[secretSpec.serviceCertFileName], KeyPem: secretData[secretSpec.serviceKeyFileName], }, - }) - - return nil + }, secretData[secretSpec.caCertFileName], nil } // PutServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted @@ -126,11 +123,10 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C // - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. func (r *serviceCertificatesRepoSecretsImpl) PutServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { - var putErr error caPem := certificates.GetCaPem() for _, cert := range certificates.GetServiceCerts() { if err := r.putServiceCertificate(ctx, caPem, cert); err != nil { - putErr = multierror.Append(putErr, err) + return err } // on context cancellation abort putting other secrets. @@ -139,7 +135,7 @@ func (r *serviceCertificatesRepoSecretsImpl) PutServiceCertificates(ctx context. } } - return putErr + return nil } func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index cc6239e10ff2b..663712c91745c 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -128,15 +128,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { } } -func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataSuccess() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { fixture := s.newFixtureAdvancedOpts("", true) - expectedCertificates := &storage.TypedServiceCertificateSet{} - expectedCertificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - certificates, err := fixture.repo.GetServiceCertificates(context.Background()) + _, err := fixture.repo.GetServiceCertificates(context.Background()) - s.NoError(err) - s.Equal(expectedCertificates, certificates) + s.ErrorIs(err, ErrMissingSecretData) } func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSuccess() { From 9b0b29c56092a056fb12904744b11ef44fb80933 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 10:42:54 +0100 Subject: [PATCH 30/49] check secret ownership on get certificates --- .../service_certificates_repository.go | 25 ++++++++- .../service_certificates_repository_test.go | 55 +++++++++++++++---- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 2e1f14250bdf1..1416e2cb1a251 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -32,8 +32,9 @@ var ( // serviceCertificatesRepoSecretsImpl is a ServiceCertificatesRepo that uses k8s secrets for persistence. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]ServiceCertSecretSpec - secretsClient corev1.SecretInterface + secrets map[storage.ServiceType]ServiceCertSecretSpec + sensorDeployment *appsApiv1.Deployment + secretsClient corev1.SecretInterface } // ServiceCertSecretSpec species the name of the secret where certificates for a service are stored, and @@ -58,7 +59,8 @@ func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec storage.ServiceType_SCANNER_SERVICE: scannerSpec, storage.ServiceType_SCANNER_DB_SERVICE: scannerDBSpec, }, - secretsClient: secretsClient, + sensorDeployment: sensorDeployment, + secretsClient: secretsClient, } if err := repo.setupSecrets(ctx, sensorDeployment, initialCertsSupplier); err != nil { return nil, errors.Wrap(err, "setting up secrets") @@ -72,6 +74,7 @@ func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec // - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. // - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. +// - Fails ErrSensorDoesNotOwnCertSecrets in case sensor deployment is not the sole owner of all secrets. func (r *serviceCertificatesRepoSecretsImpl) GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) @@ -103,10 +106,26 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C if getErr != nil { return nil, nil, getErr } + + ownerReferences := secret.GetOwnerReferences() + if len(ownerReferences) != 1 { + return nil, nil, ErrSensorDoesNotOwnCertSecrets + } + + ownerRef := ownerReferences[0] + sensorDeploymentGVK := r.sensorDeployment.GroupVersionKind() + if !(ownerRef.APIVersion == sensorDeploymentGVK.GroupVersion().String() && + ownerRef.Kind == sensorDeploymentGVK.Kind && + ownerRef.Name == r.sensorDeployment.GetName() && + ownerRef.UID == r.sensorDeployment.GetUID()) { + return nil, nil, ErrSensorDoesNotOwnCertSecrets + } + secretData := secret.Data if secretData == nil { return nil, nil, ErrMissingSecretData } + return &storage.TypedServiceCertificate{ ServiceType: serviceType, Cert: &storage.ServiceCertificate{ diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 663712c91745c..233fa490f1bdc 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -9,6 +9,7 @@ import ( "github.com/stackrox/rox/generated/storage" "github.com/stackrox/rox/pkg/mtls" "github.com/stretchr/testify/suite" + appsApiv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -26,6 +27,12 @@ var ( errForced = errors.New("forced error") serviceType = storage.ServiceType_SCANNER_SERVICE anotherServiceType = storage.ServiceType_SENSOR_SERVICE + sensorDeployment = &appsApiv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sensor-deployment", + Namespace: namespace, + }, + } ) func TestServiceCertificatesRepoSecretsImpl(t *testing.T) { @@ -74,8 +81,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { s.Run(tcName, func() { secret1 := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "secret1", - Namespace: namespace, + Name: "secret1", + Namespace: namespace, + OwnerReferences: sensorOwnerReference(), }, Data: map[string][]byte{ mtls.CACertFileName: make([]byte, 0), @@ -83,8 +91,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { } secret2 := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "secret2", - Namespace: namespace, + Name: "secret2", + Namespace: namespace, + OwnerReferences: sensorOwnerReference(), }, Data: map[string][]byte{ mtls.CACertFileName: make([]byte, tc.secondCASize), @@ -93,7 +102,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) - repo := newTestRepo(secrets, secretsClient) + repo := newTestRepo(secrets, sensorDeployment, secretsClient) _, err := repo.GetServiceCertificates(context.Background()) if tc.expectError { s.Error(err) @@ -228,8 +237,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-secret", serviceType), - Namespace: namespace, + Name: fmt.Sprintf("%s-secret", serviceType), + Namespace: namespace, + OwnerReferences: sensorOwnerReference(), }, } if !emptySecretData { @@ -243,12 +253,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE delete(secret.Data, secretDataKey) } secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} - clientSet := fake.NewSimpleClientset(secret) + clientSet := fake.NewSimpleClientset(sensorDeployment, secret) secretsClient := clientSet.CoreV1().Secrets(namespace) clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) - repo := newTestRepo(secrets, secretsClient) + repo := newTestRepo(secrets, sensorDeployment, secretsClient) return &certSecretsRepoFixture{ repo: repo, secretsClient: secretsClient, @@ -256,7 +266,26 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } } -func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { +func sensorOwnerReference() []metav1.OwnerReference { + sensorDeploymentGVK := sensorDeployment.GroupVersionKind() + blockOwnerDeletion := false + isController := false + return []metav1.OwnerReference{ + { + APIVersion: sensorDeploymentGVK.GroupVersion().String(), + Kind: sensorDeploymentGVK.Kind, + Name: sensorDeployment.GetName(), + UID: sensorDeployment.GetUID(), + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, + }, + } +} + +func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, + sensorDeployment *appsApiv1.Deployment, + secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { + secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) for serviceType, secret := range secrets { secretsSpec[serviceType] = ServiceCertSecretSpec{ @@ -267,5 +296,9 @@ func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev } } - return &serviceCertificatesRepoSecretsImpl{secrets: secretsSpec, secretsClient: secretsClient} + return &serviceCertificatesRepoSecretsImpl{ + secrets: secretsSpec, + sensorDeployment: sensorDeployment, + secretsClient: secretsClient, + } } From f8f1884518c5751e946aa55931e21742ca2fd03b Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 11:43:13 +0100 Subject: [PATCH 31/49] replace setupSecrets by CreateSecrets --- .../service_certificates_repository.go | 159 ++++++++---------- .../service_certificates_repository_test.go | 12 +- 2 files changed, 72 insertions(+), 99 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 1416e2cb1a251..1b470ff7cbd7f 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -5,9 +5,10 @@ import ( "context" "encoding/json" + "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" - appsApiv1 "k8s.io/api/apps/v1" + "github.com/stackrox/rox/pkg/mtls" v1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,9 +33,10 @@ var ( // serviceCertificatesRepoSecretsImpl is a ServiceCertificatesRepo that uses k8s secrets for persistence. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]ServiceCertSecretSpec - sensorDeployment *appsApiv1.Deployment - secretsClient corev1.SecretInterface + secrets map[storage.ServiceType]ServiceCertSecretSpec + ownerReference metav1.OwnerReference + namespace string + secretsClient corev1.SecretInterface } // ServiceCertSecretSpec species the name of the secret where certificates for a service are stored, and @@ -48,25 +50,28 @@ type ServiceCertSecretSpec struct { // NewServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists certificates for // scanner and scanner DB in k8s secrets with the secret name and secret data path specified in ServiceCertSecretSpec. -// Returns ErrSensorDoesNotOwnCertSecrets in case some secret doesn't have sensorDeployment as owner. -// In case some secret does not exist then it creates it in same namespace as sensorDeployment, and with -// sensorDeployment as owner, populating the secret data with the corresponding certificates in initialCerts. -func NewServiceCertificatesRepo(ctx context.Context, scannerSpec, scannerDBSpec ServiceCertSecretSpec, - sensorDeployment *appsApiv1.Deployment, initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error), - secretsClient corev1.SecretInterface) (*serviceCertificatesRepoSecretsImpl, error) { - repo := &serviceCertificatesRepoSecretsImpl{ +func NewServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace string, + secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { + + return &serviceCertificatesRepoSecretsImpl{ secrets: map[storage.ServiceType]ServiceCertSecretSpec{ - storage.ServiceType_SCANNER_SERVICE: scannerSpec, - storage.ServiceType_SCANNER_DB_SERVICE: scannerDBSpec, + storage.ServiceType_SCANNER_SERVICE: { + secretName: "scanner-slim-tls", + caCertFileName: mtls.CACertFileName, + serviceCertFileName: mtls.ServiceCertFileName, + serviceKeyFileName: mtls.ServiceKeyFileName, + }, + storage.ServiceType_SCANNER_DB_SERVICE: { + secretName: "scanner-db-slim-tls", + caCertFileName: mtls.CACertFileName, + serviceCertFileName: mtls.ServiceCertFileName, + serviceKeyFileName: mtls.ServiceKeyFileName, + }, }, - sensorDeployment: sensorDeployment, - secretsClient: secretsClient, + ownerReference: ownerReference, + namespace: namespace, + secretsClient: secretsClient, } - if err := repo.setupSecrets(ctx, sensorDeployment, initialCertsSupplier); err != nil { - return nil, errors.Wrap(err, "setting up secrets") - } - - return repo, nil } // GetServiceCertificates behaves as follows in case of missing data in the secrets: @@ -113,11 +118,10 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C } ownerRef := ownerReferences[0] - sensorDeploymentGVK := r.sensorDeployment.GroupVersionKind() - if !(ownerRef.APIVersion == sensorDeploymentGVK.GroupVersion().String() && - ownerRef.Kind == sensorDeploymentGVK.Kind && - ownerRef.Name == r.sensorDeployment.GetName() && - ownerRef.UID == r.sensorDeployment.GetUID()) { + if !(ownerRef.APIVersion == r.ownerReference.APIVersion && + ownerRef.Kind == r.ownerReference.Kind && + ownerRef.Name == r.ownerReference.Name && + ownerRef.UID == r.ownerReference.UID) { return nil, nil, ErrSensorDoesNotOwnCertSecrets } @@ -165,14 +169,11 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.C // we don't know how to persist this. return errors.Errorf("unkown service type %q", cert.GetServiceType()) } - secretData, err := r.secretDataForCertificate(caPem, cert) - if err != nil { - return errors.Wrapf(err, errForServiceFormat, cert.GetServiceType()) - } + patch := []patchSecretDataByteMap{{ Op: "replace", Path: "/data", - Value: secretData, + Value: r.secretDataForCertificate(secretSpec, caPem, cert), }} patchBytes, marshallingErr := json.Marshal(patch) if marshallingErr != nil { @@ -192,60 +193,56 @@ type patchSecretDataByteMap struct { Value map[string][]byte `json:"value"` } -// setupSecrets setups the k8s secrets where we store the certificates. -// - In case a secret doesn't have sensorDeployment as owner, this returns ErrSensorDoesNotOwnCertSecrets. -// - In case a secret doesn't exist this creates it setting sensorDeployment as owner, with cert stored -// in the secret data. -func (r *serviceCertificatesRepoSecretsImpl) setupSecrets(ctx context.Context, sensorDeployment *appsApiv1.Deployment, - initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error)) error { - for serviceType, secretSpec := range r.secrets { - _, err := r.setupSecret(ctx, serviceType, initialCertsSupplier, sensorDeployment, secretSpec.secretName) - if err != nil { - return errors.Wrapf(err, errForServiceFormat, serviceType) +// CreateSecrets ensures the k8s secrets where we store the certificates are created. +// In case a secret doesn't exist this creates it setting sensorDeployment as owner, with initialCertificates stored +// in the secret data. +func (r *serviceCertificatesRepoSecretsImpl) CreateSecrets(ctx context.Context, + initialCertificates *storage.TypedServiceCertificateSet) error { + + var createErr error + for _, certificate := range initialCertificates.GetServiceCerts() { + secretSpec, ok := r.secrets[certificate.GetServiceType()] + if !ok { + err := errors.Errorf("unkown service type %q", certificate.GetServiceType()) + createErr = multierror.Append(createErr, err) + } else { + _, createSecretErr := r.createSecret(ctx, initialCertificates.GetCaPem(), certificate, secretSpec) + if createSecretErr != nil { + err := errors.Wrapf(createSecretErr, errForServiceFormat, certificate.GetServiceType()) + createErr = multierror.Append(createErr, err) + } + } + if ctx.Err() != nil { + return ctx.Err() } } - return nil + return createErr } -func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, - serviceType storage.ServiceType, - initialCertsSupplier func(context.Context) (*storage.TypedServiceCertificateSet, error), - sensorDeployment *appsApiv1.Deployment, secretName string) (*v1.Secret, error) { - secret, err := r.secretsClient.Get(ctx, secretName, metav1.GetOptions{}) +func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, caPem []byte, + certificate *storage.TypedServiceCertificate, secretSpec ServiceCertSecretSpec) (*v1.Secret, error) { + secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { - initialCerts, getInitialCertsErr := initialCertsSupplier(ctx) - if getInitialCertsErr != nil { - return nil, getInitialCertsErr - } - cert, getCertErr := r.certificateForService(initialCerts, serviceType) - if getCertErr != nil { - return nil, getCertErr - } - secretData, dataForCertErr := r.secretDataForCertificate(initialCerts.GetCaPem(), cert) - if dataForCertErr != nil { - return nil, dataForCertErr - } - sensorDeploymentGVK := sensorDeployment.GroupVersionKind() blockOwnerDeletion := false isController := false newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: sensorDeployment.GetNamespace(), + Name: secretSpec.secretName, + Namespace: r.namespace, OwnerReferences: []metav1.OwnerReference{ { - APIVersion: sensorDeploymentGVK.GroupVersion().String(), - Kind: sensorDeploymentGVK.Kind, - Name: sensorDeployment.GetName(), - UID: sensorDeployment.GetUID(), + APIVersion: r.ownerReference.APIVersion, + Kind: r.ownerReference.Kind, + Name: r.ownerReference.Name, + UID: r.ownerReference.UID, BlockOwnerDeletion: &blockOwnerDeletion, Controller: &isController, }, }, }, - Data: secretData, + Data: r.secretDataForCertificate(secretSpec, caPem, certificate), }, metav1.CreateOptions{}) if createErr != nil { return nil, createErr @@ -257,38 +254,14 @@ func (r *serviceCertificatesRepoSecretsImpl) setupSecret(ctx context.Context, return nil, err } - ownerReferences := secret.GetOwnerReferences() - if len(ownerReferences) != 1 { - return nil, ErrSensorDoesNotOwnCertSecrets - } - - ownerRef := ownerReferences[0] - if ownerRef.UID != sensorDeployment.GetUID() { - return nil, ErrSensorDoesNotOwnCertSecrets - } - return secret, nil } -func (r *serviceCertificatesRepoSecretsImpl) certificateForService(certs *storage.TypedServiceCertificateSet, - serviceType storage.ServiceType) (*storage.TypedServiceCertificate, error) { - for _, cert := range certs.GetServiceCerts() { - if cert.GetServiceType() == serviceType { - return cert, nil - } - } - return nil, errors.Errorf("no certificate found for service type %q", serviceType) -} - -func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(caPem []byte, - cert *storage.TypedServiceCertificate) (map[string][]byte, error) { - secretSpec, ok := r.secrets[cert.GetServiceType()] - if !ok { - return nil, errors.Errorf("unkown service type %q", cert.GetServiceType()) - } +func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(secretSpec ServiceCertSecretSpec, caPem []byte, + cert *storage.TypedServiceCertificate) map[string][]byte { return map[string][]byte{ secretSpec.caCertFileName: caPem, secretSpec.serviceCertFileName: cert.GetCert().GetCertPem(), secretSpec.serviceKeyFileName: cert.GetCert().GetKeyPem(), - }, nil + } } diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 233fa490f1bdc..22876cb4ed2a5 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -102,7 +102,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret1, anotherServiceType: secret2} clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) - repo := newTestRepo(secrets, sensorDeployment, secretsClient) + repo := newTestRepo(secrets, secretsClient) _, err := repo.GetServiceCertificates(context.Background()) if tc.expectError { s.Error(err) @@ -258,7 +258,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) - repo := newTestRepo(secrets, sensorDeployment, secretsClient) + repo := newTestRepo(secrets, secretsClient) return &certSecretsRepoFixture{ repo: repo, secretsClient: secretsClient, @@ -283,7 +283,6 @@ func sensorOwnerReference() []metav1.OwnerReference { } func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, - sensorDeployment *appsApiv1.Deployment, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) @@ -297,8 +296,9 @@ func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, } return &serviceCertificatesRepoSecretsImpl{ - secrets: secretsSpec, - sensorDeployment: sensorDeployment, - secretsClient: secretsClient, + secrets: secretsSpec, + ownerReference: sensorOwnerReference()[0], + namespace: namespace, + secretsClient: secretsClient, } } From ca390065c020050b5bad766d50d4614d26404198 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 12:10:01 +0100 Subject: [PATCH 32/49] make methods private --- .../service_certificates_repository.go | 33 +++++----- .../service_certificates_repository_test.go | 65 ++++++++++++------- 2 files changed, 59 insertions(+), 39 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 1b470ff7cbd7f..ba7b96077d5ad 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -33,28 +33,28 @@ var ( // serviceCertificatesRepoSecretsImpl is a ServiceCertificatesRepo that uses k8s secrets for persistence. type serviceCertificatesRepoSecretsImpl struct { - secrets map[storage.ServiceType]ServiceCertSecretSpec + secrets map[storage.ServiceType]serviceCertSecretSpec ownerReference metav1.OwnerReference namespace string secretsClient corev1.SecretInterface } -// ServiceCertSecretSpec species the name of the secret where certificates for a service are stored, and +// serviceCertSecretSpec species the name of the secret where certificates for a service are stored, and // the secret data keys where each certificate file is stored. -type ServiceCertSecretSpec struct { +type serviceCertSecretSpec struct { secretName string caCertFileName string serviceCertFileName string serviceKeyFileName string } -// NewServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists certificates for -// scanner and scanner DB in k8s secrets with the secret name and secret data path specified in ServiceCertSecretSpec. -func NewServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace string, +// newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists certificates for +// scanner and scanner DB in k8s secrets with the secret name and secret data path specified in serviceCertSecretSpec. +func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace string, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { return &serviceCertificatesRepoSecretsImpl{ - secrets: map[storage.ServiceType]ServiceCertSecretSpec{ + secrets: map[storage.ServiceType]serviceCertSecretSpec{ storage.ServiceType_SCANNER_SERVICE: { secretName: "scanner-slim-tls", caCertFileName: mtls.CACertFileName, @@ -74,13 +74,13 @@ func NewServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace } } -// GetServiceCertificates behaves as follows in case of missing data in the secrets: +// getServiceCertificates behaves as follows in case of missing data in the secrets: // - Fails with ErrMissingSecretData in case any secret has no data. // - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. // - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. // - Fails ErrSensorDoesNotOwnCertSecrets in case sensor deployment is not the sole owner of all secrets. -func (r *serviceCertificatesRepoSecretsImpl) GetServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { +func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) for serviceType, secretSpec := range r.secrets { @@ -106,7 +106,7 @@ func (r *serviceCertificatesRepoSecretsImpl) GetServiceCertificates(ctx context. } func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, serviceType storage.ServiceType, - secretSpec ServiceCertSecretSpec) (cert *storage.TypedServiceCertificate, ca []byte, err error) { + secretSpec serviceCertSecretSpec) (cert *storage.TypedServiceCertificate, ca []byte, err error) { secret, getErr := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if getErr != nil { return nil, nil, getErr @@ -139,12 +139,12 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C }, secretData[secretSpec.caCertFileName], nil } -// PutServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted +// putServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted // while others are not. // Edge cases: // - Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. // - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. -func (r *serviceCertificatesRepoSecretsImpl) PutServiceCertificates(ctx context.Context, +func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { caPem := certificates.GetCaPem() for _, cert := range certificates.GetServiceCerts() { @@ -193,10 +193,11 @@ type patchSecretDataByteMap struct { Value map[string][]byte `json:"value"` } -// CreateSecrets ensures the k8s secrets where we store the certificates are created. +// createSecrets ensures the k8s secrets for initialCertificates are created. // In case a secret doesn't exist this creates it setting sensorDeployment as owner, with initialCertificates stored // in the secret data. -func (r *serviceCertificatesRepoSecretsImpl) CreateSecrets(ctx context.Context, +// This only creates secrets for the service types that appear in initialCertificates. +func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, initialCertificates *storage.TypedServiceCertificateSet) error { var createErr error @@ -221,7 +222,7 @@ func (r *serviceCertificatesRepoSecretsImpl) CreateSecrets(ctx context.Context, } func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, caPem []byte, - certificate *storage.TypedServiceCertificate, secretSpec ServiceCertSecretSpec) (*v1.Secret, error) { + certificate *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) (*v1.Secret, error) { secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { @@ -257,7 +258,7 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, c return secret, nil } -func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(secretSpec ServiceCertSecretSpec, caPem []byte, +func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(secretSpec serviceCertSecretSpec, caPem []byte, cert *storage.TypedServiceCertificate) map[string][]byte { return map[string][]byte{ secretSpec.caCertFileName: caPem, diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 22876cb4ed2a5..a2120c3e00d33 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -27,7 +27,20 @@ var ( errForced = errors.New("forced error") serviceType = storage.ServiceType_SCANNER_SERVICE anotherServiceType = storage.ServiceType_SENSOR_SERVICE - sensorDeployment = &appsApiv1.Deployment{ + serviceCertificate = &storage.TypedServiceCertificate{ + ServiceType: serviceType, + Cert: &storage.ServiceCertificate{ + CertPem: make([]byte, 0), + KeyPem: make([]byte, 1), + }, + } + certificates = &storage.TypedServiceCertificateSet{ + CaPem: make([]byte, 2), + ServiceCerts: []*storage.TypedServiceCertificate{ + serviceCertificate, + }, + } + sensorDeployment = &appsApiv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: "sensor-deployment", Namespace: namespace, @@ -60,7 +73,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { cancelGetCtx() } - certificates, err := tc.fixture.repo.GetServiceCertificates(getCtx) + certificates, err := tc.fixture.repo.getServiceCertificates(getCtx) if tc.expectedErr == nil { s.Equal(tc.fixture.certificates, certificates) } @@ -103,7 +116,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) repo := newTestRepo(secrets, secretsClient) - _, err := repo.GetServiceCertificates(context.Background()) + _, err := repo.getServiceCertificates(context.Background()) if tc.expectError { s.Error(err) } else { @@ -130,7 +143,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { cancelPutCtx() } - err := tc.fixture.repo.PutServiceCertificates(putCtx, tc.fixture.certificates) + err := tc.fixture.repo.putServiceCertificates(putCtx, tc.fixture.certificates) s.checkExpectedError(tc.expectedErr, err) }) @@ -140,7 +153,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { fixture := s.newFixtureAdvancedOpts("", true) - _, err := fixture.repo.GetServiceCertificates(context.Background()) + _, err := fixture.repo.getServiceCertificates(context.Background()) s.ErrorIs(err, ErrMissingSecretData) } @@ -171,7 +184,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu for tcName, tc := range testCases { s.Run(tcName, func() { fixture := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) - certificates, err := fixture.repo.GetServiceCertificates(context.Background()) + certificates, err := fixture.repo.getServiceCertificates(context.Background()) tc.setExpectedCertsFunc(fixture.certificates) s.NoError(err) @@ -183,17 +196,35 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { fixture := s.newFixture("") s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType - err := fixture.repo.PutServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) s.Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { fixture := s.newFixture("") fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - err := fixture.repo.PutServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) s.NoError(err) } +func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsNoCertificatesSuccess() { + clientSet := fake.NewSimpleClientset(sensorDeployment) + secretsClient := clientSet.CoreV1().Secrets(namespace) + repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) + + s.NoError(repo.createSecrets(context.Background(), nil)) +} + +func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsCancelFailure() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + clientSet := fake.NewSimpleClientset(sensorDeployment) + secretsClient := clientSet.CoreV1().Secrets(namespace) + repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) + + s.Error(repo.createSecrets(ctx, certificates.Clone())) +} + func (s *serviceCertificatesRepoSecretsImplSuite) checkExpectedError(expectedErr, err error) { if expectedErr != errForced { s.Equal(expectedErr, err) @@ -222,19 +253,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(verbToError string) func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToError string, emptySecretData bool, missingSecretDataKeys ...string) *certSecretsRepoFixture { - serviceCertificate := &storage.TypedServiceCertificate{ - ServiceType: serviceType, - Cert: &storage.ServiceCertificate{ - CertPem: make([]byte, 0), - KeyPem: make([]byte, 1), - }, - } - certificates := &storage.TypedServiceCertificateSet{ - CaPem: make([]byte, 2), - ServiceCerts: []*storage.TypedServiceCertificate{ - serviceCertificate, - }, - } + certificates := certificates.Clone() secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s-secret", serviceType), @@ -285,9 +304,9 @@ func sensorOwnerReference() []metav1.OwnerReference { func newTestRepo(secrets map[storage.ServiceType]*v1.Secret, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { - secretsSpec := make(map[storage.ServiceType]ServiceCertSecretSpec) + secretsSpec := make(map[storage.ServiceType]serviceCertSecretSpec) for serviceType, secret := range secrets { - secretsSpec[serviceType] = ServiceCertSecretSpec{ + secretsSpec[serviceType] = serviceCertSecretSpec{ secretName: secret.Name, caCertFileName: mtls.CACertFileName, serviceCertFileName: mtls.ServiceCertFileName, From 963539c5c93e6dbbffcdab141c1cb1624b1fedf4 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 12:11:25 +0100 Subject: [PATCH 33/49] simplify owner reference management --- .../service_certificates_repository.go | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index ba7b96077d5ad..3d9ccd38c38e4 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -226,22 +226,11 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, c secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { - blockOwnerDeletion := false - isController := false newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: secretSpec.secretName, - Namespace: r.namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: r.ownerReference.APIVersion, - Kind: r.ownerReference.Kind, - Name: r.ownerReference.Name, - UID: r.ownerReference.UID, - BlockOwnerDeletion: &blockOwnerDeletion, - Controller: &isController, - }, - }, + Name: secretSpec.secretName, + Namespace: r.namespace, + OwnerReferences: []metav1.OwnerReference{r.ownerReference}, }, Data: r.secretDataForCertificate(secretSpec, caPem, certificate), }, metav1.CreateOptions{}) From 64ea16b0263726a0e42db1d87ce46823aeb19560 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 12:16:15 +0100 Subject: [PATCH 34/49] polish comments --- .../service_certificates_repository.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 3d9ccd38c38e4..671b27e925a19 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -17,9 +17,9 @@ import ( ) var ( - // ErrSensorDoesNotOwnCertSecrets indicates that this repository should not be updating the certificates in - // the secrets because the owner of the secrets is not the deployment for sensor. - ErrSensorDoesNotOwnCertSecrets = errors.New("sensor deployment does not own certificate secrets") + // ErrUnexpectedSecretsOwner indicates that this repository should not be updating the certificates in + // the secrets they do not have the expected owner. + ErrUnexpectedSecretsOwner = errors.New("unexpected owner for certificate secrets") // ErrDifferentCAForDifferentServiceTypes indicates that different service types have different values // for CA stored in their secrets. @@ -49,7 +49,7 @@ type serviceCertSecretSpec struct { } // newServiceCertificatesRepo creates a new serviceCertificatesRepoSecretsImpl that persists certificates for -// scanner and scanner DB in k8s secrets with the secret name and secret data path specified in serviceCertSecretSpec. +// scanner and scanner DB in k8s secrets that are expected to have ownerReference as the only owner reference. func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace string, secretsClient corev1.SecretInterface) *serviceCertificatesRepoSecretsImpl { @@ -79,7 +79,7 @@ func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace // - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. // - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. -// - Fails ErrSensorDoesNotOwnCertSecrets in case sensor deployment is not the sole owner of all secrets. +// - Fails ErrUnexpectedSecretsOwner in case sensor deployment is not the sole owner of all secrets. func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) @@ -114,7 +114,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C ownerReferences := secret.GetOwnerReferences() if len(ownerReferences) != 1 { - return nil, nil, ErrSensorDoesNotOwnCertSecrets + return nil, nil, ErrUnexpectedSecretsOwner } ownerRef := ownerReferences[0] @@ -122,7 +122,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C ownerRef.Kind == r.ownerReference.Kind && ownerRef.Name == r.ownerReference.Name && ownerRef.UID == r.ownerReference.UID) { - return nil, nil, ErrSensorDoesNotOwnCertSecrets + return nil, nil, ErrUnexpectedSecretsOwner } secretData := secret.Data @@ -194,8 +194,8 @@ type patchSecretDataByteMap struct { } // createSecrets ensures the k8s secrets for initialCertificates are created. -// In case a secret doesn't exist this creates it setting sensorDeployment as owner, with initialCertificates stored -// in the secret data. +// In case a secret doesn't exist it creates that secret, setting sensorDeployment as owner, +// with initialCertificates stored in the secret data. // This only creates secrets for the service types that appear in initialCertificates. func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, initialCertificates *storage.TypedServiceCertificateSet) error { From 3650011eb8bc78638885f1f47499b8cfe775af70 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 14:10:53 +0100 Subject: [PATCH 35/49] always create the secrets in createSecrets --- .../service_certificates_repository.go | 39 ++++++------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 671b27e925a19..67c686bd8a3e5 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -10,7 +10,6 @@ import ( "github.com/stackrox/rox/generated/storage" "github.com/stackrox/rox/pkg/mtls" v1 "k8s.io/api/core/v1" - k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sTypes "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -79,7 +78,7 @@ func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace // - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. // - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. -// - Fails ErrUnexpectedSecretsOwner in case sensor deployment is not the sole owner of all secrets. +// - Fails ErrUnexpectedSecretsOwner in case the owner specified in the constructor is not the sole owner of all secrets. func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) @@ -193,9 +192,9 @@ type patchSecretDataByteMap struct { Value map[string][]byte `json:"value"` } -// createSecrets ensures the k8s secrets for initialCertificates are created. -// In case a secret doesn't exist it creates that secret, setting sensorDeployment as owner, -// with initialCertificates stored in the secret data. +// createSecrets creates the k8s secrets for initialCertificates. +// Each secret is created with the owner specified in the constructor as owner, and with initialCertificates stored +// in the secret data. // This only creates secrets for the service types that appear in initialCertificates. func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, initialCertificates *storage.TypedServiceCertificateSet) error { @@ -223,28 +222,14 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, caPem []byte, certificate *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) (*v1.Secret, error) { - - secret, err := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) - if k8sErrors.IsNotFound(err) { - newSecret, createErr := r.secretsClient.Create(ctx, &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretSpec.secretName, - Namespace: r.namespace, - OwnerReferences: []metav1.OwnerReference{r.ownerReference}, - }, - Data: r.secretDataForCertificate(secretSpec, caPem, certificate), - }, metav1.CreateOptions{}) - if createErr != nil { - return nil, createErr - } - return newSecret, nil - } - - if err != nil { - return nil, err - } - - return secret, nil + return r.secretsClient.Create(ctx, &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretSpec.secretName, + Namespace: r.namespace, + OwnerReferences: []metav1.OwnerReference{r.ownerReference}, + }, + Data: r.secretDataForCertificate(secretSpec, caPem, certificate), + }, metav1.CreateOptions{}) } func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(secretSpec serviceCertSecretSpec, caPem []byte, From 8456e489cd1c3971459756230cd722bd5a15dcbc Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Thu, 10 Feb 2022 19:29:01 +0100 Subject: [PATCH 36/49] return highest priority error in getServiceCertificates --- .../service_certificates_repository.go | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 67c686bd8a3e5..8ecb811e5ef4d 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -73,25 +73,31 @@ func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace } } -// getServiceCertificates behaves as follows in case of missing data in the secrets: -// - Fails with ErrMissingSecretData in case any secret has no data. +// getServiceCertificates behaves as follows in case of errors: +// - Fails as soon as the context is cancelled. +// - Otherwise, fails with ErrUnexpectedSecretsOwner in case the owner specified in the constructor is not the sole owner of all secrets. +// - Otherwise, fails with ErrMissingSecretData in case any secret has no data. +// - Otherwise, fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. +// - Otherwise, fails with any k8s API error. // - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. // for that secret will contain a zero value. -// - Fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. -// - Fails ErrUnexpectedSecretsOwner in case the owner specified in the constructor is not the sole owner of all secrets. func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + + errors := make(map[error]bool) for serviceType, secretSpec := range r.secrets { certificate, ca, err := r.getServiceCertificate(ctx, serviceType, secretSpec) if err != nil { - return nil, err + errors[err] = true + continue } if certificates.GetCaPem() == nil { certificates.CaPem = ca } else { if !bytes.Equal(certificates.GetCaPem(), ca) { - return nil, ErrDifferentCAForDifferentServiceTypes + errors[ErrDifferentCAForDifferentServiceTypes] = true + continue } } certificates.ServiceCerts = append(certificates.ServiceCerts, certificate) @@ -101,6 +107,20 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. } } + // TODO: if this works abstract as "highestPriorityError" func + if _, ok := errors[ErrUnexpectedSecretsOwner]; ok { + return nil, ErrUnexpectedSecretsOwner + } + if _, ok := errors[ErrMissingSecretData]; ok { + return nil, ErrMissingSecretData + } + if _, ok := errors[ErrDifferentCAForDifferentServiceTypes]; ok { + return nil, ErrDifferentCAForDifferentServiceTypes + } + for err := range errors { + return nil, err + } + return certificates, nil } From b0f5e69264eb79ee20ebb20e53a4a99a05b8a8d4 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:23:24 +0100 Subject: [PATCH 37/49] use our "errors" package --- .../localscanner/service_certificates_repository_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index a2120c3e00d33..0676786393465 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -2,10 +2,10 @@ package localscanner import ( "context" - "errors" "fmt" "testing" + "github.com/pkg/errors" "github.com/stackrox/rox/generated/storage" "github.com/stackrox/rox/pkg/mtls" "github.com/stretchr/testify/suite" From 5fc7892c4edd4829507bb785b3f28ba83f1a0a70 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:32:38 +0100 Subject: [PATCH 38/49] return multierror in get and put also fix some comments --- .../service_certificates_repository.go | 48 ++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 8ecb811e5ef4d..aaf367c839b8a 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -38,7 +38,7 @@ type serviceCertificatesRepoSecretsImpl struct { secretsClient corev1.SecretInterface } -// serviceCertSecretSpec species the name of the secret where certificates for a service are stored, and +// serviceCertSecretSpec specifies the name of the secret where certificates for a service are stored, and // the secret data keys where each certificate file is stored. type serviceCertSecretSpec struct { secretName string @@ -73,30 +73,28 @@ func newServiceCertificatesRepo(ownerReference metav1.OwnerReference, namespace } } -// getServiceCertificates behaves as follows in case of errors: -// - Fails as soon as the context is cancelled. -// - Otherwise, fails with ErrUnexpectedSecretsOwner in case the owner specified in the constructor is not the sole owner of all secrets. -// - Otherwise, fails with ErrMissingSecretData in case any secret has no data. -// - Otherwise, fails with ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. -// - Otherwise, fails with any k8s API error. -// - If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. -// for that secret will contain a zero value. +// getServiceCertificates fails as soon as the context is cancelled. Otherwise it returns a multierror that can contain +// the following errors: +// - ErrUnexpectedSecretsOwner in case the owner specified in the constructor is not the sole owner of all secrets. +// - ErrMissingSecretData in case any secret has no data. +// - ErrDifferentCAForDifferentServiceTypes in case the CA is not the same in all secrets. +// If the data for a secret is missing some expecting key then the corresponding field in the TypedServiceCertificate. +// for that secret will contain a zero value. func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context.Context) (*storage.TypedServiceCertificateSet, error) { certificates := &storage.TypedServiceCertificateSet{} certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - - errors := make(map[error]bool) + var getErr error for serviceType, secretSpec := range r.secrets { certificate, ca, err := r.getServiceCertificate(ctx, serviceType, secretSpec) if err != nil { - errors[err] = true + getErr = multierror.Append(getErr, err) continue } if certificates.GetCaPem() == nil { certificates.CaPem = ca } else { if !bytes.Equal(certificates.GetCaPem(), ca) { - errors[ErrDifferentCAForDifferentServiceTypes] = true + getErr = multierror.Append(getErr, ErrDifferentCAForDifferentServiceTypes) continue } } @@ -107,18 +105,8 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. } } - // TODO: if this works abstract as "highestPriorityError" func - if _, ok := errors[ErrUnexpectedSecretsOwner]; ok { - return nil, ErrUnexpectedSecretsOwner - } - if _, ok := errors[ErrMissingSecretData]; ok { - return nil, ErrMissingSecretData - } - if _, ok := errors[ErrDifferentCAForDifferentServiceTypes]; ok { - return nil, ErrDifferentCAForDifferentServiceTypes - } - for err := range errors { - return nil, err + if getErr != nil { + return nil, getErr } return certificates, nil @@ -126,6 +114,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.Context, serviceType storage.ServiceType, secretSpec serviceCertSecretSpec) (cert *storage.TypedServiceCertificate, ca []byte, err error) { + secret, getErr := r.secretsClient.Get(ctx, secretSpec.secretName, metav1.GetOptions{}) if getErr != nil { return nil, nil, getErr @@ -165,10 +154,13 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C // - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { + caPem := certificates.GetCaPem() + var putErr error for _, cert := range certificates.GetServiceCerts() { if err := r.putServiceCertificate(ctx, caPem, cert); err != nil { - return err + putErr = multierror.Append(putErr, err) + continue } // on context cancellation abort putting other secrets. @@ -177,7 +169,7 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. } } - return nil + return putErr } func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, @@ -242,6 +234,7 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, caPem []byte, certificate *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) (*v1.Secret, error) { + return r.secretsClient.Create(ctx, &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretSpec.secretName, @@ -254,6 +247,7 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, c func (r *serviceCertificatesRepoSecretsImpl) secretDataForCertificate(secretSpec serviceCertSecretSpec, caPem []byte, cert *storage.TypedServiceCertificate) map[string][]byte { + return map[string][]byte{ secretSpec.caCertFileName: caPem, secretSpec.serviceCertFileName: cert.GetCert().GetCertPem(), From fcb0a109dbf1dce3719dcbb21f9300700a889a8b Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:37:48 +0100 Subject: [PATCH 39/49] just check the UID of the owner reference in get --- .../localscanner/service_certificates_repository.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index aaf367c839b8a..15b4d826284f6 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -125,11 +125,7 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C return nil, nil, ErrUnexpectedSecretsOwner } - ownerRef := ownerReferences[0] - if !(ownerRef.APIVersion == r.ownerReference.APIVersion && - ownerRef.Kind == r.ownerReference.Kind && - ownerRef.Name == r.ownerReference.Name && - ownerRef.UID == r.ownerReference.UID) { + if ownerReferences[0].UID != r.ownerReference.UID { return nil, nil, ErrUnexpectedSecretsOwner } From 0fd69a31f0fb6d211c600d503504852a5b3a3aa4 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:42:44 +0100 Subject: [PATCH 40/49] use named parameters in test case structs --- .../service_certificates_repository_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 0676786393465..92e353d91f202 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -61,9 +61,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful get": {nil, s.newFixture("")}, - "failed get": {errForced, s.newFixture("get")}, - "cancelled get": {context.Canceled, s.newFixture("")}, + "successful get": {expectedErr: nil, fixture: s.newFixture("")}, + "failed get": {expectedErr: errForced, fixture: s.newFixture("get")}, + "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture("")}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -87,8 +87,8 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { expectError bool secondCASize int }{ - "same CAs successful get": {false, 0}, - "different CAs failed get": {true, 1}, + "same CAs successful get": {expectError: false, secondCASize: 0}, + "different CAs failed get": {expectError: true, secondCASize: 1}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -131,9 +131,9 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful put": {nil, s.newFixture("")}, - "failed put": {errForced, s.newFixture("patch")}, - "cancelled put": {context.Canceled, s.newFixture("")}, + "successful put": {expectedErr: nil, fixture: s.newFixture("")}, + "failed put": {expectedErr: errForced, fixture: s.newFixture("patch")}, + "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture("")}, } for tcName, tc := range testCases { s.Run(tcName, func() { From 4c52fbb5600e8f10bb17919d65fa51b015c37f8c Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:46:27 +0100 Subject: [PATCH 41/49] use s.ErrorIs instead of ad-hoc function --- .../service_certificates_repository_test.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 92e353d91f202..a1e3209919ee1 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -77,7 +77,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { if tc.expectedErr == nil { s.Equal(tc.fixture.certificates, certificates) } - s.checkExpectedError(tc.expectedErr, err) + s.ErrorIs(err, tc.expectedErr) }) } } @@ -145,7 +145,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { err := tc.fixture.repo.putServiceCertificates(putCtx, tc.fixture.certificates) - s.checkExpectedError(tc.expectedErr, err) + s.ErrorIs(err, tc.expectedErr) }) } } @@ -225,15 +225,6 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsCancelFailure s.Error(repo.createSecrets(ctx, certificates.Clone())) } -func (s *serviceCertificatesRepoSecretsImplSuite) checkExpectedError(expectedErr, err error) { - if expectedErr != errForced { - s.Equal(expectedErr, err) - } else { - // multierror wraps errForced - s.Error(err) - } -} - func (s *serviceCertificatesRepoSecretsImplSuite) getFirstServiceCertificate( certificates *storage.TypedServiceCertificateSet) *storage.TypedServiceCertificate { serviceCerts := certificates.GetServiceCerts() From a954e8fb5b0ad2bb4a4486c8bd1f6db57201457c Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 15:59:57 +0100 Subject: [PATCH 42/49] improve names of test cases due to k8s API errors --- .../service_certificates_repository_test.go | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index a1e3209919ee1..ef3034c72687e 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -62,7 +62,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { fixture *certSecretsRepoFixture }{ "successful get": {expectedErr: nil, fixture: s.newFixture("")}, - "failed get": {expectedErr: errForced, fixture: s.newFixture("get")}, + "failed get due to k8s API error": {expectedErr: errForced, fixture: s.newFixture("get")}, "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture("")}, } for tcName, tc := range testCases { @@ -74,6 +74,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { } certificates, err := tc.fixture.repo.getServiceCertificates(getCtx) + if tc.expectedErr == nil { s.Equal(tc.fixture.certificates, certificates) } @@ -84,11 +85,11 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { testCases := map[string]struct { - expectError bool + expectedErr error secondCASize int }{ - "same CAs successful get": {expectError: false, secondCASize: 0}, - "different CAs failed get": {expectError: true, secondCASize: 1}, + "same CAs successful get": {expectedErr: nil, secondCASize: 0}, + "different CAs failed get": {expectedErr: ErrDifferentCAForDifferentServiceTypes, secondCASize: 1}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -116,12 +117,10 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { clientSet := fake.NewSimpleClientset(secret1, secret2) secretsClient := clientSet.CoreV1().Secrets(namespace) repo := newTestRepo(secrets, secretsClient) + _, err := repo.getServiceCertificates(context.Background()) - if tc.expectError { - s.Error(err) - } else { - s.NoError(err) - } + + s.ErrorIs(err, tc.expectedErr) }) } } @@ -132,7 +131,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { fixture *certSecretsRepoFixture }{ "successful put": {expectedErr: nil, fixture: s.newFixture("")}, - "failed put": {expectedErr: errForced, fixture: s.newFixture("patch")}, + "failed put due to k8s API error": {expectedErr: errForced, fixture: s.newFixture("patch")}, "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture("")}, } for tcName, tc := range testCases { @@ -184,10 +183,11 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu for tcName, tc := range testCases { s.Run(tcName, func() { fixture := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) + certificates, err := fixture.repo.getServiceCertificates(context.Background()) - tc.setExpectedCertsFunc(fixture.certificates) - s.NoError(err) + s.Require().NoError(err) + tc.setExpectedCertsFunc(fixture.certificates) s.Equal(fixture.certificates, certificates) }) } @@ -196,14 +196,18 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { fixture := s.newFixture("") s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + s.Error(err) } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { fixture := s.newFixture("") fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) + err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + s.NoError(err) } @@ -220,6 +224,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsCancelFailure cancel() clientSet := fake.NewSimpleClientset(sensorDeployment) secretsClient := clientSet.CoreV1().Secrets(namespace) + repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) s.Error(repo.createSecrets(ctx, certificates.Clone())) From e0eda1e8ecbc598af396843af1a20550e340ff8f Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 16:32:49 +0100 Subject: [PATCH 43/49] improve readability of newFixture --- .../service_certificates_repository_test.go | 57 ++++++++++++------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index ef3034c72687e..55eb3934b9667 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -61,9 +61,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful get": {expectedErr: nil, fixture: s.newFixture("")}, - "failed get due to k8s API error": {expectedErr: errForced, fixture: s.newFixture("get")}, - "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture("")}, + "successful get": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "failed get due to k8s API error": { + expectedErr: errForced, + fixture: s.newFixture(certSecretsRepoFixtureSpec{k8sAPIVerbToError: "get"}), + }, + "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -130,9 +133,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful put": {expectedErr: nil, fixture: s.newFixture("")}, - "failed put due to k8s API error": {expectedErr: errForced, fixture: s.newFixture("patch")}, - "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture("")}, + "successful put": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "failed put due to k8s API error": { + expectedErr: errForced, + fixture: s.newFixture(certSecretsRepoFixtureSpec{k8sAPIVerbToError: "patch"}), + }, + "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -150,7 +156,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { } func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { - fixture := s.newFixtureAdvancedOpts("", true) + fixture := s.newFixture(certSecretsRepoFixtureSpec{emptySecretData: true}) _, err := fixture.repo.getServiceCertificates(context.Background()) @@ -182,7 +188,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } for tcName, tc := range testCases { s.Run(tcName, func() { - fixture := s.newFixtureAdvancedOpts("", false, tc.missingSecretDataKey) + fixture := s.newFixture(certSecretsRepoFixtureSpec{missingSecretDataKeys: []string{tc.missingSecretDataKey}}) certificates, err := fixture.repo.getServiceCertificates(context.Background()) @@ -194,7 +200,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { - fixture := s.newFixture("") + fixture := s.newFixture(certSecretsRepoFixtureSpec{}) s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) @@ -203,7 +209,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailu } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { - fixture := s.newFixture("") + fixture := s.newFixture(certSecretsRepoFixtureSpec{}) fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) @@ -243,12 +249,15 @@ type certSecretsRepoFixture struct { certificates *storage.TypedServiceCertificateSet } -func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(verbToError string) *certSecretsRepoFixture { - return s.newFixtureAdvancedOpts(verbToError, false) -} - -func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToError string, emptySecretData bool, - missingSecretDataKeys ...string) *certSecretsRepoFixture { +// newFixture creates a certSecretsRepoFixture that contains: +// - A secrets client corresponding to a fake k8s client set such that: +// - It is initialized to represent a cluster with sensorDeployment and a secret that contains certificates +// on its data, or partial data according to spec. +// - The client set will fail all operations on the HTTP verb indicated in spec. +// - The certificates used to initialize the data of the aforementioned secret. +// - A repository that uses that secrets client, sensorDeployment as owner, and with a single serviceCertSecretSpec +// for the aforementioned secret in its secrets. +func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(spec certSecretsRepoFixtureSpec) *certSecretsRepoFixture { certificates := certificates.Clone() secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -257,20 +266,20 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE OwnerReferences: sensorOwnerReference(), }, } - if !emptySecretData { + if !spec.emptySecretData { secret.Data = map[string][]byte{ mtls.CACertFileName: certificates.GetCaPem(), mtls.ServiceCertFileName: serviceCertificate.GetCert().GetCertPem(), mtls.ServiceKeyFileName: serviceCertificate.GetCert().GetKeyPem(), } } - for _, secretDataKey := range missingSecretDataKeys { + for _, secretDataKey := range spec.missingSecretDataKeys { delete(secret.Data, secretDataKey) } secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} clientSet := fake.NewSimpleClientset(sensorDeployment, secret) secretsClient := clientSet.CoreV1().Secrets(namespace) - clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(verbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { + clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(spec.k8sAPIVerbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) repo := newTestRepo(secrets, secretsClient) @@ -281,6 +290,16 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixtureAdvancedOpts(verbToE } } +type certSecretsRepoFixtureSpec struct { + // HTTP verb of the k8s API should for which all operations will fail in the fake k8s client set. + // Use the zero value so all operations work. + k8sAPIVerbToError string + // If true then the data of the secret used to initialize the fake k8s client set will be empty. + emptySecretData bool + // These keys will be removed from the data keys of the secret used to initialize the fake k8s client set. + missingSecretDataKeys []string +} + func sensorOwnerReference() []metav1.OwnerReference { sensorDeploymentGVK := sensorDeployment.GroupVersionKind() blockOwnerDeletion := false From 45b6a8175aaae842db34f84cc5ee5ec7703b8b4d Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 17:40:51 +0100 Subject: [PATCH 44/49] properly cancel on context cancellation --- .../service_certificates_repository.go | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index 15b4d826284f6..c381b96052e8b 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -85,6 +85,11 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) var getErr error for serviceType, secretSpec := range r.secrets { + // on context cancellation abort getting other secrets. + if ctx.Err() != nil { + return nil, ctx.Err() + } + certificate, ca, err := r.getServiceCertificate(ctx, serviceType, secretSpec) if err != nil { getErr = multierror.Append(getErr, err) @@ -99,10 +104,6 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificates(ctx context. } } certificates.ServiceCerts = append(certificates.ServiceCerts, certificate) - // on context cancellation abort getting other secrets. - if ctx.Err() != nil { - return nil, ctx.Err() - } } if getErr != nil { @@ -154,29 +155,28 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. caPem := certificates.GetCaPem() var putErr error for _, cert := range certificates.GetServiceCerts() { - if err := r.putServiceCertificate(ctx, caPem, cert); err != nil { - putErr = multierror.Append(putErr, err) - continue - } - // on context cancellation abort putting other secrets. if ctx.Err() != nil { return ctx.Err() } + + secretSpec, ok := r.secrets[cert.GetServiceType()] + if !ok { + // we don't know how to persist this. + err := errors.Errorf("unkown service type %q", cert.GetServiceType()) + putErr = multierror.Append(putErr, err) + continue + } + if err := r.putServiceCertificate(ctx, caPem, cert, secretSpec); err != nil { + putErr = multierror.Append(putErr, err) + } } return putErr } func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, - cert *storage.TypedServiceCertificate) error { - - secretSpec, ok := r.secrets[cert.GetServiceType()] - if !ok { - // we don't know how to persist this. - return errors.Errorf("unkown service type %q", cert.GetServiceType()) - } - + cert *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) error { patch := []patchSecretDataByteMap{{ Op: "replace", Path: "/data", @@ -209,19 +209,19 @@ func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, var createErr error for _, certificate := range initialCertificates.GetServiceCerts() { + if ctx.Err() != nil { + return ctx.Err() + } + secretSpec, ok := r.secrets[certificate.GetServiceType()] if !ok { err := errors.Errorf("unkown service type %q", certificate.GetServiceType()) createErr = multierror.Append(createErr, err) - } else { - _, createSecretErr := r.createSecret(ctx, initialCertificates.GetCaPem(), certificate, secretSpec) - if createSecretErr != nil { - err := errors.Wrapf(createSecretErr, errForServiceFormat, certificate.GetServiceType()) - createErr = multierror.Append(createErr, err) - } + continue } - if ctx.Err() != nil { - return ctx.Err() + if _, createSecretErr := r.createSecret(ctx, initialCertificates.GetCaPem(), certificate, secretSpec); createSecretErr != nil { + err := errors.Wrapf(createSecretErr, errForServiceFormat, certificate.GetServiceType()) + createErr = multierror.Append(createErr, err) } } From 32e1cb4218d1a44589022c79b776b2ebcb573b10 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Fri, 11 Feb 2022 17:53:54 +0100 Subject: [PATCH 45/49] unify put and update in ensure --- .../service_certificates_repository.go | 63 +++++++------------ .../service_certificates_repository_test.go | 20 ++++-- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository.go b/sensor/kubernetes/localscanner/service_certificates_repository.go index c381b96052e8b..ea677811c0e5f 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository.go @@ -10,6 +10,7 @@ import ( "github.com/stackrox/rox/generated/storage" "github.com/stackrox/rox/pkg/mtls" v1 "k8s.io/api/core/v1" + k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sTypes "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -144,16 +145,18 @@ func (r *serviceCertificatesRepoSecretsImpl) getServiceCertificate(ctx context.C }, secretData[secretSpec.caCertFileName], nil } -// putServiceCertificates is idempotent but not atomic in sense that on error some secrets might be persisted +// ensureServiceCertificates ensures the services for certificates exists, and that they contain the certificates +// in their data. +// This operation is idempotent but not atomic in sense that on error some secrets might be created and updated, // while others are not. -// Edge cases: -// - Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. -// - Not all services types in r.secrets are required to appear in certificates, missing service types are just skipped. -func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context.Context, +// Each missing secret is created with the owner specified in the constructor as owner. +// This only creates secrets for the service types that appear in certificates, missing service types are just skipped. +// Fails for certificates with a service type that doesn't appear in r.secrets, as we don't know where to store them. +func (r *serviceCertificatesRepoSecretsImpl) ensureServiceCertificates(ctx context.Context, certificates *storage.TypedServiceCertificateSet) error { caPem := certificates.GetCaPem() - var putErr error + var serviceErrors error for _, cert := range certificates.GetServiceCerts() { // on context cancellation abort putting other secrets. if ctx.Err() != nil { @@ -164,18 +167,28 @@ func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificates(ctx context. if !ok { // we don't know how to persist this. err := errors.Errorf("unkown service type %q", cert.GetServiceType()) - putErr = multierror.Append(putErr, err) + serviceErrors = multierror.Append(serviceErrors, err) continue } - if err := r.putServiceCertificate(ctx, caPem, cert, secretSpec); err != nil { - putErr = multierror.Append(putErr, err) + if err := r.ensureServiceCertificate(ctx, caPem, cert, secretSpec); err != nil { + serviceErrors = multierror.Append(serviceErrors, err) } } - return putErr + return serviceErrors } -func (r *serviceCertificatesRepoSecretsImpl) putServiceCertificate(ctx context.Context, caPem []byte, +func (r *serviceCertificatesRepoSecretsImpl) ensureServiceCertificate(ctx context.Context, caPem []byte, + cert *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) error { + patchErr := r.patchServiceCertificate(ctx, caPem, cert, secretSpec) + if k8sErrors.IsNotFound(patchErr) { + _, createErr := r.createSecret(ctx, caPem, cert, secretSpec) + return createErr + } + return patchErr +} + +func (r *serviceCertificatesRepoSecretsImpl) patchServiceCertificate(ctx context.Context, caPem []byte, cert *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) error { patch := []patchSecretDataByteMap{{ Op: "replace", @@ -200,34 +213,6 @@ type patchSecretDataByteMap struct { Value map[string][]byte `json:"value"` } -// createSecrets creates the k8s secrets for initialCertificates. -// Each secret is created with the owner specified in the constructor as owner, and with initialCertificates stored -// in the secret data. -// This only creates secrets for the service types that appear in initialCertificates. -func (r *serviceCertificatesRepoSecretsImpl) createSecrets(ctx context.Context, - initialCertificates *storage.TypedServiceCertificateSet) error { - - var createErr error - for _, certificate := range initialCertificates.GetServiceCerts() { - if ctx.Err() != nil { - return ctx.Err() - } - - secretSpec, ok := r.secrets[certificate.GetServiceType()] - if !ok { - err := errors.Errorf("unkown service type %q", certificate.GetServiceType()) - createErr = multierror.Append(createErr, err) - continue - } - if _, createSecretErr := r.createSecret(ctx, initialCertificates.GetCaPem(), certificate, secretSpec); createSecretErr != nil { - err := errors.Wrapf(createSecretErr, errForServiceFormat, certificate.GetServiceType()) - createErr = multierror.Append(createErr, err) - } - } - - return createErr -} - func (r *serviceCertificatesRepoSecretsImpl) createSecret(ctx context.Context, caPem []byte, certificate *storage.TypedServiceCertificate, secretSpec serviceCertSecretSpec) (*v1.Secret, error) { diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 55eb3934b9667..269e06fddcba2 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -148,7 +148,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { cancelPutCtx() } - err := tc.fixture.repo.putServiceCertificates(putCtx, tc.fixture.certificates) + err := tc.fixture.repo.ensureServiceCertificates(putCtx, tc.fixture.certificates) s.ErrorIs(err, tc.expectedErr) }) @@ -203,7 +203,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailu fixture := s.newFixture(certSecretsRepoFixtureSpec{}) s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType - err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.ensureServiceCertificates(context.Background(), fixture.certificates) s.Error(err) } @@ -212,7 +212,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSucce fixture := s.newFixture(certSecretsRepoFixtureSpec{}) fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) - err := fixture.repo.putServiceCertificates(context.Background(), fixture.certificates) + err := fixture.repo.ensureServiceCertificates(context.Background(), fixture.certificates) s.NoError(err) } @@ -222,7 +222,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsNoCertificate secretsClient := clientSet.CoreV1().Secrets(namespace) repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) - s.NoError(repo.createSecrets(context.Background(), nil)) + s.NoError(repo.ensureServiceCertificates(context.Background(), nil)) } func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsCancelFailure() { @@ -233,7 +233,17 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestCreateSecretsCancelFailure repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) - s.Error(repo.createSecrets(ctx, certificates.Clone())) + s.Error(repo.ensureServiceCertificates(ctx, certificates.Clone())) +} + +func (s *serviceCertificatesRepoSecretsImplSuite) TestEnsureServiceCertificateMissingSecretSuccess() { + clientSet := fake.NewSimpleClientset(sensorDeployment) + secretsClient := clientSet.CoreV1().Secrets(namespace) + repo := newServiceCertificatesRepo(sensorOwnerReference()[0], namespace, secretsClient) + + err := repo.ensureServiceCertificates(context.Background(), certificates) + + s.NoError(err) } func (s *serviceCertificatesRepoSecretsImplSuite) getFirstServiceCertificate( From 931609c80c78fbb405801db1d84b8c50653e4a36 Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 14 Feb 2022 10:18:40 +0100 Subject: [PATCH 46/49] rename certSecretsRepoFixtureSpec to certSecretsRepoFixtureConfig --- .../service_certificates_repository_test.go | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 269e06fddcba2..9c22ab812b160 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -61,12 +61,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGet() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful get": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "successful get": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, "failed get due to k8s API error": { expectedErr: errForced, - fixture: s.newFixture(certSecretsRepoFixtureSpec{k8sAPIVerbToError: "get"}), + fixture: s.newFixture(certSecretsRepoFixtureConfig{k8sAPIVerbToError: "get"}), }, - "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "cancelled get": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -133,12 +133,12 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { expectedErr error fixture *certSecretsRepoFixture }{ - "successful put": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "successful put": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, "failed put due to k8s API error": { expectedErr: errForced, - fixture: s.newFixture(certSecretsRepoFixtureSpec{k8sAPIVerbToError: "patch"}), + fixture: s.newFixture(certSecretsRepoFixtureConfig{k8sAPIVerbToError: "patch"}), }, - "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureSpec{})}, + "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, } for tcName, tc := range testCases { s.Run(tcName, func() { @@ -156,7 +156,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { } func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { - fixture := s.newFixture(certSecretsRepoFixtureSpec{emptySecretData: true}) + fixture := s.newFixture(certSecretsRepoFixtureConfig{emptySecretData: true}) _, err := fixture.repo.getServiceCertificates(context.Background()) @@ -188,7 +188,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } for tcName, tc := range testCases { s.Run(tcName, func() { - fixture := s.newFixture(certSecretsRepoFixtureSpec{missingSecretDataKeys: []string{tc.missingSecretDataKey}}) + fixture := s.newFixture(certSecretsRepoFixtureConfig{missingSecretDataKeys: []string{tc.missingSecretDataKey}}) certificates, err := fixture.repo.getServiceCertificates(context.Background()) @@ -200,7 +200,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { - fixture := s.newFixture(certSecretsRepoFixtureSpec{}) + fixture := s.newFixture(certSecretsRepoFixtureConfig{}) s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType err := fixture.repo.ensureServiceCertificates(context.Background(), fixture.certificates) @@ -209,7 +209,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailu } func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { - fixture := s.newFixture(certSecretsRepoFixtureSpec{}) + fixture := s.newFixture(certSecretsRepoFixtureConfig{}) fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) err := fixture.repo.ensureServiceCertificates(context.Background(), fixture.certificates) @@ -267,7 +267,7 @@ type certSecretsRepoFixture struct { // - The certificates used to initialize the data of the aforementioned secret. // - A repository that uses that secrets client, sensorDeployment as owner, and with a single serviceCertSecretSpec // for the aforementioned secret in its secrets. -func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(spec certSecretsRepoFixtureSpec) *certSecretsRepoFixture { +func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(spec certSecretsRepoFixtureConfig) *certSecretsRepoFixture { certificates := certificates.Clone() secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -300,7 +300,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(spec certSecretsRep } } -type certSecretsRepoFixtureSpec struct { +type certSecretsRepoFixtureConfig struct { // HTTP verb of the k8s API should for which all operations will fail in the fake k8s client set. // Use the zero value so all operations work. k8sAPIVerbToError string From 35792e4d2b2fda25f3188be0d75944a6b8f15fca Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 14 Feb 2022 10:21:13 +0100 Subject: [PATCH 47/49] remove references to put --- .../service_certificates_repository_test.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 9c22ab812b160..2da335856b57e 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -128,27 +128,27 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetDifferentCAsFailure() { } } -func (s *serviceCertificatesRepoSecretsImplSuite) TestPut() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestPatch() { testCases := map[string]struct { expectedErr error fixture *certSecretsRepoFixture }{ - "successful put": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, - "failed put due to k8s API error": { + "successful patch": {expectedErr: nil, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, + "failed patch due to k8s API error": { expectedErr: errForced, fixture: s.newFixture(certSecretsRepoFixtureConfig{k8sAPIVerbToError: "patch"}), }, - "cancelled put": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, + "cancelled patch": {expectedErr: context.Canceled, fixture: s.newFixture(certSecretsRepoFixtureConfig{})}, } for tcName, tc := range testCases { s.Run(tcName, func() { - putCtx, cancelPutCtx := context.WithCancel(context.Background()) - defer cancelPutCtx() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() if tc.expectedErr == context.Canceled { - cancelPutCtx() + cancel() } - err := tc.fixture.repo.ensureServiceCertificates(putCtx, tc.fixture.certificates) + err := tc.fixture.repo.ensureServiceCertificates(ctx, tc.fixture.certificates) s.ErrorIs(err, tc.expectedErr) }) @@ -199,7 +199,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSu } } -func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailure() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestEnsureCertsUnknownServiceTypeFailure() { fixture := s.newFixture(certSecretsRepoFixtureConfig{}) s.getFirstServiceCertificate(fixture.certificates).ServiceType = anotherServiceType @@ -208,7 +208,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestPutUnknownServiceTypeFailu s.Error(err) } -func (s *serviceCertificatesRepoSecretsImplSuite) TestPutMissingServiceTypeSuccess() { +func (s *serviceCertificatesRepoSecretsImplSuite) TestEnsureCertsMissingServiceTypeSuccess() { fixture := s.newFixture(certSecretsRepoFixtureConfig{}) fixture.certificates.ServiceCerts = make([]*storage.TypedServiceCertificate, 0) From 70b4f022cafa8b5214e0d0a7ad830d79cbb1fb1a Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 14 Feb 2022 10:35:11 +0100 Subject: [PATCH 48/49] complete tests --- .../service_certificates_repository_test.go | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 2da335856b57e..23f53f624a21a 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -13,6 +13,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" fakecorev1 "k8s.io/client-go/kubernetes/typed/core/v1/fake" @@ -163,6 +164,14 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { s.ErrorIs(err, ErrMissingSecretData) } +func (s *serviceCertificatesRepoSecretsImplSuite) TestGetUnexpectedSecretsOwnerFailure() { + fixture := s.newFixture(certSecretsRepoFixtureConfig{ownerRefUID: "wrong owner"}) + + _, err := fixture.repo.getServiceCertificates(context.Background()) + + s.ErrorIs(err, ErrUnexpectedSecretsOwner) +} + func (s *serviceCertificatesRepoSecretsImplSuite) TestGetSecretDataMissingKeysSuccess() { testCases := map[string]struct { missingSecretDataKey string @@ -267,29 +276,33 @@ type certSecretsRepoFixture struct { // - The certificates used to initialize the data of the aforementioned secret. // - A repository that uses that secrets client, sensorDeployment as owner, and with a single serviceCertSecretSpec // for the aforementioned secret in its secrets. -func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(spec certSecretsRepoFixtureConfig) *certSecretsRepoFixture { +func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(config certSecretsRepoFixtureConfig) *certSecretsRepoFixture { certificates := certificates.Clone() + ownerRef := sensorOwnerReference() + if config.ownerRefUID != "" { + ownerRef[0].UID = types.UID(config.ownerRefUID) + } secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s-secret", serviceType), Namespace: namespace, - OwnerReferences: sensorOwnerReference(), + OwnerReferences: ownerRef, }, } - if !spec.emptySecretData { + if !config.emptySecretData { secret.Data = map[string][]byte{ mtls.CACertFileName: certificates.GetCaPem(), mtls.ServiceCertFileName: serviceCertificate.GetCert().GetCertPem(), mtls.ServiceKeyFileName: serviceCertificate.GetCert().GetKeyPem(), } } - for _, secretDataKey := range spec.missingSecretDataKeys { + for _, secretDataKey := range config.missingSecretDataKeys { delete(secret.Data, secretDataKey) } secrets := map[storage.ServiceType]*v1.Secret{serviceType: secret} clientSet := fake.NewSimpleClientset(sensorDeployment, secret) secretsClient := clientSet.CoreV1().Secrets(namespace) - clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(spec.k8sAPIVerbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { + clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor(config.k8sAPIVerbToError, "secrets", func(action k8sTesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Secret{}, errForced }) repo := newTestRepo(secrets, secretsClient) @@ -308,6 +321,8 @@ type certSecretsRepoFixtureConfig struct { emptySecretData bool // These keys will be removed from the data keys of the secret used to initialize the fake k8s client set. missingSecretDataKeys []string + // FIXME + ownerRefUID string } func sensorOwnerReference() []metav1.OwnerReference { From 0bc9854aaa3f60db4f24421feb54d29e9cf1331e Mon Sep 17 00:00:00 2001 From: Juan Rodriguez Hortala Date: Mon, 14 Feb 2022 10:39:40 +0100 Subject: [PATCH 49/49] fix comment --- .../service_certificates_repository_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sensor/kubernetes/localscanner/service_certificates_repository_test.go b/sensor/kubernetes/localscanner/service_certificates_repository_test.go index 23f53f624a21a..314bf00797639 100644 --- a/sensor/kubernetes/localscanner/service_certificates_repository_test.go +++ b/sensor/kubernetes/localscanner/service_certificates_repository_test.go @@ -165,7 +165,7 @@ func (s *serviceCertificatesRepoSecretsImplSuite) TestGetNoSecretDataFailure() { } func (s *serviceCertificatesRepoSecretsImplSuite) TestGetUnexpectedSecretsOwnerFailure() { - fixture := s.newFixture(certSecretsRepoFixtureConfig{ownerRefUID: "wrong owner"}) + fixture := s.newFixture(certSecretsRepoFixtureConfig{secretOwnerRefUID: "wrong owner"}) _, err := fixture.repo.getServiceCertificates(context.Background()) @@ -279,8 +279,8 @@ type certSecretsRepoFixture struct { func (s *serviceCertificatesRepoSecretsImplSuite) newFixture(config certSecretsRepoFixtureConfig) *certSecretsRepoFixture { certificates := certificates.Clone() ownerRef := sensorOwnerReference() - if config.ownerRefUID != "" { - ownerRef[0].UID = types.UID(config.ownerRefUID) + if config.secretOwnerRefUID != "" { + ownerRef[0].UID = types.UID(config.secretOwnerRefUID) } secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -321,8 +321,9 @@ type certSecretsRepoFixtureConfig struct { emptySecretData bool // These keys will be removed from the data keys of the secret used to initialize the fake k8s client set. missingSecretDataKeys []string - // FIXME - ownerRefUID string + // If set to a non-zero value, then the UID of the owner of the secret used to initialize the fake k8s client + // set will take this value. + secretOwnerRefUID string } func sensorOwnerReference() []metav1.OwnerReference {