Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

fix: de-dup vnet roleAssignment deployments #3857

Merged
merged 10 commits into from
Sep 29, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"orchestratorProfile": {
"orchestratorType": "Kubernetes",
"kubernetesConfig": {
"useManagedIdentity": true,
"clusterSubnet": "10.239.0.0/16",
"addons": [
{
Expand Down Expand Up @@ -146,10 +147,6 @@
"adminPassword": "replacepassword1234$",
"sshEnabled": true,
"enableAutomaticUpdates": false
},
"servicePrincipalProfile": {
"clientId": "",
"secret": ""
}
}
}
3 changes: 1 addition & 2 deletions pkg/engine/armtype.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

"github.com/Azure/azure-sdk-for-go/services/cosmos-db/mgmt/2015-04-08/documentdb"
"github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault"
sysauth "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization"
"github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources"

Expand Down Expand Up @@ -116,7 +115,7 @@ type StorageAccountARM struct {
// SystemRoleAssignmentARM embeds the ARMResource type in authorization.SystemRoleAssignment(2018-01-01-preview).
type SystemRoleAssignmentARM struct {
ARMResource
sysauth.RoleAssignment
authorization.RoleAssignment
}

// VirtualNetworkARM embeds the ARMResource type in network.VirtualNetwork.
Expand Down
50 changes: 37 additions & 13 deletions pkg/engine/masterarmresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package engine

import (
"strings"

"github.com/Azure/aks-engine/pkg/api"
"github.com/Azure/go-autorest/autorest/to"
)
Expand Down Expand Up @@ -120,19 +122,41 @@ func createKubernetesMasterResourcesVMAS(cs *api.ContainerService) []interface{}
masterCSE.ARMResource.DependsOn = append(masterCSE.ARMResource.DependsOn, "[concat('Microsoft.KeyVault/vaults/', variables('clusterKeyVaultName'))]")
}

// TODO: This is only necessary if the resource group of the masters is different from the RG of the node pool
// subnet. But when we generate the template we don't know to which RG it will be deployed to. To solve this we
// would have to add the necessary condition into the template. For the resources we can use the `condition` field
// but how can we conditionally declare the dependencies? Perhaps by creating a variable for the dependency array
// and conditionally adding more dependencies.
if kubernetesConfig.SystemAssignedIDEnabled() &&
// The fix for ticket 2373 is only available for individual VMs / AvailabilitySet.
cs.Properties.MasterProfile.IsAvailabilitySet() {
masterRoleAssignmentForAgentPools := createKubernetesMasterRoleAssignmentForAgentPools(cs.Properties.MasterProfile, cs.Properties.AgentPoolProfiles)

for _, assignmentForAgentPool := range masterRoleAssignmentForAgentPools {
masterResources = append(masterResources, assignmentForAgentPool)
masterCSE.ARMResource.DependsOn = append(masterCSE.ARMResource.DependsOn, *assignmentForAgentPool.Name)
var hasDistinctControlPlaneVNET bool
if cs.Properties.MasterProfile.IsCustomVNET() {
var controlPlaneVNETResourceURI string
controlPlaneSubnetElements := strings.Split(cs.Properties.MasterProfile.VnetSubnetID, "/")
if len(controlPlaneSubnetElements) >= 9 {
controlPlaneVNETResourceURI = strings.Join(controlPlaneSubnetElements[:9], "/")
}
if controlPlaneVNETResourceURI != "" {
for _, agentPool := range cs.Properties.AgentPoolProfiles {
subnetElements := strings.Split(agentPool.VnetSubnetID, "/")
if len(subnetElements) < 9 {
continue
}
if strings.Join(subnetElements[:9], "/") != controlPlaneVNETResourceURI {
hasDistinctControlPlaneVNET = true
}
}
}
}
// If the control plane is in a discrete VNET
if hasDistinctControlPlaneVNET {
// TODO: This is only necessary if the resource group of the masters is different from the RG of the node pool
// subnet. But when we generate the template we don't know to which RG it will be deployed to. To solve this we
// would have to add the necessary condition into the template. For the resources we can use the `condition` field
// but how can we conditionally declare the dependencies? Perhaps by creating a variable for the dependency array
// and conditionally adding more dependencies.
if kubernetesConfig.SystemAssignedIDEnabled() &&
// The fix for ticket 2373 is only available for individual VMs / AvailabilitySet.
cs.Properties.MasterProfile.IsAvailabilitySet() {
masterRoleAssignmentForAgentPools := createKubernetesMasterRoleAssignmentForAgentPools(cs.Properties.MasterProfile, cs.Properties.AgentPoolProfiles)

for _, assignmentForAgentPool := range masterRoleAssignmentForAgentPools {
masterResources = append(masterResources, assignmentForAgentPool)
masterCSE.ARMResource.DependsOn = append(masterCSE.ARMResource.DependsOn, *assignmentForAgentPool.Name)
}
}
}

Expand Down
36 changes: 31 additions & 5 deletions pkg/engine/systemroleassignments.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ package engine

import (
"fmt"
"strings"

"github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources"

"github.com/Azure/aks-engine/pkg/api"
"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization"
"github.com/Azure/go-autorest/autorest/to"
)

Expand All @@ -32,6 +33,7 @@ func createVMASRoleAssignment() SystemRoleAssignmentARM {
systemRoleAssignment.RoleAssignmentPropertiesWithScope = &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('contributorRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(concat('Microsoft.Compute/virtualMachines/', variables('masterVMNamePrefix'), copyIndex(variables('masterOffset'))), '2017-03-30', 'Full').identity.principalId]"),
PrincipalType: authorization.ServicePrincipal,
}
return systemRoleAssignment
}
Expand All @@ -56,6 +58,7 @@ func createAgentVMASSysRoleAssignment(profile *api.AgentPoolProfile) SystemRoleA
systemRoleAssignment.RoleAssignmentPropertiesWithScope = &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('readerRoleDefinitionId')]"),
PrincipalID: to.StringPtr(fmt.Sprintf("[reference(concat('Microsoft.Compute/virtualMachines/', variables('%[1]sVMNamePrefix'), copyIndex(variables('%[1]sOffset'))), '2017-03-30', 'Full').identity.principalId]", profile.Name)),
PrincipalType: authorization.ServicePrincipal,
}

return systemRoleAssignment
Expand All @@ -76,6 +79,7 @@ func createAgentVMSSSysRoleAssignment(profile *api.AgentPoolProfile) SystemRoleA
systemRoleAssignment.RoleAssignmentPropertiesWithScope = &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('readerRoleDefinitionId')]"),
PrincipalID: to.StringPtr(fmt.Sprintf("[reference(concat('Microsoft.Compute/virtualMachineScaleSets/', variables('%[1]sVMNamePrefix')), '2017-03-30', 'Full').identity.principalId]", profile.Name)),
PrincipalType: authorization.ServicePrincipal,
}

