Skip to content

Commit

Permalink
[ARO-15116] DES missing Keyvault Access Policy on E2E retry (#4093)
Browse files Browse the repository at this point in the history 
* reuse kv for des if already exists
* advice what to do with invalid DES
* Inverse the if logic
* inverse also if logic to manage only errors
* NameAvailable nil is an error
* add debug log in cas of retry
* type, and replace debug by info
* remote code for testing...
  • Loading branch information
@Tof1973
Tof1973 authored Feb 13, 2025
1 parent 1e45ba4 commit 73579fb
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 44 deletions.
2 changes: 1 addition & 1 deletion pkg/frontend/adminactions/azureactions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func NewAzureActions(log *logrus.Entry, env env.Interface, oc *api.OpenShiftClus
resourceSkus: compute.NewResourceSkusClient(env.Environment(), subscriptionDoc.ID, fpAuth),
virtualMachines: compute.NewVirtualMachinesClient(env.Environment(), subscriptionDoc.ID, fpAuth),
virtualNetworks: virtualNetworks,
diskEncryptionSets: compute.NewDiskEncryptionSetsClient(env.Environment(), subscriptionDoc.ID, fpAuth),
diskEncryptionSets: compute.NewDiskEncryptionSetsClientWithAROEnvironment(env.Environment(), subscriptionDoc.ID, fpAuth),
routeTables: routeTables,
storageAccounts: storage.NewAccountsClient(env.Environment(), subscriptionDoc.ID, fpAuth),
networkInterfaces: network.NewInterfacesClient(env.Environment(), subscriptionDoc.ID, fpAuth),
Expand Down
13 changes: 12 additions & 1 deletion pkg/util/azureclient/mgmt/compute/diskencryptionsets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type diskEncryptionSetsClient struct {
var _ DiskEncryptionSetsClient = &diskEncryptionSetsClient{}

// NewDisksClient creates a new DisksClient
func NewDiskEncryptionSetsClient(environment *azureclient.AROEnvironment, subscriptionID string, authorizer autorest.Authorizer) DiskEncryptionSetsClient {
func NewDiskEncryptionSetsClientWithAROEnvironment(environment *azureclient.AROEnvironment, subscriptionID string, authorizer autorest.Authorizer) DiskEncryptionSetsClient {
client := mgmtcompute.NewDiskEncryptionSetsClientWithBaseURI(environment.ResourceManagerEndpoint, subscriptionID)
client.Authorizer = authorizer
client.Sender = azureclient.DecorateSenderWithLogging(client.Sender)
Expand All @@ -33,3 +33,14 @@ func NewDiskEncryptionSetsClient(environment *azureclient.AROEnvironment, subscr
DiskEncryptionSetsClient: client,
}
}

// NewDisksClient creates a new DisksClient
func NewDiskEncryptionSetsClient(subscriptionID string, authorizer autorest.Authorizer) DiskEncryptionSetsClient {
client := mgmtcompute.NewDiskEncryptionSetsClient(subscriptionID)
client.Authorizer = authorizer
client.Sender = azureclient.DecorateSenderWithLogging(client.Sender)

return &diskEncryptionSetsClient{
DiskEncryptionSetsClient: client,
}
}
123 changes: 83 additions & 40 deletions pkg/util/cluster/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armkeyvault"
"github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armnetwork"
"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/authorization"
"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/compute"
"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/features"
"github.com/Azure/ARO-RP/pkg/util/azureerrors"
utilgraph "github.com/Azure/ARO-RP/pkg/util/graph"
Expand All @@ -55,17 +56,18 @@ type Cluster struct {
ci bool
ciParentVnet string

spGraphClient *utilgraph.GraphServiceClient
deployments features.DeploymentsClient
groups features.ResourceGroupsClient
openshiftclusters InternalClient
securitygroups armnetwork.SecurityGroupsClient
subnets armnetwork.SubnetsClient
routetables armnetwork.RouteTablesClient
roleassignments authorization.RoleAssignmentsClient
peerings armnetwork.VirtualNetworkPeeringsClient
ciParentVnetPeerings armnetwork.VirtualNetworkPeeringsClient
vaultsClient armkeyvault.VaultsClient
spGraphClient *utilgraph.GraphServiceClient
deployments features.DeploymentsClient
groups features.ResourceGroupsClient
openshiftclusters InternalClient
securitygroups armnetwork.SecurityGroupsClient
subnets armnetwork.SubnetsClient
routetables armnetwork.RouteTablesClient
roleassignments authorization.RoleAssignmentsClient
peerings armnetwork.VirtualNetworkPeeringsClient
ciParentVnetPeerings armnetwork.VirtualNetworkPeeringsClient
vaultsClient armkeyvault.VaultsClient
diskEncryptionSetsClient compute.DiskEncryptionSetsClient
}

const GenerateSubnetMaxTries = 100
Expand Down Expand Up @@ -118,6 +120,8 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) {
return nil, err
}

diskEncryptionSetsClient := compute.NewDiskEncryptionSetsClient(environment.SubscriptionID(), authorizer)

securityGroupsClient, err := armnetwork.NewSecurityGroupsClient(environment.SubscriptionID(), spTokenCredential, clientOptions)
if err != nil {
return nil, err
Expand All @@ -143,16 +147,17 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) {
env: environment,
ci: ci,

spGraphClient: spGraphClient,
deployments: features.NewDeploymentsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
groups: features.NewResourceGroupsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
openshiftclusters: NewInternalClient(log, environment, authorizer),
securitygroups: securityGroupsClient,
subnets: subnetsClient,
routetables: routeTablesClient,
roleassignments: authorization.NewRoleAssignmentsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
peerings: virtualNetworkPeeringsClient,
vaultsClient: vaultClient,
spGraphClient: spGraphClient,
deployments: features.NewDeploymentsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
groups: features.NewResourceGroupsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
openshiftclusters: NewInternalClient(log, environment, authorizer),
securitygroups: securityGroupsClient,
subnets: subnetsClient,
routetables: routeTablesClient,
roleassignments: authorization.NewRoleAssignmentsClient(environment.Environment(), environment.SubscriptionID(), authorizer),
peerings: virtualNetworkPeeringsClient,
vaultsClient: vaultClient,
diskEncryptionSetsClient: diskEncryptionSetsClient,
}

if ci && env.IsLocalDevelopmentMode() {
Expand Down Expand Up @@ -250,26 +255,65 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
return err
}

diskEncryptionSetName := fmt.Sprintf(
"%s%s",
vnetResourceGroup,
generator.SharedDiskEncryptionSetNameSuffix,
)

var kvName string
if len(vnetResourceGroup) > 10 {
// keyvault names need to have a maximum length of 24,
// so we need to cut off some chars if the resource group name is too long
kvName = vnetResourceGroup[:10] + generator.SharedDiskEncryptionKeyVaultNameSuffix
if !c.ci {
if len(vnetResourceGroup) > 10 {
// keyvault names need to have a maximum length of 24,
// so we need to cut off some chars if the resource group name is too long
kvName = vnetResourceGroup[:10] + generator.SharedDiskEncryptionKeyVaultNameSuffix
} else {
kvName = vnetResourceGroup + generator.SharedDiskEncryptionKeyVaultNameSuffix
}
} else {
kvName = vnetResourceGroup + generator.SharedDiskEncryptionKeyVaultNameSuffix
}

if c.ci {
// name is limited to 24 characters, but must be globally unique, so we generate one and try if it is available
kvName = "kv-" + uuid.DefaultGenerator.Generate()[:21]
// if DES already exists in RG, then reuse KV hosting the key of this DES,
// otherwise, name is limited to 24 characters, but must be globally unique,
// so we generate a name randomly until it is available
diskEncryptionSet, err := c.diskEncryptionSetsClient.Get(ctx, vnetResourceGroup, diskEncryptionSetName)
if err == nil {
if diskEncryptionSet.EncryptionSetProperties == nil ||
diskEncryptionSet.EncryptionSetProperties.ActiveKey == nil ||
diskEncryptionSet.EncryptionSetProperties.ActiveKey.SourceVault == nil ||
diskEncryptionSet.EncryptionSetProperties.ActiveKey.SourceVault.ID == nil {
return fmt.Errorf("no valid Key Vault found in Disk Encryption Set: %v. Delete the Disk Encryption Set and retry", diskEncryptionSet)
}
ID := *diskEncryptionSet.EncryptionSetProperties.ActiveKey.SourceVault.ID
var found bool
_, kvName, found = strings.Cut(ID, "/providers/Microsoft.KeyVault/vaults/")
if !found {
return fmt.Errorf("could not find Key Vault name in ID: %v", ID)
}
} else {
if autorestErr, ok := err.(autorest.DetailedError); !ok ||
autorestErr.Response == nil ||
autorestErr.Response.StatusCode != http.StatusNotFound {
return fmt.Errorf("failed to get Disk Encryption Set: %v", err)
}
for {
kvName = "kv-" + uuid.DefaultGenerator.Generate()[:21]
result, err := c.vaultsClient.CheckNameAvailability(
ctx,
sdkkeyvault.VaultCheckNameAvailabilityParameters{Name: &kvName, Type: to.StringPtr("Microsoft.KeyVault/vaults")},
nil,
)
if err != nil {
return err
}

result, err := c.vaultsClient.CheckNameAvailability(ctx, sdkkeyvault.VaultCheckNameAvailabilityParameters{Name: &kvName, Type: to.StringPtr("Microsoft.KeyVault/vaults")}, nil)
if err != nil {
return err
}
if result.NameAvailable == nil {
return fmt.Errorf("have unexpected nil NameAvailable for key vault: %v", kvName)
}

if result.NameAvailable != nil && !*result.NameAvailable {
return fmt.Errorf("could not generate unique key vault name: %v", result.Reason)
if *result.NameAvailable {
break
}
c.log.Infof("key vault %v is not available and we will try an other one", kvName)
}
}
}

Expand Down Expand Up @@ -315,11 +359,10 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str
}

diskEncryptionSetID := fmt.Sprintf(
"/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/diskEncryptionSets/%s%s",
"/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/diskEncryptionSets/%s",
c.env.SubscriptionID(),
vnetResourceGroup,
vnetResourceGroup,
generator.SharedDiskEncryptionSetNameSuffix,
diskEncryptionSetName,
)

c.log.Info("creating role assignments")
Expand Down
2 changes: 1 addition & 1 deletion pkg/validate/dynamic/dynamic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func NewValidator(

spNetworkUsage: usagesClient,
virtualNetworks: newVirtualNetworksCache(virtualNetworksClient),
diskEncryptionSets: compute.NewDiskEncryptionSetsClient(azEnv, subscriptionID, authorizer),
diskEncryptionSets: compute.NewDiskEncryptionSetsClientWithAROEnvironment(azEnv, subscriptionID, authorizer),
resourceSkusClient: compute.NewResourceSkusClient(azEnv, subscriptionID, authorizer),
pdpClient: pdpClient,
loadBalancerBackendAddressPoolsClient: network.NewLoadBalancerBackendAddressPoolsClient(azEnv, subscriptionID, authorizer),
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/setup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ func newClientSet(ctx context.Context) (*clientSet, error) {
VirtualMachines: compute.NewVirtualMachinesClient(_env.Environment(), _env.SubscriptionID(), authorizer),
Resources: features.NewResourcesClient(_env.Environment(), _env.SubscriptionID(), authorizer),
Disks: compute.NewDisksClient(_env.Environment(), _env.SubscriptionID(), authorizer),
DiskEncryptionSets: compute.NewDiskEncryptionSetsClient(_env.Environment(), _env.SubscriptionID(), authorizer),
DiskEncryptionSets: compute.NewDiskEncryptionSetsClientWithAROEnvironment(_env.Environment(), _env.SubscriptionID(), authorizer),
Interfaces: interfacesClient,
LoadBalancers: loadBalancersClient,
NetworkSecurityGroups: securityGroupsClient,
Expand Down

0 comments on commit 73579fb

Please sign in to comment.