Skip to content

Commit a59bd8d

Browse files
flavianmissiflavianmissi
committed
pkg/storage/azure: support private storage accounts
* abstract azure private endpoint creation * allow private endpoint configuration via operator config * add e2e coverage for private storage account life cycle
1 parent 06dfe47 commit a59bd8d

File tree

6 files changed

+1222
-33
lines changed

6 files changed

+1222
-33
lines changed

‎pkg/storage/azure/azure.go

Lines changed: 175 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
regopclient "github.com/openshift/cluster-image-registry-operator/pkg/client"
3636
"github.com/openshift/cluster-image-registry-operator/pkg/defaults"
3737
"github.com/openshift/cluster-image-registry-operator/pkg/envvar"
38+
"github.com/openshift/cluster-image-registry-operator/pkg/storage/azure/azureclient"
3839
"github.com/openshift/cluster-image-registry-operator/pkg/storage/util"
3940
)
4041

@@ -173,6 +174,10 @@ func (d *driver) createStorageAccount(storageAccountsClient storage.AccountsClie
173174
klog.Infof("attempt to create azure storage account %s (resourceGroup=%q, location=%q)...", accountName, resourceGroupName, location)
174175

175176
kind := storage.StorageV2
177+
// NOTE: looks like this legacy version of the storage library does not support
178+
// disabling public network access at all... either we update the storage
179+
// account after creation using the new sdk, or we use the new sdk to create it
180+
// outside of azure stack hub, and the old sdk for azure stack hub.
176181
params := &storage.AccountPropertiesCreateParameters{
177182
EnableHTTPSTrafficOnly: to.BoolPtr(true),
178183
AllowBlobPublicAccess: to.BoolPtr(false),
@@ -540,10 +545,89 @@ func (d *driver) StorageChanged(cr *imageregistryv1.Config) bool {
540545
return !reflect.DeepEqual(cr.Status.Storage.Azure, cr.Spec.Storage.Azure)
541546
}
542547

548+
func (d *driver) assurePrivateAccount(cfg *Azure, infra *configv1.Infrastructure, tagset map[string]*string, accountName string) (string, error) {
549+
if d.Config.NetworkAccess == nil || d.Config.NetworkAccess.Type == imageregistryv1.AzureNetworkAccessTypeExternal {
550+
// user did not request private storage account setup - skip.
551+
return "", nil
552+
}
553+
if d.Config.NetworkAccess.Internal.VNetName == "" || d.Config.NetworkAccess.Internal.SubnetName == "" {
554+
return "", fmt.Errorf("both vnetName and subnetName are required to setup a private storage account")
555+
}
556+
557+
var privateEndpointName string
558+
if d.Config.NetworkAccess.Internal != nil && d.Config.NetworkAccess.Internal.PrivateEndpointName != "" {
559+
privateEndpointName = d.Config.NetworkAccess.Internal.PrivateEndpointName
560+
} else {
561+
privateEndpointName = generateAccountName(infra.Status.InfrastructureName)
562+
}
563+
564+
environment, err := getEnvironmentByName(d.Config.CloudName)
565+
if err != nil {
566+
return "", err
567+
}
568+
569+
azclient, err := azureclient.New(&azureclient.Options{
570+
Environment: environment,
571+
TenantID: cfg.TenantID,
572+
ClientID: cfg.ClientID,
573+
ClientSecret: cfg.ClientSecret,
574+
FederatedTokenFile: cfg.FederatedTokenFile,
575+
SubscriptionID: cfg.SubscriptionID,
576+
ResourceGroupName: cfg.ResourceGroup,
577+
TagSet: tagset,
578+
})
579+
if err != nil {
580+
return "", err
581+
}
582+
583+
// the last step in this function is to disable public network for the
584+
// storage account - if we already did that, then none of the steps
585+
// below need to be executed.
586+
if azclient.IsStorageAccountPrivate(d.Context, accountName) {
587+
return privateEndpointName, nil
588+
}
589+
590+
klog.V(3).Infof("configuring private endpoint %q for storage account...", privateEndpointName)
591+
pe, err := azclient.CreatePrivateEndpoint(
592+
d.Context,
593+
&azureclient.PrivateEndpointCreateOptions{
594+
Location: cfg.Region,
595+
VNetName: d.Config.NetworkAccess.Internal.VNetName,
596+
SubnetName: d.Config.NetworkAccess.Internal.SubnetName,
597+
PrivateEndpointName: privateEndpointName,
598+
StorageAccountName: accountName,
599+
},
600+
)
601+
if err != nil {
602+
return "", err
603+
}
604+
klog.V(3).Info("private endpoint configured")
605+
606+
klog.V(3).Info("configuring private DNS...")
607+
if err := azclient.ConfigurePrivateDNS(
608+
d.Context, pe, d.Config.NetworkAccess.Internal.VNetName, accountName,
609+
); err != nil {
610+
return privateEndpointName, err
611+
}
612+
klog.V(3).Info("private DNS configured")
613+
614+
klog.V(3).Infof("disabling public network access for storage account %q...", accountName)
615+
if err := azclient.UpdateStorageAccountNetworkAccess(d.Context, accountName, false); err != nil {
616+
return privateEndpointName, err
617+
}
618+
619+
klog.Infof(
620+
"storage account %q is now served by private endpoint %q. public network access is disabled",
621+
accountName, privateEndpointName,
622+
)
623+
624+
return privateEndpointName, nil
625+
}
626+
543627
// assureStorageAccount makes sure there is a storage account in place and apply any provided tags.
544628
// If no storage account name is provided it attempts to generate one. Returns the account name
545629
// (either the one provided or the one generated), if the account was created or was already there and an error.
546-
func (d *driver) assureStorageAccount(cfg *Azure, infra *configv1.Infrastructure) (string, bool, error) {
630+
func (d *driver) assureStorageAccount(cfg *Azure, infra *configv1.Infrastructure, tagset map[string]*string) (string, bool, error) {
547631
environment, err := getEnvironmentByName(d.Config.CloudName)
548632
if err != nil {
549633
return "", false, err
@@ -571,26 +655,6 @@ func (d *driver) assureStorageAccount(cfg *Azure, infra *configv1.Infrastructure
571655
return "", false, fmt.Errorf("create storage account failed, name not available")
572656
}
573657

574-
// Tag the storage account with the openshiftClusterID
575-
// along with any user defined tags from the cluster configuration
576-
klog.V(2).Info("setting azure storage account tags")
577-
578-
tagset := map[string]*string{
579-
fmt.Sprintf("kubernetes.io_cluster.%s", infra.Status.InfrastructureName): to.StringPtr("owned"),
580-
}
581-
582-
// at this stage we are not keeping user tags in sync. as per enhancement proposal
583-
// we only set user provided tags when we created the bucket.
584-
hasAzureStatus := infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.Azure != nil && infra.Status.PlatformStatus.Azure.ResourceTags != nil
585-
if hasAzureStatus {
586-
klog.V(5).Infof("user has provided %d tags", len(infra.Status.PlatformStatus.Azure.ResourceTags))
587-
for _, tag := range infra.Status.PlatformStatus.Azure.ResourceTags {
588-
klog.V(5).Infof("user has provided storage account tag: %s: %s", tag.Key, tag.Value)
589-
tagset[tag.Key] = to.StringPtr(tag.Value)
590-
}
591-
}
592-
klog.V(5).Infof("tagging storage account with tags: %+v", tagset)
593-
594658
// regardless if the storage account name was provided by the user or we generated it,
595659
// if it is available, we do attempt to create it.
596660
var storageAccountCreated bool
@@ -743,7 +807,26 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
743807
}
744808
}
745809

746-
storageAccountName, storageAccountCreated, err := d.assureStorageAccount(cfg, infra)
810+
// Tag the storage account with the openshiftClusterID
811+
// along with any user defined tags from the cluster configuration
812+
klog.V(2).Info("setting azure storage account tags")
813+
tagset := map[string]*string{
814+
fmt.Sprintf("kubernetes.io_cluster.%s", infra.Status.InfrastructureName): to.StringPtr("owned"),
815+
}
816+
817+
// at this stage we are not keeping user tags in sync. as per enhancement proposal
818+
// we only set user provided tags when we created the bucket.
819+
hasAzureStatus := infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.Azure != nil && infra.Status.PlatformStatus.Azure.ResourceTags != nil
820+
if hasAzureStatus {
821+
klog.V(5).Infof("user has provided %d tags", len(infra.Status.PlatformStatus.Azure.ResourceTags))
822+
for _, tag := range infra.Status.PlatformStatus.Azure.ResourceTags {
823+
klog.V(5).Infof("user has provided storage account tag: %s: %s", tag.Key, tag.Value)
824+
tagset[tag.Key] = to.StringPtr(tag.Value)
825+
}
826+
}
827+
klog.V(5).Infof("tagging storage account with tags: %+v", tagset)
828+
829+
storageAccountName, storageAccountCreated, err := d.assureStorageAccount(cfg, infra, tagset)
747830
if err != nil {
748831
util.UpdateCondition(
749832
cr,
@@ -769,6 +852,26 @@ func (d *driver) CreateStorage(cr *imageregistryv1.Config) error {
769852
}
770853
d.Config.Container = containerName
771854

855+
privateEndpointName, err := d.assurePrivateAccount(cfg, infra, tagset, storageAccountName)
856+
if err != nil {
857+
util.UpdateCondition(
858+
cr,
859+
defaults.StorageExists,
860+
operatorapiv1.ConditionUnknown,
861+
storageExistsReasonAzureError,
862+
fmt.Sprintf("Unable to process private endpoint: %s", err),
863+
)
864+
return err
865+
}
866+
if privateEndpointName != "" {
867+
// in most cases, a call to d.assurePrivateAccount will not result
868+
// in the creation of a private endpoint. this will only happen
869+
// when users explicitly request so by setting
870+
// VNetName and SubnetName
871+
// in the registry config. only then we set the private endpoint name.
872+
d.Config.NetworkAccess.Internal.PrivateEndpointName = privateEndpointName
873+
}
874+
772875
// We only set the storage management if it is not already set.
773876
if cr.Spec.Storage.ManagementState == "" {
774877
if storageAccountCreated || containerCreated {
@@ -821,6 +924,56 @@ func (d *driver) RemoveStorage(cr *imageregistryv1.Config) (retry bool, err erro
821924
return false, err
822925
}
823926

927+
if d.Config.NetworkAccess != nil && d.Config.NetworkAccess.Internal != nil && d.Config.NetworkAccess.Internal.PrivateEndpointName != "" {
928+
azclient, err := azureclient.New(&azureclient.Options{
929+
Environment: environment,
930+
TenantID: cfg.TenantID,
931+
ClientID: cfg.ClientID,
932+
ClientSecret: cfg.ClientSecret,
933+
FederatedTokenFile: cfg.FederatedTokenFile,
934+
SubscriptionID: cfg.SubscriptionID,
935+
ResourceGroupName: cfg.ResourceGroup,
936+
})
937+
if err != nil {
938+
util.UpdateCondition(
939+
cr,
940+
defaults.StorageExists,
941+
operatorapiv1.ConditionUnknown,
942+
storageExistsReasonAzureError,
943+
fmt.Sprintf("Unable to get azure client: %s", err),
944+
)
945+
return false, err
946+
}
947+
if err := azclient.DestroyPrivateDNS(
948+
d.Context,
949+
d.Config.NetworkAccess.Internal.PrivateEndpointName,
950+
d.Config.NetworkAccess.Internal.VNetName,
951+
d.Config.AccountName,
952+
); err != nil {
953+
util.UpdateCondition(
954+
cr,
955+
defaults.StorageExists,
956+
operatorapiv1.ConditionUnknown,
957+
storageExistsReasonAzureError,
958+
fmt.Sprintf("Unable to destroy private dns: %q", err),
959+
)
960+
return false, err
961+
}
962+
if err := azclient.DeletePrivateEndpoint(
963+
d.Context, d.Config.NetworkAccess.Internal.PrivateEndpointName,
964+
); err != nil {
965+
util.UpdateCondition(
966+
cr,
967+
defaults.StorageExists,
968+
operatorapiv1.ConditionUnknown,
969+
storageExistsReasonAzureError,
970+
fmt.Sprintf("Unable to delete private endpoint: %q", err),
971+
)
972+
return false, err
973+
}
974+
d.Config.NetworkAccess = nil
975+
}
976+
824977
if d.Config.Container != "" {
825978
key, err := d.getAccountPrimaryKey(storageAccountsClient, cfg.ResourceGroup, d.Config.AccountName)
826979
if _, ok := err.(*errDoesNotExist); ok {

‎pkg/storage/azure/azure_test.go

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/Azure/azure-pipeline-go/pipeline"
1717
"github.com/Azure/go-autorest/autorest"
1818
"github.com/Azure/go-autorest/autorest/mocks"
19+
"github.com/Azure/go-autorest/autorest/to"
1920
"github.com/google/go-cmp/cmp"
2021

2122
configlisters "github.com/openshift/client-go/config/listers/config/v1"
@@ -484,16 +485,16 @@ func TestUserProvidedTags(t *testing.T) {
484485
for _, tt := range []struct {
485486
name string
486487
userTags []configv1.AzureResourceTag
487-
expectedTags map[string]string
488+
expectedTags map[string]*string
488489
infraName string
489490
responseBody string
490491
}{
491492
{
492493
name: "no-user-tags",
493494
infraName: "some-infra",
494495
// only default tags
495-
expectedTags: map[string]string{
496-
"kubernetes.io_cluster.some-infra": "owned",
496+
expectedTags: map[string]*string{
497+
"kubernetes.io_cluster.some-infra": to.StringPtr("owned"),
497498
},
498499
responseBody: `{"nameAvailable":true}`,
499500
},
@@ -511,10 +512,10 @@ func TestUserProvidedTags(t *testing.T) {
511512
},
512513
},
513514
// default tags and user tags
514-
expectedTags: map[string]string{
515-
"kubernetes.io_cluster.test-infra": "owned",
516-
"tag1": "value1",
517-
"tag2": "value2",
515+
expectedTags: map[string]*string{
516+
"kubernetes.io_cluster.test-infra": to.StringPtr("owned"),
517+
"tag1": to.StringPtr("value1"),
518+
"tag2": to.StringPtr("value2"),
518519
},
519520
responseBody: `{"nameAvailable":true}`,
520521
},
@@ -547,6 +548,7 @@ func TestUserProvidedTags(t *testing.T) {
547548
},
548549
},
549550
},
551+
tt.expectedTags,
550552
)
551553
if err != nil {
552554
t.Errorf("unexpected error %q", err)
@@ -572,9 +574,9 @@ func TestUserProvidedTags(t *testing.T) {
572574
t.Fatal("unable to type assert tags field")
573575
}
574576
// convert into correct type
575-
receivedTags := make(map[string]string)
577+
receivedTags := make(map[string]*string)
576578
for k, v := range tags {
577-
receivedTags[k] = fmt.Sprintf("%+v", v)
579+
receivedTags[k] = to.StringPtr(fmt.Sprintf("%+v", v))
578580
}
579581

580582
// compare the tags
@@ -683,6 +685,7 @@ func Test_assureStorageAccount(t *testing.T) {
683685
ResourceGroup: "resource_group",
684686
},
685687
&configv1.Infrastructure{},
688+
map[string]*string{},
686689
)
687690

688691
if err != nil {

0 commit comments

Comments
 (0)