return systemRoleAssignment
Expand All @@ -88,13 +92,35 @@ func createKubernetesMasterRoleAssignmentForAgentPools(masterProfile *api.Master
dependenciesToMasterVms[masterIdx] = fmt.Sprintf("[concat(variables('masterVMNamePrefix'), %d)]", masterIdx)
}

var roleAssignmentsForAllAgentPools = make([]DeploymentWithResourceGroupARM, len(agentPoolProfiles))
roleAssignmentsForAllAgentPools := []DeploymentWithResourceGroupARM{}

// The following is based on:
// * https://github.com/MicrosoftDocs/azure-docs/blob/master/articles/role-based-access-control/role-assignments-template.md#create-a-role-assignment-at-a-resource-scope
// * https://github.com/Azure/azure-quickstart-templates/blob/master/201-rbac-builtinrole-multipleVMs/azuredeploy.json#L79
for agentPoolIdx, agentPool := range agentPoolProfiles {

// We're gonna keep track of distinct VNETs in use across all our node pools;
// we only want to define master VM --> VNET role assignments once per VNET.
// If our cluster configuration includes more than one pool sharing a common VNET,
// we define the master VM --> VNET role assignments (one per master VM) just once for those pools
var vnetInCluster = struct{}{}
vnets := make(map[string]struct{})
for _, agentPool := range agentPoolProfiles {
var roleAssignments = make([]interface{}, masterProfile.Count)
subnetElements := strings.Split(agentPool.VnetSubnetID, "/")
// We expect a very specific string format for the VnetSubnetID property;
mboersma marked this conversation as resolved.
Show resolved Hide resolved
// if it can't be split into at least 9 "/"-delimited elements,
// then we should assume that our role assignment composition below will be malformed,
// and so we simply skip assigning a role assigment for this pool.
// This should never happen, but this defensive posture ensures no code panic execution path
// // when we statically grab the first 9 elements (`subnetElements[:9]` below)
if len(subnetElements) < 9 {
continue
}
vnetResourceURI := strings.Join(subnetElements[:9], "/")
if _, ok := vnets[vnetResourceURI]; ok {
continue
}
vnets[vnetResourceURI] = vnetInCluster

for masterIdx := 0; masterIdx < masterProfile.Count; masterIdx++ {
masterVMReference := fmt.Sprintf("reference(resourceId(resourceGroup().name, 'Microsoft.Compute/virtualMachines', concat(variables('masterVMNamePrefix'), %d)), '2017-03-30', 'Full').identity.principalId", masterIdx)
Expand All @@ -110,7 +136,7 @@ func createKubernetesMasterRoleAssignmentForAgentPools(masterProfile *api.Master
},
*/
},
// Reference to the subnet of the worker VMs:
// Reference to the VNET of the worker VMs:
RoleAssignment: authorization.RoleAssignment{
Name: to.StringPtr(fmt.Sprintf("[concat(variables('%sVnet'), '/Microsoft.Authorization/', guid(uniqueString(%s)))]", agentPool.Name, masterVMReference)),
Type: to.StringPtr("Microsoft.Network/virtualNetworks/providers/roleAssignments"),
Expand Down Expand Up @@ -146,7 +172,7 @@ func createKubernetesMasterRoleAssignmentForAgentPools(masterProfile *api.Master
},
}

roleAssignmentsForAllAgentPools[agentPoolIdx] = roleAssignmentsForAgentPoolSubDeployment
roleAssignmentsForAllAgentPools = append(roleAssignmentsForAllAgentPools, roleAssignmentsForAgentPoolSubDeployment)
}

return roleAssignmentsForAllAgentPools
Expand Down
84 changes: 80 additions & 4 deletions pkg/engine/systemroleassignments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/Azure/aks-engine/pkg/api"
"github.com/google/go-cmp/cmp"

"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization"
"github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/resources"
"github.com/Azure/go-autorest/autorest/to"
)
Expand All @@ -35,6 +35,7 @@ func TestCreateVmasRoleAssignment(t *testing.T) {
RoleAssignmentPropertiesWithScope: &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('contributorRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(concat('Microsoft.Compute/virtualMachines/', variables('masterVMNamePrefix'), copyIndex(variables('masterOffset'))), '2017-03-30', 'Full').identity.principalId]"),
PrincipalType: authorization.ServicePrincipal,
},
},
}
Expand Down Expand Up @@ -70,6 +71,7 @@ func TestCreateAgentVmasSysRoleAssignment(t *testing.T) {
RoleAssignmentPropertiesWithScope: &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('readerRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(concat('Microsoft.Compute/virtualMachines/', variables('fooprofileVMNamePrefix'), copyIndex(variables('fooprofileOffset'))), '2017-03-30', 'Full').identity.principalId]"),
PrincipalType: authorization.ServicePrincipal,
},
},
}
Expand Down Expand Up @@ -102,6 +104,7 @@ func TestCreateAgentVmssSysRoleAssignment(t *testing.T) {
RoleAssignmentPropertiesWithScope: &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('readerRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(concat('Microsoft.Compute/virtualMachineScaleSets/', variables('fooprofileVMNamePrefix')), '2017-03-30', 'Full').identity.principalId]"),
PrincipalType: authorization.ServicePrincipal,
},
},
}
Expand All @@ -118,11 +121,14 @@ func TestCreateKubernetesMasterRoleAssignmentForAgentPools(t *testing.T) {
masterProfile := &api.MasterProfile{
Count: 2,
}
// Two pools sharing a common VNET
agentProfile1 := &api.AgentPoolProfile{
Name: "agentProfile1",
Name: "agentProfile1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET1_NAME",
}
agentProfile2 := &api.AgentPoolProfile{
Name: "agentProfile2",
Name: "agentProfile2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET2_NAME",
}

actual := createKubernetesMasterRoleAssignmentForAgentPools(masterProfile, []*api.AgentPoolProfile{agentProfile1, agentProfile2})
Expand Down Expand Up @@ -177,6 +183,76 @@ func TestCreateKubernetesMasterRoleAssignmentForAgentPools(t *testing.T) {
},
},
},
}

