diff --git a/CHANGELOG.md b/CHANGELOG.md index 2afee87..f14530e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Secure Azure Storage Account by making them private. - Update Kyverno PolicyException to v2beta1. ## [0.9.0] - 2024-10-03 diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index e9308ba..a1c84cc 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -110,7 +110,12 @@ func (r BucketReconciler) reconcileNormal(ctx context.Context, objectStorageServ return ctrl.Result{}, errors.WithStack(err) } } else { - logger.Info("Bucket exists and you already own it.") + logger.Info("Bucket exists and you already own it, let's update it") + err = objectStorageService.UpdateBucket(ctx, bucket) + if err != nil { + logger.Error(err, "Bucket could not be updated") + return ctrl.Result{}, errors.WithStack(err) + } } logger.Info("Configuring bucket settings") diff --git a/internal/pkg/service/objectstorage/cloud/aws/s3.go b/internal/pkg/service/objectstorage/cloud/aws/s3.go index 9aa3648..6ab7618 100644 --- a/internal/pkg/service/objectstorage/cloud/aws/s3.go +++ b/internal/pkg/service/objectstorage/cloud/aws/s3.go @@ -64,6 +64,11 @@ func (s S3ObjectStorageAdapter) CreateBucket(ctx context.Context, bucket *v1alph return err } +// UpdateBucket does nothing as we cannot update an s3 bucket +func (s S3ObjectStorageAdapter) UpdateBucket(ctx context.Context, bucket *v1alpha1.Bucket) error { + return nil +} + func (s S3ObjectStorageAdapter) DeleteBucket(ctx context.Context, bucket *v1alpha1.Bucket) error { // First we need to empty the bucket paginator := s3.NewListObjectsV2Paginator(s.s3Client, &s3.ListObjectsV2Input{ diff --git a/internal/pkg/service/objectstorage/cloud/azure/storage.go b/internal/pkg/service/objectstorage/cloud/azure/storage.go index 7da0d67..d3c8578 100644 --- a/internal/pkg/service/objectstorage/cloud/azure/storage.go +++ b/internal/pkg/service/objectstorage/cloud/azure/storage.go @@ -33,7 +33,6 @@ type AzureObjectStorageAdapter struct { logger logr.Logger cluster AzureCluster client client.Client - listStorageAccountName []string } // NewAzureStorageService creates a new instance of AzureObjectStorageAdapter. @@ -42,7 +41,6 @@ type AzureObjectStorageAdapter struct { // The logger is used for logging purposes. // The cluster represents the Azure cluster. // The client is the Kubernetes client used for interacting with the Kubernetes API. -// The listStorageAccountName is a list of storage account names. func NewAzureStorageService(storageAccountClient *armstorage.AccountsClient, blobContainerClient *armstorage.BlobContainersClient, managementPoliciesClient *armstorage.ManagementPoliciesClient, logger logr.Logger, cluster AzureCluster, client client.Client) AzureObjectStorageAdapter { return AzureObjectStorageAdapter{ storageAccountClient: storageAccountClient, @@ -51,7 +49,6 @@ func NewAzureStorageService(storageAccountClient *armstorage.AccountsClient, blo logger: logger, cluster: cluster, client: client, - listStorageAccountName: []string{}, } } @@ -121,41 +118,46 @@ func (s AzureObjectStorageAdapter) CreateBucket(ctx context.Context, bucket *v1a if err != nil { return err } - // If Storage Account does not exists, we need to create it first - if !existsStorageAccount { - // Create Storage Account - pollerResp, err := s.storageAccountClient.BeginCreate( - ctx, - s.cluster.GetResourceGroup(), - storageAccountName, - armstorage.AccountCreateParameters{ - Kind: to.Ptr(armstorage.KindBlobStorage), - SKU: &armstorage.SKU{ - Name: to.Ptr(armstorage.SKUNameStandardLRS), - }, - Location: to.Ptr(s.cluster.GetRegion()), - Properties: &armstorage.AccountPropertiesCreateParameters{ - AccessTier: to.Ptr(armstorage.AccessTierHot), - Encryption: &armstorage.Encryption{ - Services: &armstorage.EncryptionServices{ - Blob: &armstorage.EncryptionService{ - KeyType: to.Ptr(armstorage.KeyTypeAccount), - Enabled: to.Ptr(true), - }, + + // Create or Update Storage Account + pollerResp, err := s.storageAccountClient.BeginCreate( + ctx, + s.cluster.GetResourceGroup(), + storageAccountName, + armstorage.AccountCreateParameters{ + Kind: to.Ptr(armstorage.KindBlobStorage), + SKU: &armstorage.SKU{ + Name: to.Ptr(armstorage.SKUNameStandardLRS), + }, + Location: to.Ptr(s.cluster.GetRegion()), + Properties: &armstorage.AccountPropertiesCreateParameters{ + AllowSharedKeyAccess: to.Ptr(true), + AccessTier: to.Ptr(armstorage.AccessTierHot), + Encryption: &armstorage.Encryption{ + Services: &armstorage.EncryptionServices{ + Blob: &armstorage.EncryptionService{ + KeyType: to.Ptr(armstorage.KeyTypeAccount), + Enabled: to.Ptr(true), }, - KeySource: to.Ptr(armstorage.KeySourceMicrosoftStorage), }, - EnableHTTPSTrafficOnly: to.Ptr(true), + KeySource: to.Ptr(armstorage.KeySourceMicrosoftStorage), }, - }, nil) - if err != nil { - return err - } - _, err = pollerResp.PollUntilDone(ctx, nil) - if err != nil { - return err - } + EnableHTTPSTrafficOnly: to.Ptr(true), + PublicNetworkAccess: to.Ptr(armstorage.PublicNetworkAccessDisabled), + }, + }, nil) + if err != nil { + return err + } + _, err = pollerResp.PollUntilDone(ctx, nil) + if err != nil { + return err + } + + if !existsStorageAccount { s.logger.Info(fmt.Sprintf("Storage Account %s created", storageAccountName)) + } else { + s.logger.Info(fmt.Sprintf("Storage Account %s updated", storageAccountName)) } // Create Storage Container @@ -187,43 +189,49 @@ func (s AzureObjectStorageAdapter) CreateBucket(ctx context.Context, bucket *v1a if err != nil { return fmt.Errorf("unable to retrieve access keys from storage account %s", storageAccountName) } - // Then, we retrieve the Access Key for 'key1' - foundKey1 := false - for _, k := range listKeys.Keys { - if *k.KeyName == "key1" { - foundKey1 = true - // Finally, we create the Secret into the bucket namespace - secret := &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: bucket.Spec.Name, - Namespace: bucket.Namespace, - Labels: map[string]string{ - "giantswarm.io/managed-by": "object-storage-operator", - }, - Finalizers: []string{ - v1alpha1.AzureSecretFinalizer, - }, - }, - Data: map[string][]byte{ + + secret := v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: bucket.Spec.Name, + Namespace: bucket.Namespace, + Labels: map[string]string{ + "giantswarm.io/managed-by": "object-storage-operator", + }, + Finalizers: []string{ + v1alpha1.AzureSecretFinalizer, + }, + }, + } + + _, err = controllerutil.CreateOrUpdate(ctx, s.client, &secret, func() error { + // Then, we retrieve the Access Key for 'key1' + for _, k := range listKeys.Keys { + if *k.KeyName == "key1" { + // Finally, we create the Secret into the bucket namespace + secret.Data = map[string][]byte{ "accountName": []byte(storageAccountName), "accountKey": []byte(*k.Value), - }, - } - err := s.client.Create(ctx, secret) - if err != nil { - return err + } + return nil } - s.logger.Info(fmt.Sprintf("created secret %s", bucket.Spec.Name)) - break } - } - if !foundKey1 { + return fmt.Errorf("unable to retrieve access keys 'key1' from storage account %s", storageAccountName) + }) + + if err != nil { + return err } + s.logger.Info(fmt.Sprintf("created secret %s", bucket.Spec.Name)) return nil } +// UpdateBucket creates or updates the Storage Account if it not exists AND the Storage Container +func (s AzureObjectStorageAdapter) UpdateBucket(ctx context.Context, bucket *v1alpha1.Bucket) error { + return s.CreateBucket(ctx, bucket) +} + // DeleteBucket deletes the Storage Account (Storage Container will be deleted by cascade) // Here, we decided to have a Storage Account dedicated to a Storage Container (relation 1 - 1) // We want to prevent the Storage Account from being used by anyone @@ -394,17 +402,9 @@ func (s AzureObjectStorageAdapter) setTags(ctx context.Context, bucket *v1alpha1 } // getStorageAccountName returns the storage account name for the given bucket name. -// It sanitizes the bucket name and checks if it already exists in the list of storage account names. -// If it exists, it returns the sanitized name. Otherwise, it adds the sanitized name to the list and returns it. +// It sanitizes the bucket name and returns it. func (s *AzureObjectStorageAdapter) getStorageAccountName(bucketName string) string { - sanitizeName := sanitizeAlphanumeric24(bucketName) - for _, name := range s.listStorageAccountName { - if sanitizeName == name { - return sanitizeName - } - } - s.listStorageAccountName = append(s.listStorageAccountName, sanitizeName) - return sanitizeName + return sanitizeAlphanumeric24(bucketName) } // sanitizeAlphanumeric24 sanitizes the given name by removing any non-alphanumeric characters and truncating it to a maximum length of 24 characters. diff --git a/internal/pkg/service/objectstorage/cloud/azure/storage_test.go b/internal/pkg/service/objectstorage/cloud/azure/storage_test.go index d40d38c..5b9b954 100644 --- a/internal/pkg/service/objectstorage/cloud/azure/storage_test.go +++ b/internal/pkg/service/objectstorage/cloud/azure/storage_test.go @@ -42,55 +42,3 @@ func Test_sanitizeAlphanumeric24(t *testing.T) { }) } } - -func Test_getStorageAccountName(t *testing.T) { - testCases := []struct { - name string - bucketName string - listStorageNames []string - expectedResult string - expectedListNames []string - }{ - { - name: "case 0: bucket name exists in list", - bucketName: "giantswarm-glippy-loki", - listStorageNames: []string{"giantswarmglippyloki", "giantswarmverylonginstal"}, - expectedResult: "giantswarmglippyloki", - expectedListNames: []string{"giantswarmglippyloki", "giantswarmverylonginstal"}, - }, - { - name: "case 1: bucket name does not exist in list", - bucketName: "giantswarm-verylonginstallationname-loki", - listStorageNames: []string{"giantswarmglippyloki"}, - expectedResult: "giantswarmverylonginstal", - expectedListNames: []string{"giantswarmglippyloki", "giantswarmverylonginstal"}, - }, - { - name: "case 2: empty list", - bucketName: "giantswarm-1111-loki", - listStorageNames: []string{}, - expectedResult: "giantswarm1111loki", - expectedListNames: []string{"giantswarm1111loki"}, - }, - } - - for i, tc := range testCases { - t.Run(strconv.Itoa(i), func(t *testing.T) { - t.Log(tc.name) - - adapter := &AzureObjectStorageAdapter{ - listStorageAccountName: tc.listStorageNames, - } - - result := adapter.getStorageAccountName(tc.bucketName) - - if result != tc.expectedResult { - t.Fatalf("Expected result: %s, but got: %s", tc.expectedResult, result) - } - - if !cmp.Equal(adapter.listStorageAccountName, tc.expectedListNames) { - t.Fatalf("\n\n%s\n", cmp.Diff(tc.expectedListNames, adapter.listStorageAccountName)) - } - }) - } -} diff --git a/internal/pkg/service/objectstorage/objectstoragefakes/fake_object_storage_service.go b/internal/pkg/service/objectstorage/objectstoragefakes/fake_object_storage_service.go index a7436db..1e7af39 100644 --- a/internal/pkg/service/objectstorage/objectstoragefakes/fake_object_storage_service.go +++ b/internal/pkg/service/objectstorage/objectstoragefakes/fake_object_storage_service.go @@ -60,6 +60,18 @@ type FakeObjectStorageService struct { result1 bool result2 error } + UpdateBucketStub func(context.Context, *v1alpha1.Bucket) error + updateBucketMutex sync.RWMutex + updateBucketArgsForCall []struct { + arg1 context.Context + arg2 *v1alpha1.Bucket + } + updateBucketReturns struct { + result1 error + } + updateBucketReturnsOnCall map[int]struct { + result1 error + } invocations map[string][][]interface{} invocationsMutex sync.RWMutex } @@ -315,6 +327,68 @@ func (fake *FakeObjectStorageService) ExistsBucketReturnsOnCall(i int, result1 b }{result1, result2} } +func (fake *FakeObjectStorageService) UpdateBucket(arg1 context.Context, arg2 *v1alpha1.Bucket) error { + fake.updateBucketMutex.Lock() + ret, specificReturn := fake.updateBucketReturnsOnCall[len(fake.updateBucketArgsForCall)] + fake.updateBucketArgsForCall = append(fake.updateBucketArgsForCall, struct { + arg1 context.Context + arg2 *v1alpha1.Bucket + }{arg1, arg2}) + stub := fake.UpdateBucketStub + fakeReturns := fake.updateBucketReturns + fake.recordInvocation("UpdateBucket", []interface{}{arg1, arg2}) + fake.updateBucketMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeObjectStorageService) UpdateBucketCallCount() int { + fake.updateBucketMutex.RLock() + defer fake.updateBucketMutex.RUnlock() + return len(fake.updateBucketArgsForCall) +} + +func (fake *FakeObjectStorageService) UpdateBucketCalls(stub func(context.Context, *v1alpha1.Bucket) error) { + fake.updateBucketMutex.Lock() + defer fake.updateBucketMutex.Unlock() + fake.UpdateBucketStub = stub +} + +func (fake *FakeObjectStorageService) UpdateBucketArgsForCall(i int) (context.Context, *v1alpha1.Bucket) { + fake.updateBucketMutex.RLock() + defer fake.updateBucketMutex.RUnlock() + argsForCall := fake.updateBucketArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeObjectStorageService) UpdateBucketReturns(result1 error) { + fake.updateBucketMutex.Lock() + defer fake.updateBucketMutex.Unlock() + fake.UpdateBucketStub = nil + fake.updateBucketReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeObjectStorageService) UpdateBucketReturnsOnCall(i int, result1 error) { + fake.updateBucketMutex.Lock() + defer fake.updateBucketMutex.Unlock() + fake.UpdateBucketStub = nil + if fake.updateBucketReturnsOnCall == nil { + fake.updateBucketReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.updateBucketReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeObjectStorageService) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() @@ -326,6 +400,8 @@ func (fake *FakeObjectStorageService) Invocations() map[string][][]interface{} { defer fake.deleteBucketMutex.RUnlock() fake.existsBucketMutex.RLock() defer fake.existsBucketMutex.RUnlock() + fake.updateBucketMutex.RLock() + defer fake.updateBucketMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} for key, value := range fake.invocations { copiedInvocations[key] = value diff --git a/internal/pkg/service/objectstorage/service.go b/internal/pkg/service/objectstorage/service.go index 168b387..47f8de1 100644 --- a/internal/pkg/service/objectstorage/service.go +++ b/internal/pkg/service/objectstorage/service.go @@ -11,6 +11,7 @@ type ObjectStorageService interface { // Configure all bucket related configurations (Tags, ...) ConfigureBucket(ctx context.Context, bucket *v1alpha1.Bucket) error CreateBucket(ctx context.Context, bucket *v1alpha1.Bucket) error + UpdateBucket(ctx context.Context, bucket *v1alpha1.Bucket) error DeleteBucket(ctx context.Context, bucket *v1alpha1.Bucket) error // Exists checks whether a bucket exists in the current account. ExistsBucket(ctx context.Context, bucket *v1alpha1.Bucket) (bool, error)