Skip to content

🐛 Fix nodes outbound connectivity with SLB #674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k8s-ci-robot merged 1 commit into from
Jun 5, 2020
Merged

🐛 Fix nodes outbound connectivity with SLB #674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 13 additions & 10 deletions api/v1alpha3/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,23 @@ const (
// dedicated to this cluster api provider implementation.
NameAzureClusterAPIRole = NameAzureProviderPrefix + "role"

// APIServerRoleTagValue describes the value for the apiserver role
APIServerRoleTagValue = "apiserver"
// APIServerRole describes the value for the apiserver role
APIServerRole = "apiserver"

// BastionRoleTagValue describes the value for the bastion role
BastionRoleTagValue = "bastion"
// NodeOutboundRole describes the value for the node outbound LB role
NodeOutboundRole = "nodeOutbound"

// CommonRoleTagValue describes the value for the common role
CommonRoleTagValue = "common"
// BastionRole describes the value for the bastion role
BastionRole = "bastion"

// PublicRoleTagValue describes the value for the public role
PublicRoleTagValue = "public"
// CommonRole describes the value for the common role
CommonRole = "common"

// PrivateRoleTagValue describes the value for the private role
PrivateRoleTagValue = "private"
// PublicRole describes the value for the public role
PublicRole = "public"

// PrivateRole describes the value for the private role
PrivateRole = "private"
)

// ClusterTagKey generates the key for resources associated with a cluster.
Expand Down
5 changes: 5 additions & 0 deletions cloud/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ func GeneratePublicIPName(clusterName, hash string) string {
return fmt.Sprintf("%s-%s", clusterName, hash)
}

// GenerateNodeOutboundIPName generates a public IP name, based on the cluster name.
func GenerateNodeOutboundIPName(clusterName string) string {
return fmt.Sprintf("pip-%s-node-outbound", clusterName)
}

// GenerateNodePublicIPName generates a node public IP name, based on the NIC name.
func GenerateNodePublicIPName(nicName string) string {
return fmt.Sprintf("%s-public-ip", nicName)
Expand Down
2 changes: 1 addition & 1 deletion cloud/services/groups/groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
ClusterName: s.Scope.Name(),
Lifecycle: infrav1.ResourceLifecycleOwned,
Name: to.StringPtr(s.Scope.ResourceGroup()),
Role: to.StringPtr(infrav1.CommonRoleTagValue),
Role: to.StringPtr(infrav1.CommonRole),
Additional: s.Scope.AdditionalTags(),
})),
}
Expand Down
25 changes: 14 additions & 11 deletions cloud/services/networkinterfaces/networkinterfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ import (
"github.com/Azure/go-autorest/autorest/to"
"github.com/pkg/errors"
"k8s.io/klog"
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
)

