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

Commit

Permalink
fix: de-dup vnet roleAssignment deployments (#3857)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jackfrancis
jackfrancis authored Sep 29, 2020
1 parent f13990b commit fa9bb91
Show file tree
Hide file tree
Showing 19 changed files with 342 additions and 3,489 deletions.
5 changes: 1 addition & 4 deletions examples/e2e-tests/kubernetes/release/default/definition.json
View file
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
View file
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
61 changes: 48 additions & 13 deletions pkg/engine/masterarmresources.go
View file
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,22 @@ 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)
// If the control plane is in a discrete VNET
if hasDistinctControlPlaneVNET(cs) {
// 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 Expand Up @@ -207,3 +212,33 @@ func createKubernetesMasterResourcesVMSS(cs *api.ContainerService) []interface{}

return masterResources
}

// hasDistinctControlPlaneVNET returns whether or not the VNET config of the control plane is distinct from any one node pool
// If the VnetSubnetID string is malformed in either the MasterProfile or any AgentPoolProfile, we return false
func hasDistinctControlPlaneVNET(cs *api.ContainerService) bool {
var controlPlaneVNETResourceURI string
if cs.Properties.MasterProfile.VnetSubnetID != "" {
controlPlaneSubnetElements := strings.Split(cs.Properties.MasterProfile.VnetSubnetID, "/")
if len(controlPlaneSubnetElements) >= 9 {
controlPlaneVNETResourceURI = strings.Join(controlPlaneSubnetElements[:9], "/")
} else {
return false
}
}
for _, agentPool := range cs.Properties.AgentPoolProfiles {
if agentPool.VnetSubnetID != "" {
nodePoolSubnetElements := strings.Split(agentPool.VnetSubnetID, "/")
if len(nodePoolSubnetElements) < 9 {
return false
}
if strings.Join(nodePoolSubnetElements[:9], "/") != controlPlaneVNETResourceURI {
return true
}
} else {
if cs.Properties.MasterProfile.VnetSubnetID != "" {
return true
}
}
}
return false
}
164 changes: 164 additions & 0 deletions pkg/engine/masterarmresources_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -853,3 +853,167 @@ func TestCreateKubernetesMasterResourcesVMSS(t *testing.T) {
t.Errorf("unexpected error while comparing ARM resources: %s", diff)
}
}

func TestHasDistinctControlPlaneVNET(t *testing.T) {
testcases := []struct {
name string
cs *api.ContainerService
expected bool
}{
{
name: "No VNET specified",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
},
},
},
},
expected: false,
},
{
name: "Control Plane has VNET",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
},
},
},
},
expected: true,
},
{
name: "Node pool has VNET",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
},
},
},
},
expected: true,
},
{
name: "VNET is shared",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
},
{
Name: "agentpool2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME/subnets/SUBNET_NAME",
},
},
},
},
expected: false,
},
{
name: "Heterogeneous VNETs",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME1/subnets/SUBNET_NAME",
},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME3/subnets/SUBNET_NAME",
},
},
},
},
expected: true,
},
{
name: "Same node pool VNETs, different control plane VNET",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME1/subnets/SUBNET_NAME",
},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool3",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool4",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
},
},
},
expected: true,
},
{
name: "All have the same VNET except one node pool",
cs: &api.ContainerService{
Properties: &api.Properties{
MasterProfile: &api.MasterProfile{
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
AgentPoolProfiles: []*api.AgentPoolProfile{
{
Name: "agentpool1",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool2",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool3",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME1/subnets/SUBNET_NAME",
},
{
Name: "agentpool4",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
{
Name: "agentpool5",
VnetSubnetID: "/subscriptions/SUB_ID/resourceGroups/RG_NAME/providers/Microsoft.Network/virtualNetworks/VNET_NAME2/subnets/SUBNET_NAME",
},
},
},
},
expected: true,
},
}
for _, testcase := range testcases {
actual := hasDistinctControlPlaneVNET(testcase.cs)
if testcase.expected != actual {
t.Errorf("Test \"%s\": expected hasDistinctControlPlaneVNET() to return %t, but got %t . ", testcase.name, testcase.expected, actual)
}
}
}
36 changes: 31 additions & 5 deletions pkg/engine/systemroleassignments.go
View file
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;
// 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
Loading

0 comments on commit fa9bb91

Please sign in to comment.