Skip to content

UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly #35

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
openshift-merge-robot merged 1 commit into from
Jun 6, 2019
Merged

UPSTREAM: <carry>: openshift: Set instance subnet and load balancers explicitly #35

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
12 changes: 12 additions & 0 deletions pkg/apis/azureprovider/v1alpha1/azuremachineproviderconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ type AzureMachineProviderSpec struct {
SSHPublicKey string `json:"sshPublicKey"`
SSHPrivateKey string `json:"sshPrivateKey"`
PublicIP bool `json:"publicIP"`

// Subnet to use for this instance
Subnet string `json:"subnet"`

// PublicLoadBalancer to use for this instance
PublicLoadBalancer string `json:"publicLoadBalancer"`

// InternalLoadBalancerName to use for this instance
InternalLoadBalancer string `json:"internalLoadBalancer"`

// NatRule to set inbound NAT rule of the load balancer
NatRule *int `json:"natRule"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
4 changes: 3 additions & 1 deletion pkg/cloud/azure/actuators/machine/actuator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ func newFakeScope(t *testing.T, label string) *actuators.MachineScope {
Scope: scope,
Machine: m,
MachineClient: c.Machines("dummyNamespace"),
MachineConfig: &v1alpha1.AzureMachineProviderSpec{},
MachineConfig: &v1alpha1.AzureMachineProviderSpec{
Subnet: "dummySubnet",
},
MachineStatus: &v1alpha1.AzureMachineProviderStatus{},
}
}
Expand Down
25 changes: 15 additions & 10 deletions pkg/cloud/azure/actuators/machine/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,16 +549,21 @@ func (s *Reconciler) createNetworkInterface(ctx context.Context, nicName string)
Name: nicName,
VnetName: azure.GenerateVnetName(s.scope.Cluster.Name),
}
switch set := s.scope.Machine.ObjectMeta.Labels[v1alpha1.MachineRoleLabel]; set {
Copy link

@awesomenix awesomenix May 31, 2019

Choose a reason for hiding this comment

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

If the Subnet is empty you could populate the hardcoded ones above just for backward compat.

Something like

if subnet == "" {
} else {
//fallback
}

Eventually the fallback code can be removed

case v1alpha1.Node:
networkInterfaceSpec.SubnetName = azure.GenerateNodeSubnetName(s.scope.Cluster.Name)
case v1alpha1.ControlPlane:
networkInterfaceSpec.SubnetName = azure.GenerateControlPlaneSubnetName(s.scope.Cluster.Name)
networkInterfaceSpec.PublicLoadBalancerName = azure.GeneratePublicLBName(s.scope.Cluster.Name)
Copy link

@awesomenix awesomenix May 31, 2019

Choose a reason for hiding this comment

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

These names are must have, since control plane requires public and internal load balancer for access into the cluster.

its an unfortunate design hope to change it in future. The way it works now is

CreateControlPlaneVM -> CreateNetworkInterface -> AttachInterfacetoloadblancer

Instead hope to work this way

CreateControlPlaneVM -> CreateNetworkInterface
UpdateLoadBalancerRules with NetworkInterface

That way control plane is decoupled from load balancers.

Copy link
Member Author

@ingvagabund ingvagabund May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So you still have a use for the control plane code? How does the installer reuses the azure actuator these days? E.g. what resources are create by the azure actuator and which by the installer?

Copy link

@awesomenix awesomenix May 31, 2019

Choose a reason for hiding this comment

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

azure actuator only creates vm and sets up configuration of the base infra (example registering network interface with the load balancers)

as for worker nodes it just creates it on the base infra (right now all the names are hardcoded between installer and actuator hence little fragile), aim was to get it decoupled if we had time.

Copy link

@awesomenix awesomenix May 31, 2019

Choose a reason for hiding this comment

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

Also azure will never have name conflicts so its ok to hardcode since all the resources are restricted to a certain resource group for each sub/tenantid

networkInterfaceSpec.InternalLoadBalancerName = azure.GenerateInternalLBName(s.scope.Cluster.Name)
networkInterfaceSpec.NatRule = 0
default:
return errors.Errorf("unknown value %s for label `set` on machine %s, skipping machine creation", set, s.scope.Machine.Name)

if s.scope.MachineConfig.Subnet == "" {
return errors.Errorf("MachineConfig subnet is missing on machine %s, skipping machine creation", s.scope.Machine.Name)
}

networkInterfaceSpec.SubnetName = s.scope.MachineConfig.Subnet

if s.scope.MachineConfig.PublicLoadBalancer != "" {
networkInterfaceSpec.PublicLoadBalancerName = s.scope.MachineConfig.PublicLoadBalancer
if s.scope.MachineConfig.NatRule != nil {
networkInterfaceSpec.NatRule = s.scope.MachineConfig.NatRule
}
}
if s.scope.MachineConfig.InternalLoadBalancer != "" {
networkInterfaceSpec.InternalLoadBalancerName = s.scope.MachineConfig.InternalLoadBalancer
}

if s.scope.MachineConfig.PublicIP {
Expand Down
12 changes: 7 additions & 5 deletions pkg/cloud/azure/services/networkinterfaces/networkinterfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type Spec struct {
StaticIPAddress string
PublicLoadBalancerName string
InternalLoadBalancerName string
NatRule int
NatRule *int
PublicIP string
}

Expand Down Expand Up @@ -112,10 +112,12 @@ func (s *Service) CreateOrUpdate(ctx context.Context, spec azure.Spec) error {
network.BackendAddressPool{
ID: (*lb.BackendAddressPools)[0].ID,
})
nicConfig.LoadBalancerInboundNatRules = &[]network.InboundNatRule{
{
ID: (*lb.InboundNatRules)[nicSpec.NatRule].ID,
},
if nicSpec.NatRule != nil {
nicConfig.LoadBalancerInboundNatRules = &[]network.InboundNatRule{
{
ID: (*lb.InboundNatRules)[*nicSpec.NatRule].ID,
},
}
}
}
if nicSpec.InternalLoadBalancerName != "" {
Expand Down