// Spec specification for routetable
type Spec struct {
Name string
MachineRole string
SubnetName string
VnetName string
StaticIPAddress string
Expand Down Expand Up @@ -77,27 +79,28 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {

backendAddressPools := []network.BackendAddressPool{}
if nicSpec.PublicLoadBalancerName != "" {
// only control planes have an attached public LB
lb, lberr := s.PublicLoadBalancersClient.Get(ctx, s.Scope.ResourceGroup(), nicSpec.PublicLoadBalancerName)
if lberr != nil {
return errors.Wrap(lberr, "failed to get publicLB")
return errors.Wrap(lberr, "failed to get public LB")
}

backendAddressPools = append(backendAddressPools,
network.BackendAddressPool{
ID: (*lb.BackendAddressPools)[0].ID,
})

ruleName := s.MachineScope.Name()
naterr := s.createInboundNatRule(ctx, lb, ruleName)
if naterr != nil {
return errors.Wrap(naterr, "failed to create NAT rule")
}
if nicSpec.MachineRole == infrav1.ControlPlane {
ruleName := s.MachineScope.Name()
naterr := s.createInboundNatRule(ctx, lb, ruleName)
if naterr != nil {
return errors.Wrap(naterr, "failed to create NAT rule")
}

nicConfig.LoadBalancerInboundNatRules = &[]network.InboundNatRule{
{
ID: to.StringPtr(fmt.Sprintf("%s/inboundNatRules/%s", to.String(lb.ID), ruleName)),
},
nicConfig.LoadBalancerInboundNatRules = &[]network.InboundNatRule{
{
ID: to.StringPtr(fmt.Sprintf("%s/inboundNatRules/%s", to.String(lb.ID), ruleName)),
},
}
}
}
if nicSpec.InternalLoadBalancerName != "" {
Expand Down
115 changes: 80 additions & 35 deletions cloud/services/networkinterfaces/networkinterfaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,11 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "get subnets fails",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "failed to get subnets: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -263,9 +265,11 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "node network interface create fails",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "failed to create network interface my-net-interface in resource group my-rg: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -279,17 +283,20 @@ func TestReconcileNetworkInterface(t *testing.T) {
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").
Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})).
Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")))
},
},
{
name: "node network interface with Static private IP successfully created",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
StaticIPAddress: "1.2.3.4",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
StaticIPAddress: "1.2.3.4",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -302,15 +309,18 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku.EXPECT().HasAcceleratedNetworking(gomock.Any(), gomock.Any())
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})))
},
},
{
name: "node network interface with Dynamic private IP successfully created",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -323,6 +333,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku.EXPECT().HasAcceleratedNetworking(gomock.Any(), gomock.Any())
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})))
},
},
Expand All @@ -334,6 +345,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-publiclb",
InternalLoadBalancerName: "my-internal-lb",
MachineRole: infrav1.ControlPlane,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand Down Expand Up @@ -413,8 +425,9 @@ func TestReconcileNetworkInterface(t *testing.T) {
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-publiclb",
InternalLoadBalancerName: "my-internal-lb",
MachineRole: infrav1.ControlPlane,
},
expectedError: "failed to get publicLB: #: Internal Server Error: StatusCode=500",
expectedError: "failed to get public LB: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
mSubnet *mock_subnets.MockClientMockRecorder,
mPublicLoadBalancer *mock_publicloadbalancers.MockClientMockRecorder,
Expand All @@ -436,6 +449,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-publiclb",
InternalLoadBalancerName: "my-internal-lb",
MachineRole: infrav1.ControlPlane,
},
expectedError: "failed to create NAT rule: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand Down Expand Up @@ -501,6 +515,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
VnetName: "my-vnet",
SubnetName: "my-subnet",
InternalLoadBalancerName: "my-internal-lb",
MachineRole: infrav1.ControlPlane,
},
expectedError: "failed to get internalLB: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -519,10 +534,12 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "network interface with Public IP successfully created",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicIPName: "my-public-ip",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicIPName: "my-public-ip",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -535,6 +552,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku.EXPECT().HasAcceleratedNetworking(gomock.Any(), gomock.Any())
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
mPublicIP.CreateOrUpdate(context.TODO(), "my-rg", "my-public-ip", gomock.AssignableToTypeOf(network.PublicIPAddress{})),
mPublicIP.Get(context.TODO(), "my-rg", "my-public-ip").Return(network.PublicIPAddress{}, nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})))
Expand All @@ -543,10 +561,12 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "network interface with Public IP fail to get Public IP",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicIPName: "my-public-ip",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicIPName: "my-public-ip",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "failed to get publicIP: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -558,17 +578,19 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku *mock_resourceskus.MockClient) {
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
mPublicIP.CreateOrUpdate(context.TODO(), "my-rg", "my-public-ip", gomock.AssignableToTypeOf(network.PublicIPAddress{})),
mPublicIP.Get(context.TODO(), "my-rg", "my-public-ip").Return(network.PublicIPAddress{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})))
mPublicIP.Get(context.TODO(), "my-rg", "my-public-ip").Return(network.PublicIPAddress{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")))
},
},
{
name: "network interface with accelerated networking successfully created",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -581,6 +603,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku.EXPECT().HasAcceleratedNetworking(context.TODO(), gomock.Any()).Return(true, nil)
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", matchers.DiffEq(network.Interface{
Location: to.StringPtr("test-location"),
InterfacePropertiesFormat: &network.InterfacePropertiesFormat{
Expand All @@ -591,7 +614,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{
Subnet: &network.Subnet{},
PrivateIPAllocationMethod: network.Dynamic,
LoadBalancerBackendAddressPools: &[]network.BackendAddressPool{},
LoadBalancerBackendAddressPools: &[]network.BackendAddressPool{{ID: to.StringPtr("cluster-name-outboundBackendPool")}},
},
},
},
Expand All @@ -603,9 +626,11 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "network interface without accelerated networking successfully created",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -618,6 +643,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
mResourceSku.EXPECT().HasAcceleratedNetworking(context.TODO(), gomock.Any()).Return(false, nil)
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", matchers.DiffEq(network.Interface{
Location: to.StringPtr("test-location"),
InterfacePropertiesFormat: &network.InterfacePropertiesFormat{
Expand All @@ -628,7 +654,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{
Subnet: &network.Subnet{},
PrivateIPAllocationMethod: network.Dynamic,
LoadBalancerBackendAddressPools: &[]network.BackendAddressPool{},
LoadBalancerBackendAddressPools: &[]network.BackendAddressPool{{ID: to.StringPtr("cluster-name-outboundBackendPool")}},
},
},
},
Expand All @@ -640,9 +666,11 @@ func TestReconcileNetworkInterface(t *testing.T) {
{
name: "network interface fails to get accelerated networking capability",
netInterfaceSpec: Spec{
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
Name: "my-net-interface",
VnetName: "my-vnet",
SubnetName: "my-subnet",
PublicLoadBalancerName: "my-cluster",
MachineRole: infrav1.Node,
},
expectedError: "failed to get accelerated networking capability: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_networkinterfaces.MockClientMockRecorder,
Expand All @@ -656,6 +684,7 @@ func TestReconcileNetworkInterface(t *testing.T) {
false, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
gomock.InOrder(
mSubnet.Get(context.TODO(), "my-rg", "my-vnet", "my-subnet").Return(network.Subnet{}, nil),
mPublicLoadBalancer.Get(context.TODO(), "my-rg", "my-cluster").Return(getFakeNodeOutboundLoadBalancer(), nil),
m.CreateOrUpdate(context.TODO(), "my-rg", "my-net-interface", gomock.AssignableToTypeOf(network.Interface{})),
)
},
Expand Down Expand Up @@ -953,3 +982,19 @@ func TestDeleteNetworkInterface(t *testing.T) {
})
}
}

func getFakeNodeOutboundLoadBalancer() network.LoadBalancer {
return network.LoadBalancer{
LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
{
ID: to.StringPtr("frontend-ip-config-id"),
},
},
BackendAddressPools: &[]network.BackendAddressPool{
{
ID: pointer.StringPtr("cluster-name-outboundBackendPool"),
},
},
}}
}
Loading