From ece038f0ed12579a7196c010b93fa8de0d1e155f Mon Sep 17 00:00:00 2001 From: Emilio Garcia Date: Wed, 31 Mar 2021 13:25:24 -0400 Subject: [PATCH 1/4] Device Owner and Device ID unsafe These fields need to be set by OpenStack, otherwise there is the potential to create severe errors. They also require admin/owner permission to set. They should not be exposed to end users at all. --- pkg/apis/openstackproviderconfig/v1alpha1/types.go | 2 -- pkg/cloud/openstack/clients/machineservice.go | 2 -- 2 files changed, 4 deletions(-) diff --git a/pkg/apis/openstackproviderconfig/v1alpha1/types.go b/pkg/apis/openstackproviderconfig/v1alpha1/types.go index a7ac9a3fa8..7ffc7fffe2 100644 --- a/pkg/apis/openstackproviderconfig/v1alpha1/types.go +++ b/pkg/apis/openstackproviderconfig/v1alpha1/types.go @@ -206,8 +206,6 @@ type PortOpts struct { AdminStateUp *bool `json:"adminStateUp,omitempty"` MACAddress string `json:"macAddress,omitempty"` FixedIPs []ports.IP `json:"fixedIPs,omitempty"` - DeviceID string `json:"deviceID,omitempty"` - DeviceOwner string `json:"deviceOwner,omitempty"` TenantID string `json:"tenantID,omitempty"` ProjectID string `json:"projectID,omitempty"` SecurityGroups *[]string `json:"securityGroups,omitempty"` diff --git a/pkg/cloud/openstack/clients/machineservice.go b/pkg/cloud/openstack/clients/machineservice.go index 08d8509461..41d7428868 100644 --- a/pkg/cloud/openstack/clients/machineservice.go +++ b/pkg/cloud/openstack/clients/machineservice.go @@ -339,8 +339,6 @@ func getOrCreatePort(is *InstanceService, name string, portOpts openstackconfigv Description: portOpts.Description, AdminStateUp: portOpts.AdminStateUp, MACAddress: portOpts.MACAddress, - DeviceID: portOpts.DeviceID, - DeviceOwner: portOpts.DeviceOwner, TenantID: portOpts.TenantID, ProjectID: portOpts.ProjectID, SecurityGroups: portOpts.SecurityGroups, From a277eace8d109678f3bee963f82eb4bd4cf996c9 Mon Sep 17 00:00:00 2001 From: Emilio Garcia Date: Wed, 31 Mar 2021 14:56:26 -0400 Subject: [PATCH 2/4] Unify Port Creation Logic There is duplicate logic on how ports for machine interfaces are created. This converts all portCreate calls to use a single private function to create ports and uses the same process for each of them. --- pkg/cloud/openstack/clients/machineservice.go | 96 +++++-------------- 1 file changed, 24 insertions(+), 72 deletions(-) diff --git a/pkg/cloud/openstack/clients/machineservice.go b/pkg/cloud/openstack/clients/machineservice.go index 41d7428868..37a12e68ad 100644 --- a/pkg/cloud/openstack/clients/machineservice.go +++ b/pkg/cloud/openstack/clients/machineservice.go @@ -390,27 +390,6 @@ func listPorts(is *InstanceService, opts ports.ListOpts) ([]ports.Port, error) { return portList, nil } -func CreatePort(is *InstanceService, name string, net ServerNetwork, securityGroups *[]string, allowedAddressPairs *[]ports.AddressPair) (ports.Port, error) { - portCreateOpts := ports.CreateOpts{ - Name: name, - NetworkID: net.networkID, - SecurityGroups: securityGroups, - AllowedAddressPairs: *allowedAddressPairs, - } - if net.subnetID != "" { - portCreateOpts.FixedIPs = []ports.IP{{SubnetID: net.subnetID}} - } - createOps := portsbinding.CreateOptsExt{ - CreateOptsBuilder: portCreateOpts, - VNICType: net.vnicType, - } - newPort, err := ports.Create(is.networkClient, createOps).Extract() - if err != nil { - return ports.Port{}, fmt.Errorf("Create port for server err: %v", err) - } - return *newPort, nil -} - func isDuplicate(list []string, name string) bool { if list == nil || len(list) == 0 { return false @@ -486,7 +465,7 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust return nil, err } // Get all network UUIDs - var nets []ServerNetwork + var nets []openstackconfigv1.PortOpts netsWithoutAllowedAddressPairs := map[string]struct{}{} for _, net := range config.Networks { opts := networks.ListOpts(net.Filter) @@ -500,11 +479,11 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust netsWithoutAllowedAddressPairs[netID] = struct{}{} } if net.Subnets == nil { - nets = append(nets, ServerNetwork{ - networkID: netID, - portTags: net.PortTags, - vnicType: net.VNICType, - portSecurity: net.PortSecurity, + nets = append(nets, openstackconfigv1.PortOpts{ + NetworkID: netID, + Tags: net.PortTags, + VNICType: net.VNICType, + PortSecurity: net.PortSecurity, }) } @@ -524,12 +503,12 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust return nil, err } for _, snet := range snetResults { - nets = append(nets, ServerNetwork{ - networkID: snet.NetworkID, - subnetID: snet.ID, - portTags: append(net.PortTags, snetParam.PortTags...), - vnicType: net.VNICType, - portSecurity: portSecurity, + nets = append(nets, openstackconfigv1.PortOpts{ + NetworkID: snet.NetworkID, + FixedIPs: []ports.IP{{SubnetID: snet.ID}}, + Tags: append(net.PortTags, snetParam.PortTags...), + VNICType: net.VNICType, + PortSecurity: portSecurity, }) } } @@ -561,53 +540,26 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust userData := base64.StdEncoding.EncodeToString([]byte(cmd)) var portsList []servers.Network - for _, net := range nets { - if net.networkID == "" { + for _, portOpt := range nets { + if portOpt.NetworkID == "" { return nil, fmt.Errorf("No network was found or provided. Please check your machine configuration and try again") } - allPages, err := ports.List(is.networkClient, ports.ListOpts{ - Name: name, - NetworkID: net.networkID, - }).AllPages() - if err != nil { - return nil, fmt.Errorf("Searching for existing port for server err: %v", err) - } - portList, err := ports.ExtractPorts(allPages) - if err != nil { - return nil, fmt.Errorf("Searching for existing port for server err: %v", err) + portOpt.SecurityGroups = &securityGroups + portOpt.AllowedAddressPairs = allowedAddressPairs + if portOpt.PortSecurity != nil && *portOpt.PortSecurity == false { + portOpt.SecurityGroups = &[]string{} + portOpt.AllowedAddressPairs = []ports.AddressPair{} } - var port ports.Port - if len(portList) == 0 { - // create server port - secGroups := &securityGroups - addrPairs := &allowedAddressPairs - if net.portSecurity != nil && *net.portSecurity == false { - secGroups = &[]string{} - addrPairs = &[]ports.AddressPair{} - } - if _, ok := netsWithoutAllowedAddressPairs[net.networkID]; ok { - addrPairs = &[]ports.AddressPair{} - } - port, err = CreatePort(is, name, net, secGroups, addrPairs) - if err != nil { - return nil, fmt.Errorf("Failed to create port err: %v", err) - } - } else { - port = portList[0] + if _, ok := netsWithoutAllowedAddressPairs[portOpt.NetworkID]; ok { + portOpt.AllowedAddressPairs = []ports.AddressPair{} } - // Update the port with the correct port security settings - // TODO(egarcia): figure out if possible to make this part of the prior create and update api calls - updateOpts := portsecurity.PortUpdateOptsExt{ - UpdateOptsBuilder: ports.UpdateOpts{}, - PortSecurityEnabled: net.portSecurity, - } - err = ports.Update(is.networkClient, port.ID, updateOpts).ExtractInto(&portWithPortSecurityExtensions) + port, err := getOrCreatePort(is, name, portOpt) if err != nil { - return nil, fmt.Errorf("Failed to update port security on port %s: %v", port.ID, err) + return nil, fmt.Errorf("Failed to create port err: %v", err) } - portTags := deduplicateList(append(machineTags, net.portTags...)) + portTags := deduplicateList(append(machineTags, portOpt.Tags...)) _, err = attributestags.ReplaceAll(is.networkClient, "ports", port.ID, attributestags.ReplaceAllOpts{ Tags: portTags}).Extract() if err != nil { From 223c5deb07ab205f32cd47c8d79128c633f7daaf Mon Sep 17 00:00:00 2001 From: Emilio Garcia Date: Tue, 13 Apr 2021 13:27:27 -0400 Subject: [PATCH 3/4] Fix Port Security feature Allowed address pairs and security groups are now removed from the port when the port security is disabled. This ensures that no nova errors occur since this is a hard requirement of diabling port security. --- pkg/cloud/openstack/clients/machineservice.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/cloud/openstack/clients/machineservice.go b/pkg/cloud/openstack/clients/machineservice.go index 37a12e68ad..9cfbf6868d 100644 --- a/pkg/cloud/openstack/clients/machineservice.go +++ b/pkg/cloud/openstack/clients/machineservice.go @@ -357,9 +357,14 @@ func getOrCreatePort(is *InstanceService, name string, portOpts openstackconfigv return nil, err } - if portOpts.PortSecurity != nil && *portOpts.PortSecurity == false { + if portOpts.PortSecurity != nil { + portUpdateOpts := ports.UpdateOpts{} + if *portOpts.PortSecurity == false { + portUpdateOpts.SecurityGroups = &[]string{} + portUpdateOpts.AllowedAddressPairs = &[]ports.AddressPair{} + } updateOpts := portsecurity.PortUpdateOptsExt{ - UpdateOptsBuilder: ports.UpdateOpts{}, + UpdateOptsBuilder: portUpdateOpts, PortSecurityEnabled: portOpts.PortSecurity, } err = ports.Update(is.networkClient, newPort.ID, updateOpts).ExtractInto(&portWithPortSecurityExtensions) @@ -546,10 +551,6 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust } portOpt.SecurityGroups = &securityGroups portOpt.AllowedAddressPairs = allowedAddressPairs - if portOpt.PortSecurity != nil && *portOpt.PortSecurity == false { - portOpt.SecurityGroups = &[]string{} - portOpt.AllowedAddressPairs = []ports.AddressPair{} - } if _, ok := netsWithoutAllowedAddressPairs[portOpt.NetworkID]; ok { portOpt.AllowedAddressPairs = []ports.AddressPair{} } From 234638e337dde5adf4d16fb5668f097dbdbec634 Mon Sep 17 00:00:00 2001 From: Emilio Garcia Date: Tue, 13 Apr 2021 15:29:49 -0400 Subject: [PATCH 4/4] Port Opts YAML did not follow proper conventions The yaml for the vnic type required an argument "binding:vnicType" when it should have been "vnicType". The HostID had the same issue. We also created a struct for the FixedIPs to ensure the api remains camel case. --- pkg/apis/openstackproviderconfig/v1alpha1/types.go | 11 ++++++++--- pkg/cloud/openstack/clients/machineservice.go | 9 +++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/apis/openstackproviderconfig/v1alpha1/types.go b/pkg/apis/openstackproviderconfig/v1alpha1/types.go index 7ffc7fffe2..8ee8a66063 100644 --- a/pkg/apis/openstackproviderconfig/v1alpha1/types.go +++ b/pkg/apis/openstackproviderconfig/v1alpha1/types.go @@ -205,7 +205,7 @@ type PortOpts struct { Description string `json:"description,omitempty"` AdminStateUp *bool `json:"adminStateUp,omitempty"` MACAddress string `json:"macAddress,omitempty"` - FixedIPs []ports.IP `json:"fixedIPs,omitempty"` + FixedIPs []FixedIPs `json:"fixedIPs,omitempty"` TenantID string `json:"tenantID,omitempty"` ProjectID string `json:"projectID,omitempty"` SecurityGroups *[]string `json:"securityGroups,omitempty"` @@ -213,17 +213,22 @@ type PortOpts struct { Tags []string `json:"tags,omitempty"` // The ID of the host where the port is allocated - HostID string `json:"binding:hostID,omitempty"` + HostID string `json:"hostID,omitempty"` // The virtual network interface card (vNIC) type that is bound to the // neutron port. - VNICType string `json:"binding:vnicType,omitempty"` + VNICType string `json:"vnicType,omitempty"` // enable or disable security on a given port // incompatible with securityGroups and allowedAddressPairs PortSecurity *bool `json:"portSecurity,omitempty"` } +type FixedIPs struct { + SubnetID string `json:"subnetID"` + IPAddress string `json:"ipAddress,omitempty"` +} + type RootVolume struct { SourceType string `json:"sourceType,omitempty"` SourceUUID string `json:"sourceUUID,omitempty"` diff --git a/pkg/cloud/openstack/clients/machineservice.go b/pkg/cloud/openstack/clients/machineservice.go index 9cfbf6868d..2f7820b118 100644 --- a/pkg/cloud/openstack/clients/machineservice.go +++ b/pkg/cloud/openstack/clients/machineservice.go @@ -345,7 +345,12 @@ func getOrCreatePort(is *InstanceService, name string, portOpts openstackconfigv AllowedAddressPairs: portOpts.AllowedAddressPairs, } if len(portOpts.FixedIPs) != 0 { - createOpts.FixedIPs = portOpts.FixedIPs + fixedIPs := make([]ports.IP, len(portOpts.FixedIPs)) + for i, portOptIP := range portOpts.FixedIPs { + fixedIPs[i].SubnetID = portOptIP.SubnetID + fixedIPs[i].IPAddress = portOptIP.IPAddress + } + createOpts.FixedIPs = fixedIPs } newPort, err := ports.Create(is.networkClient, portsbinding.CreateOptsExt{ CreateOptsBuilder: createOpts, @@ -510,7 +515,7 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust for _, snet := range snetResults { nets = append(nets, openstackconfigv1.PortOpts{ NetworkID: snet.NetworkID, - FixedIPs: []ports.IP{{SubnetID: snet.ID}}, + FixedIPs: []openstackconfigv1.FixedIPs{{SubnetID: snet.ID}}, Tags: append(net.PortTags, snetParam.PortTags...), VNICType: net.VNICType, PortSecurity: portSecurity,