From 3685380dbcf70a2f3adf70bbf075c61ce3708c5d Mon Sep 17 00:00:00 2001 From: Brian Lieberman Date: Fri, 24 Jun 2022 11:29:37 -0400 Subject: [PATCH] switch to int-based ipConfig management + feedback --- api/v1beta1/azuremachine_webhook_test.go | 8 +- api/v1beta1/types.go | 23 ++++- api/v1beta1/zz_generated.deepcopy.go | 5 - azure/scope/machine.go | 96 ++++++++++--------- azure/scope/machine_test.go | 11 ++- azure/services/scalesets/scalesets.go | 32 ++++++- azure/services/scalesets/scalesets_test.go | 10 +- ...re.cluster.x-k8s.io_azuremachinepools.yaml | 23 ++--- ...ucture.cluster.x-k8s.io_azuremachines.yaml | 21 ++-- ...luster.x-k8s.io_azuremachinetemplates.yaml | 23 ++--- 10 files changed, 142 insertions(+), 110 deletions(-) diff --git a/api/v1beta1/azuremachine_webhook_test.go b/api/v1beta1/azuremachine_webhook_test.go index 208116ab557..0eebd5555e2 100644 --- a/api/v1beta1/azuremachine_webhook_test.go +++ b/api/v1beta1/azuremachine_webhook_test.go @@ -105,17 +105,17 @@ func TestAzureMachine_ValidateCreate(t *testing.T) { }, { name: "azuremachine with invalid network configuration", - machine: createrMachineWithNetworkConfig("subnet", []AzureNetworkInterface{{SubnetName: "subnet1"}}), + machine: createMachineWithNetworkConfig("subnet", []AzureNetworkInterface{{SubnetName: "subnet1"}}), wantErr: true, }, { name: "azuremachine with valid legacy network configuration", - machine: createrMachineWithNetworkConfig("subnet", []AzureNetworkInterface{}), + machine: createMachineWithNetworkConfig("subnet", []AzureNetworkInterface{}), wantErr: false, }, { name: "azuremachine with valid network configuration", - machine: createrMachineWithNetworkConfig("", []AzureNetworkInterface{{SubnetName: "subnet"}}), + machine: createMachineWithNetworkConfig("", []AzureNetworkInterface{{SubnetName: "subnet"}}), wantErr: false, }, } @@ -621,7 +621,7 @@ func TestAzureMachine_Default(t *testing.T) { } } -func createrMachineWithNetworkConfig(subnetName string, interfaces []AzureNetworkInterface) *AzureMachine { +func createMachineWithNetworkConfig(subnetName string, interfaces []AzureNetworkInterface) *AzureMachine { return &AzureMachine{ Spec: AzureMachineSpec{ SubnetName: subnetName, diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index df5e3ea8047..bfb803bd0dd 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -582,10 +582,25 @@ type SubnetSpec struct { // Network Interfaces to attach to each VM // +optional type AzureNetworkInterface struct { - SubnetName string `json:"subnetName,omitempty"` - IPConfigs []AzureIPConfig `json:"ipConfigs,omitempty"` - AcceleratedNetworking *bool `json:"acceleratedNetworking,omitempty"` - ID string `json:"id,omitempty"` + // The subnet to place the interface in + SubnetName string `json:"subnetName,omitempty"` + + // Number of private IP address to attach to the interface + // +optional + PrivateIPConfigs int `json:"privateIPConfigs,omitempty"` + + // Number of public IP addresses to attach to the interface + // +optional + + PublicIPConfigs int `json:"publicIPConfigs,omitempty"` + + // Enable acccelerated networking on the interface + // +optional + AcceleratedNetworking *bool `json:"acceleratedNetworking,omitempty"` + + // Attach an already provisioned interface by ID + // +optional + ID string `json:"id,omitempty"` } // IP Configuration defines options to confiure a network interface. diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 05d73c28a59..7ad1a57c279 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -789,11 +789,6 @@ func (in *AzureMarketplaceImage) DeepCopy() *AzureMarketplaceImage { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureNetworkInterface) DeepCopyInto(out *AzureNetworkInterface) { *out = *in - if in.IPConfigs != nil { - in, out := &in.IPConfigs, &out.IPConfigs - *out = make([]AzureIPConfig, len(*in)) - copy(*out, *in) - } if in.AcceleratedNetworking != nil { in, out := &in.AcceleratedNetworking, &out.AcceleratedNetworking *out = new(bool) diff --git a/azure/scope/machine.go b/azure/scope/machine.go index 742e3b091b0..a269a100acd 100644 --- a/azure/scope/machine.go +++ b/azure/scope/machine.go @@ -224,45 +224,7 @@ func (m *MachineScope) NICSpecs() []azure.ResourceSpecGetter { nicSpecs := []azure.ResourceSpecGetter{} if len(m.AzureMachine.Spec.NetworkInterfaces) < 1 { - spec := &networkinterfaces.NICSpec{ - Name: azure.GenerateNICName(m.Name()), - ResourceGroup: m.ResourceGroup(), - Location: m.Location(), - SubscriptionID: m.SubscriptionID(), - MachineName: m.Name(), - VNetName: m.Vnet().Name, - VNetResourceGroup: m.Vnet().ResourceGroup, - AcceleratedNetworking: m.AzureMachine.Spec.AcceleratedNetworking, - IPv6Enabled: m.IsIPv6Enabled(), - EnableIPForwarding: m.AzureMachine.Spec.EnableIPForwarding, - SubnetName: m.Subnet().Name, - } - if m.Role() == infrav1.ControlPlane { - spec.PublicLBName = m.OutboundLBName(m.Role()) - spec.PublicLBAddressPoolName = m.OutboundPoolName(m.OutboundLBName(m.Role())) - if m.IsAPIServerPrivate() { - spec.InternalLBName = m.APIServerLBName() - spec.InternalLBAddressPoolName = m.APIServerLBPoolName(m.APIServerLBName()) - } else { - spec.PublicLBNATRuleName = m.Name() - spec.PublicLBAddressPoolName = m.APIServerLBPoolName(m.APIServerLBName()) - } - } - - // If NAT gateway is not enabled and node has no public IP, then the NIC needs to reference the LB to get outbound traffic. - if m.Role() == infrav1.Node && !m.Subnet().IsNatGatewayEnabled() && !m.AzureMachine.Spec.AllocatePublicIP { - spec.PublicLBName = m.OutboundLBName(m.Role()) - spec.PublicLBAddressPoolName = m.OutboundPoolName(m.OutboundLBName(m.Role())) - } - - if m.cache != nil { - spec.SKU = &m.cache.VMSKU - } - - if m.Role() == infrav1.Node && m.AzureMachine.Spec.AllocatePublicIP { - spec.PublicIPName = azure.GenerateNodePublicIPName(m.Name()) - } - return append(nicSpecs, spec) + return append(nicSpecs, getDefaultNetworkInterfaceSpec(m)) } for i, n := range m.AzureMachine.Spec.NetworkInterfaces { @@ -313,13 +275,16 @@ func (m *MachineScope) NICSpecs() []azure.ResourceSpecGetter { spec.SKU = &m.cache.VMSKU } - for _, c := range n.IPConfigs { - config := networkinterfaces.IPConfig{ - PublicIP: c.PublicIP, - PrivateIP: c.PrivateIP, - PublicIPAddress: c.PublicIPAddress, + // Create an IPconfig for both public + private IP pair + if n.PrivateIPConfigs >= n.PublicIPConfigs { + for i := 0; i < n.PublicIPConfigs; i++ { + spec.IPConfigs = append(spec.IPConfigs, networkinterfaces.IPConfig{PublicIP: true}) } - spec.IPConfigs = append(spec.IPConfigs, config) + } + + // Create any remaining private IPConfigs + for i := 0; i < n.PrivateIPConfigs-n.PublicIPConfigs; i++ { + spec.IPConfigs = append(spec.IPConfigs, networkinterfaces.IPConfig{}) } nicSpecs = append(nicSpecs, spec) } @@ -340,6 +305,47 @@ func (m *MachineScope) NICIDs() []string { } return nicIDs } +func getDefaultNetworkInterfaceSpec(m *MachineScope) *networkinterfaces.NICSpec { + spec := &networkinterfaces.NICSpec{ + Name: azure.GenerateNICName(m.Name()), + ResourceGroup: m.ResourceGroup(), + Location: m.Location(), + SubscriptionID: m.SubscriptionID(), + MachineName: m.Name(), + VNetName: m.Vnet().Name, + VNetResourceGroup: m.Vnet().ResourceGroup, + AcceleratedNetworking: m.AzureMachine.Spec.AcceleratedNetworking, + IPv6Enabled: m.IsIPv6Enabled(), + EnableIPForwarding: m.AzureMachine.Spec.EnableIPForwarding, + SubnetName: m.Subnet().Name, + } + if m.Role() == infrav1.ControlPlane { + spec.PublicLBName = m.OutboundLBName(m.Role()) + spec.PublicLBAddressPoolName = m.OutboundPoolName(m.OutboundLBName(m.Role())) + if m.IsAPIServerPrivate() { + spec.InternalLBName = m.APIServerLBName() + spec.InternalLBAddressPoolName = m.APIServerLBPoolName(m.APIServerLBName()) + } else { + spec.PublicLBNATRuleName = m.Name() + spec.PublicLBAddressPoolName = m.APIServerLBPoolName(m.APIServerLBName()) + } + } + + // If NAT gateway is not enabled and node has no public IP, then the NIC needs to reference the LB to get outbound traffic. + if m.Role() == infrav1.Node && !m.Subnet().IsNatGatewayEnabled() && !m.AzureMachine.Spec.AllocatePublicIP { + spec.PublicLBName = m.OutboundLBName(m.Role()) + spec.PublicLBAddressPoolName = m.OutboundPoolName(m.OutboundLBName(m.Role())) + } + + if m.cache != nil { + spec.SKU = &m.cache.VMSKU + } + + if m.Role() == infrav1.Node && m.AzureMachine.Spec.AllocatePublicIP { + spec.PublicIPName = azure.GenerateNodePublicIPName(m.Name()) + } + return spec +} // DiskSpecs returns the disk specs. func (m *MachineScope) DiskSpecs() []azure.ResourceSpecGetter { diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index 9171ea265ae..98c16a5ef2d 100644 --- a/azure/scope/machine_test.go +++ b/azure/scope/machine_test.go @@ -2145,12 +2145,13 @@ func TestMachineScope_NICSpecs(t *testing.T) { { SubnetName: "subnet1", AcceleratedNetworking: pointer.Bool(true), - IPConfigs: []infrav1.AzureIPConfig{{}}, + PrivateIPConfigs: 2, }, { SubnetName: "subnet2", AcceleratedNetworking: pointer.Bool(true), - IPConfigs: []infrav1.AzureIPConfig{{}}, + PrivateIPConfigs: 2, + PublicIPConfigs: 1, }, }, }, @@ -2170,7 +2171,7 @@ func TestMachineScope_NICSpecs(t *testing.T) { SubscriptionID: "123", MachineName: "machine-name", SubnetName: "subnet1", - IPConfigs: []networkinterfaces.IPConfig{{}}, + IPConfigs: []networkinterfaces.IPConfig{{}, {}}, VNetName: "vnet1", VNetResourceGroup: "rg1", PublicLBName: "outbound-lb", @@ -2191,7 +2192,7 @@ func TestMachineScope_NICSpecs(t *testing.T) { SubscriptionID: "123", MachineName: "machine-name", SubnetName: "subnet2", - IPConfigs: []networkinterfaces.IPConfig{{}}, + IPConfigs: []networkinterfaces.IPConfig{{PublicIP: true}, {}}, VNetName: "vnet1", VNetResourceGroup: "rg1", PublicLBName: "", @@ -2274,7 +2275,7 @@ func TestMachineScope_NICSpecs(t *testing.T) { { SubnetName: "subnet1", AcceleratedNetworking: pointer.Bool(true), - IPConfigs: []infrav1.AzureIPConfig{{}, {}, {}, {}, {}, {}, {}, {}, {}, {}}, + PrivateIPConfigs: 10, }, }, }, diff --git a/azure/services/scalesets/scalesets.go b/azure/services/scalesets/scalesets.go index 340ea2776ea..e7c45eea7e9 100644 --- a/azure/services/scalesets/scalesets.go +++ b/azure/services/scalesets/scalesets.go @@ -467,7 +467,7 @@ func (s *Service) buildVMSSFromSpec(ctx context.Context, vmssSpec azure.ScaleSet } else { nicConfig.VirtualMachineScaleSetNetworkConfigurationProperties.EnableAcceleratedNetworking = to.BoolPtr(false) } - if len(n.IPConfigs) == 0 { + if n.PrivateIPConfigs == 0 && n.PublicIPConfigs == 0 { nicConfig.VirtualMachineScaleSetNetworkConfigurationProperties.IPConfigurations = &[]compute.VirtualMachineScaleSetIPConfiguration{ { Name: to.StringPtr(vmssSpec.Name + "-" + strconv.Itoa(i)), @@ -483,9 +483,35 @@ func (s *Service) buildVMSSFromSpec(ctx context.Context, vmssSpec azure.ScaleSet } } else { ipconfigs := []compute.VirtualMachineScaleSetIPConfiguration{} - for j := range n.IPConfigs { + for j := 0; j < n.PublicIPConfigs; j++ { ipconfig := compute.VirtualMachineScaleSetIPConfiguration{ - Name: to.StringPtr(fmt.Sprintf("ipConfig%v", j)), + Name: to.StringPtr(fmt.Sprintf("public-ipConfig%v", j)), + VirtualMachineScaleSetIPConfigurationProperties: &compute.VirtualMachineScaleSetIPConfigurationProperties{ + PrivateIPAddressVersion: compute.IPVersionIPv4, + Subnet: &compute.APIEntityReference{ + ID: to.StringPtr(azure.SubnetID(s.Scope.SubscriptionID(), vmssSpec.VNetResourceGroup, vmssSpec.VNetName, n.SubnetName)), + }, + PublicIPAddressConfiguration: &compute.VirtualMachineScaleSetPublicIPAddressConfiguration{}, + }, + } + if j == 0 { + ipconfig.Primary = to.BoolPtr(true) + if i == 0 { + // only set Load Balancer Backend Address Pool on primary nic/ipconfig + ipconfig.LoadBalancerBackendAddressPools = &backendAddressPools + } + } else { + ipconfig.Primary = to.BoolPtr(false) + } + ipconfig.Subnet = &compute.APIEntityReference{ + ID: to.StringPtr(azure.SubnetID(s.Scope.SubscriptionID(), vmssSpec.VNetResourceGroup, vmssSpec.VNetName, n.SubnetName)), + } + ipconfigs = append(ipconfigs, ipconfig) + } + // Create any remaining private IPConfigs + for j := 0; j < n.PrivateIPConfigs-n.PublicIPConfigs; j++ { + ipconfig := compute.VirtualMachineScaleSetIPConfiguration{ + Name: to.StringPtr(fmt.Sprintf("private-ipConfig%v", j)), VirtualMachineScaleSetIPConfigurationProperties: &compute.VirtualMachineScaleSetIPConfigurationProperties{ PrivateIPAddressVersion: compute.IPVersionIPv4, Subnet: &compute.APIEntityReference{ diff --git a/azure/services/scalesets/scalesets_test.go b/azure/services/scalesets/scalesets_test.go index caa9695dcde..94f934f75a5 100644 --- a/azure/services/scalesets/scalesets_test.go +++ b/azure/services/scalesets/scalesets_test.go @@ -271,12 +271,12 @@ func TestReconcileVMSS(t *testing.T) { spec.NetworkInterfaces = []infrav1.AzureNetworkInterface{ { SubnetName: "my-subnet", - IPConfigs: []infrav1.AzureIPConfig{{}}, + PrivateIPConfigs: 1, AcceleratedNetworking: pointer.Bool(true), }, { SubnetName: "subnet2", - IPConfigs: []infrav1.AzureIPConfig{{}, {}}, + PrivateIPConfigs: 2, AcceleratedNetworking: pointer.Bool(true), }, } @@ -288,12 +288,12 @@ func TestReconcileVMSS(t *testing.T) { (*netConfigs)[0].Name = to.StringPtr("my-vmss-0") (*netConfigs)[0].EnableIPForwarding = nil nic1IPConfigs := (*netConfigs)[0].IPConfigurations - (*nic1IPConfigs)[0].Name = to.StringPtr("ipConfig0") + (*nic1IPConfigs)[0].Name = to.StringPtr("private-ipConfig0") (*nic1IPConfigs)[0].PrivateIPAddressVersion = compute.IPVersionIPv4 (*netConfigs)[0].EnableAcceleratedNetworking = to.BoolPtr(true) vmssIPConfigs := []compute.VirtualMachineScaleSetIPConfiguration{ { - Name: to.StringPtr("ipConfig0"), + Name: to.StringPtr("private-ipConfig0"), VirtualMachineScaleSetIPConfigurationProperties: &compute.VirtualMachineScaleSetIPConfigurationProperties{ Primary: to.BoolPtr(true), PrivateIPAddressVersion: compute.IPVersionIPv4, @@ -304,7 +304,7 @@ func TestReconcileVMSS(t *testing.T) { }, }, { - Name: to.StringPtr("ipConfig1"), + Name: to.StringPtr("private-ipConfig1"), VirtualMachineScaleSetIPConfigurationProperties: &compute.VirtualMachineScaleSetIPConfigurationProperties{ Primary: to.BoolPtr(false), PrivateIPAddressVersion: compute.IPVersionIPv4, diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml index 1e5bd99e9c6..76427d0d58a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinepools.yaml @@ -1712,23 +1712,20 @@ spec: description: Network Interfaces to attach to each VM properties: acceleratedNetworking: + description: Enable acccelerated networking on the interface type: boolean id: + description: Attach an already provisioned interface by + ID type: string - ipConfigs: - items: - description: IP Configuration defines options to confiure - a network interface. - properties: - privateIP: - type: string - publicIP: - type: boolean - publicIPAddress: - type: string - type: object - type: array + privateIPConfigs: + description: Number of private IP address to attach to the + interface + type: integer + publicIPConfigs: + type: integer subnetName: + description: The subnet to place the interface in type: string type: object type: array diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml index 8f5fc611b1f..da5dc7a45da 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachines.yaml @@ -1317,23 +1317,18 @@ spec: description: Network Interfaces to attach to each VM properties: acceleratedNetworking: + description: Enable acccelerated networking on the interface type: boolean id: + description: Attach an already provisioned interface by ID type: string - ipConfigs: - items: - description: IP Configuration defines options to confiure - a network interface. - properties: - privateIP: - type: string - publicIP: - type: boolean - publicIPAddress: - type: string - type: object - type: array + privateIPConfigs: + description: Number of private IP address to attach to the interface + type: integer + publicIPConfigs: + type: integer subnetName: + description: The subnet to place the interface in type: string type: object type: array diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml index faae6ad8f2e..7b6cb7c7ec2 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremachinetemplates.yaml @@ -1099,23 +1099,20 @@ spec: description: Network Interfaces to attach to each VM properties: acceleratedNetworking: + description: Enable acccelerated networking on the interface type: boolean id: + description: Attach an already provisioned interface + by ID type: string - ipConfigs: - items: - description: IP Configuration defines options to confiure - a network interface. - properties: - privateIP: - type: string - publicIP: - type: boolean - publicIPAddress: - type: string - type: object - type: array + privateIPConfigs: + description: Number of private IP address to attach + to the interface + type: integer + publicIPConfigs: + type: integer subnetName: + description: The subnet to place the interface in type: string type: object type: array