diff := cmp.Diff(actual, expected)

if diff != "" {
t.Errorf("unexpected diff while comparing: %s", diff)
}

// Two pools with discrete VNETs
agentProfile1 = &api.AgentPoolProfile{
Name: "agentProfile1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET1_NAME/subnets/SUBNET1_NAME",
}
agentProfile2 = &api.AgentPoolProfile{
Name: "agentProfile2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET2_NAME/subnets/SUBNET2_NAME",
}

actual = createKubernetesMasterRoleAssignmentForAgentPools(masterProfile, []*api.AgentPoolProfile{agentProfile1, agentProfile2})

expected = []DeploymentWithResourceGroupARM{
{
DeploymentARMResource: DeploymentARMResource{
APIVersion: "2017-05-10",
DependsOn: []string{
"[concat(variables('masterVMNamePrefix'), 0)]",
"[concat(variables('masterVMNamePrefix'), 1)]",
},
},
ResourceGroup: to.StringPtr("[variables('agentProfile1SubnetResourceGroup')]"),
DeploymentExtended: resources.DeploymentExtended{
Name: to.StringPtr("[concat('masterMsiRoleAssignment-', variables('agentProfile1VMNamePrefix'))]"),
Type: to.StringPtr("Microsoft.Resources/deployments"),
Properties: &resources.DeploymentPropertiesExtended{
Mode: "Incremental",
Template: map[string]interface{}{
"resources": []interface{}{
SystemRoleAssignmentARM{
ARMResource: ARMResource{
APIVersion: "[variables('apiVersionAuthorizationSystem')]",
},
RoleAssignment: authorization.RoleAssignment{
Name: to.StringPtr("[concat(variables('agentProfile1Vnet'), '/Microsoft.Authorization/', guid(uniqueString(reference(resourceId(resourceGroup().name, 'Microsoft.Compute/virtualMachines', concat(variables('masterVMNamePrefix'), 0)), '2017-03-30', 'Full').identity.principalId)))]"),
Type: to.StringPtr("Microsoft.Network/virtualNetworks/providers/roleAssignments"),
RoleAssignmentPropertiesWithScope: &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('networkContributorRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(resourceId(resourceGroup().name, 'Microsoft.Compute/virtualMachines', concat(variables('masterVMNamePrefix'), 0)), '2017-03-30', 'Full').identity.principalId]"),
},
},
},
SystemRoleAssignmentARM{
ARMResource: ARMResource{
APIVersion: "[variables('apiVersionAuthorizationSystem')]",
},
RoleAssignment: authorization.RoleAssignment{
Name: to.StringPtr("[concat(variables('agentProfile1Vnet'), '/Microsoft.Authorization/', guid(uniqueString(reference(resourceId(resourceGroup().name, 'Microsoft.Compute/virtualMachines', concat(variables('masterVMNamePrefix'), 1)), '2017-03-30', 'Full').identity.principalId)))]"),
Type: to.StringPtr("Microsoft.Network/virtualNetworks/providers/roleAssignments"),
RoleAssignmentPropertiesWithScope: &authorization.RoleAssignmentPropertiesWithScope{
RoleDefinitionID: to.StringPtr("[variables('networkContributorRoleDefinitionId')]"),
PrincipalID: to.StringPtr("[reference(resourceId(resourceGroup().name, 'Microsoft.Compute/virtualMachines', concat(variables('masterVMNamePrefix'), 1)), '2017-03-30', 'Full').identity.principalId]"),
},
},
},
},
"contentVersion": "1.0.0.0",
"$schema": "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
},
},
},
},
{
DeploymentARMResource: DeploymentARMResource{
APIVersion: "2017-05-10",
Expand Down Expand Up @@ -228,7 +304,7 @@ func TestCreateKubernetesMasterRoleAssignmentForAgentPools(t *testing.T) {
},
}

diff := cmp.Diff(actual, expected)
diff = cmp.Diff(actual, expected)

if diff != "" {
t.Errorf("unexpected diff while comparing: %s", diff)
Expand Down
16 changes: 14 additions & 2 deletions test/e2e/azure/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package azure

import (
"bytes"
"context"
"encoding/json"
"fmt"
Expand Down Expand Up @@ -278,16 +279,27 @@ func (a *Account) CreateDeployment(name string, e *engine.Engine) error {
}
}()

cmd := exec.Command("az", "deployment", "group", "create",
azArgsStringSlice := []string{"deployment", "group", "create",
"--name", d.Name,
"--resource-group", a.ResourceGroup.Name,
"--template-file", e.Config.GeneratedTemplatePath,
"--parameters", e.Config.GeneratedParametersPath)
"--parameters", e.Config.GeneratedParametersPath}
cmd := exec.Command("az", azArgsStringSlice...)
util.PrintCommand(cmd)
out, err := cmd.CombinedOutput()
if err != nil {
log.Printf("\nError from deployment for %s in resource group %s:%s\n", d.Name, a.ResourceGroup.Name, err)
log.Printf("Command Output: %s\n", out)
if bytes.Contains(out, []byte("PrincipalNotFound")) {
for err != nil {
cmd = exec.Command("az", azArgsStringSlice...)
util.PrintCommand(cmd)
out, err = cmd.CombinedOutput()
if err != nil {
log.Printf("Command Output: %s\n", out)
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of the result of this change:

$ az deployment group create --name kubernetes-eastus-37551 --resource-group kubernetes-eastus-37551 --template-file /Users/jackfrancis/work/src/github.com/Azure/aks-engine/_output/kubernetes-eastus-37551/azuredeploy.json --parameters /Users/jackfrancis/work/src/github.com/Azure/aks-engine/_output/kubernetes-eastus-37551/azuredeploy.parameters.json
.......2020/09/23 17:26:56 
Error from deployment for kubernetes-eastus-37551 in resource group kubernetes-eastus-37551:exit status 1
2020/09/23 17:26:56 Command Output: Deployment failed. Correlation ID: 0934f995-914a-47ec-a60c-cbf5774220ba. {
  "error": {
    "code": "PrincipalNotFound",
    "message": "Principal 4aaa67de8ac644c5bf234a1d069a995e does not exist in the directory 72f988bf-86f1-41af-91ab-2d7cd011db47."
  }
}


$ az deployment group create --name kubernetes-eastus-37551 --resource-group kubernetes-eastus-37551 --template-file /Users/jackfrancis/work/src/github.com/Azure/aks-engine/_output/kubernetes-eastus-37551/azuredeploy.json --parameters /Users/jackfrancis/work/src/github.com/Azure/aks-engine/_output/kubernetes-eastus-37551/azuredeploy.parameters.json
.

And then the E2E execution proceeded successfully. Basically, this change enables us to functionally test system-assigned identity reliably, as we need to assume that such a config will regularly produce an error response from the initial ARM template deployment based on observed behaviors.

Additionally, documentation outlining this "redeploy your ARM template" guidance will be added to this PR:

#3837

return err
}
quit <- true
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/engine/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func Build(cfg *config.Config, masterSubnetID string, agentSubnetIDs []string, i
isAzureStackCloud = true
}

if config.ClientID != "" && config.ClientSecret != "" {
if config.ClientID != "" && config.ClientSecret != "" && prop.OrchestratorProfile.KubernetesConfig != nil && !prop.OrchestratorProfile.KubernetesConfig.UseManagedIdentity {
if !prop.IsAzureStackCloud() {
prop.ServicePrincipalProfile = &vlabs.ServicePrincipalProfile{
ClientID: config.ClientID,
Expand Down
Loading