diff --git a/pkg/driver/executor/executor.go b/pkg/driver/executor/executor.go index e75f99ed..f0393fb3 100644 --- a/pkg/driver/executor/executor.go +++ b/pkg/driver/executor/executor.go @@ -11,18 +11,19 @@ import ( "strings" "time" - "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/cloudprovider" - api "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/openstack" - "github.com/gardener/machine-controller-manager-provider-openstack/pkg/client" - "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/bootfromvolume" "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/keypairs" "github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/schedulerhints" "github.com/gophercloud/gophercloud/openstack/compute/v2/servers" "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog" "k8s.io/utils/pointer" + + "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/cloudprovider" + api "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/openstack" + "github.com/gardener/machine-controller-manager-provider-openstack/pkg/client" ) // Executor concretely handles the execution of requests to the machine controller. Executor is responsible @@ -57,7 +58,7 @@ func NewExecutor(factory *client.Factory, config *api.MachineProviderConfig) (*E // CreateMachine creates a new OpenStack server instance and waits until it reports "ACTIVE". // If there is an error during the build process, or if the building phase timeouts, it will delete any artifacts created. func (ex *Executor) CreateMachine(ctx context.Context, machineName string, userData []byte) (string, error) { - serverNetworks, podNetworkIDs, err := ex.resolveServerNetworks(machineName) + serverNetworks, err := ex.resolveServerNetworks(machineName) if err != nil { return "", fmt.Errorf("failed to resolve server networks: %w", err) } @@ -83,46 +84,54 @@ func (ex *Executor) CreateMachine(ctx context.Context, machineName string, userD return "", deleteOnFail(fmt.Errorf("error waiting for server [ID=%q] to reach target status: %w", server.ID, err)) } - if err := ex.patchServerPortsForPodNetwork(server.ID, podNetworkIDs); err != nil { + if err := ex.patchServerPortsForPodNetwork(server.ID); err != nil { return "", deleteOnFail(fmt.Errorf("failed to patch server [ID=%q] ports: %s", server.ID, err)) } return providerID, nil } -// resolveServerNetworks resolves the network configuration for a server. -// It returns a list of networks that the server should be part of and a map of Network IDs that are part of the Pod Network. -func (ex *Executor) resolveServerNetworks(machineName string) ([]servers.Network, map[string]struct{}, error) { +// resolveServerNetworks resolves the network configuration for the server. +func (ex *Executor) resolveServerNetworks(machineName string) ([]servers.Network, error) { var ( networkID = ex.Config.Spec.NetworkID subnetID = ex.Config.Spec.SubnetID networks = ex.Config.Spec.Networks serverNetworks = make([]servers.Network, 0) - podNetworkIDs = make(map[string]struct{}) ) klog.V(3).Infof("resolving network setup for machine %q", machineName) // If NetworkID is specified in the spec, we deploy the VMs in an existing Network. // If SubnetID is specified in addition to NetworkID, we have to preallocate a Neutron Port to force the VMs to get IP from the subnet's range. - if !isEmptyString(pointer.StringPtr(networkID)) { - klog.V(3).Infof("deploying in existing network [ID=%q]", networkID) - if isEmptyString(ex.Config.Spec.SubnetID) { - // if no SubnetID is specified, use only the NetworkID for the network attachments. - serverNetworks = append(serverNetworks, servers.Network{UUID: ex.Config.Spec.NetworkID}) - } else { - klog.V(3).Infof("deploying in existing subnet [ID=%q]. Pre-allocating Neutron Port... ", *subnetID) - if _, err := ex.Network.GetSubnet(*subnetID); err != nil { - return nil, nil, err - } + if !isEmptyString(pointer.StringPtr(networkID)) && !isEmptyString(ex.Config.Spec.SubnetID) { + // check if the subnet exists + if _, err := ex.Network.GetSubnet(*subnetID); err != nil { + return nil, err + } - var securityGroupIDs []string - for _, securityGroup := range ex.Config.Spec.SecurityGroups { - securityGroupID, err := ex.Network.GroupIDFromName(securityGroup) - if err != nil { - return nil, nil, err - } - securityGroupIDs = append(securityGroupIDs, securityGroupID) + klog.V(3).Infof("deploying in subnet [ID=%q]", *subnetID) + + var ( + portID string + err error + securityGroupIDs []string + ) + + for _, securityGroup := range ex.Config.Spec.SecurityGroups { + securityGroupID, err := ex.Network.GroupIDFromName(securityGroup) + if err != nil { + return nil, err } + securityGroupIDs = append(securityGroupIDs, securityGroupID) + } + portID, err = ex.Network.PortIDFromName(machineName) + if err != nil && !client.IsNotFoundError(err) { + return nil, fmt.Errorf("error fetching port with name %q: %s", machineName, err) + } + + if client.IsNotFoundError(err) { + klog.V(3).Infof("failed to find port [Name=%q]", machineName) + klog.V(3).Infof("creating port [Name=%q]... ", machineName) port, err := ex.Network.CreatePort(&ports.CreateOpts{ Name: machineName, NetworkID: ex.Config.Spec.NetworkID, @@ -131,33 +140,41 @@ func (ex *Executor) resolveServerNetworks(machineName string) ([]servers.Network SecurityGroups: &securityGroupIDs, }) if err != nil { - return nil, nil, err + return nil, err } - klog.V(3).Infof("port [ID=%q] successfully created", port.ID) - serverNetworks = append(serverNetworks, servers.Network{UUID: ex.Config.Spec.NetworkID, Port: port.ID}) + + klog.V(3).Infof("port [Name=%q] successfully created", port.Name) + portID = port.ID + } else { + klog.V(3).Infof("found port [Name=%q] skipping creation", machineName) } - podNetworkIDs[networkID] = struct{}{} - } else { - for _, network := range networks { - var ( - resolvedNetworkID string - err error - ) - if isEmptyString(pointer.StringPtr(network.Id)) { - resolvedNetworkID, err = ex.Network.NetworkIDFromName(network.Name) - if err != nil { - return nil, nil, err - } - } else { - resolvedNetworkID = network.Id - } - serverNetworks = append(serverNetworks, servers.Network{UUID: resolvedNetworkID}) - if network.PodNetwork { - podNetworkIDs[resolvedNetworkID] = struct{}{} + + serverNetworks = append(serverNetworks, servers.Network{UUID: ex.Config.Spec.NetworkID, Port: portID}) + return serverNetworks, nil + } + + if !isEmptyString(pointer.StringPtr(networkID)) { + klog.V(3).Infof("deploying in network [ID=%q]", networkID) + serverNetworks = append(serverNetworks, servers.Network{UUID: ex.Config.Spec.NetworkID}) + return serverNetworks, nil + } + + for _, network := range networks { + var ( + resolvedNetworkID string + err error + ) + if isEmptyString(pointer.StringPtr(network.Id)) { + resolvedNetworkID, err = ex.Network.NetworkIDFromName(network.Name) + if err != nil { + return nil, err } + } else { + resolvedNetworkID = network.Id } + serverNetworks = append(serverNetworks, servers.Network{UUID: resolvedNetworkID}) } - return serverNetworks, podNetworkIDs, nil + return serverNetworks, nil } // waitForStatus blocks until the server with the specified ID reaches one of the target status. @@ -282,7 +299,7 @@ func resourceInstanceBlockDevicesV2(rootDiskSize int, imageID string) ([]bootfro } // patchServerPortsForPodNetwork updates a server's ports with rules for whitelisting the pod network CIDR. -func (ex *Executor) patchServerPortsForPodNetwork(serverID string, podNetworkIDs map[string]struct{}) error { +func (ex *Executor) patchServerPortsForPodNetwork(serverID string) error { allPorts, err := ex.Network.ListPorts(&ports.ListOpts{ DeviceID: serverID, }) @@ -294,20 +311,71 @@ func (ex *Executor) patchServerPortsForPodNetwork(serverID string, podNetworkIDs return fmt.Errorf("got an empty port list for server %q", serverID) } + podNetworkIDs, err := ex.resolveNetworkIDsForPodNetwork() + if err != nil { + return fmt.Errorf("failed to resolve network IDs for the pod network %v", err) + } + for _, port := range allPorts { - for id := range podNetworkIDs { - if port.NetworkID == id { - if err := ex.Network.UpdatePort(port.ID, ports.UpdateOpts{ - AllowedAddressPairs: &[]ports.AddressPair{{IPAddress: ex.Config.Spec.PodNetworkCidr}}, - }); err != nil { - return fmt.Errorf("failed to update allowed address pair for port [ID=%q]: %v", port.ID, err) + if podNetworkIDs.Has(port.NetworkID) { + addressPairFound := false + + for _, pair := range port.AllowedAddressPairs { + if pair.IPAddress == ex.Config.Spec.PodNetworkCidr { + klog.V(3).Infof("port [ID=%q] already allows pod network CIDR range. Skipping update...", port.ID) + addressPairFound = true + // break inner loop if target found + break } } + // continue outer loop if target found + if addressPairFound { + continue + } + + if err := ex.Network.UpdatePort(port.ID, ports.UpdateOpts{ + AllowedAddressPairs: &[]ports.AddressPair{{IPAddress: ex.Config.Spec.PodNetworkCidr}}, + }); err != nil { + return fmt.Errorf("failed to update allowed address pair for port [ID=%q]: %v", port.ID, err) + } } } return nil } +// resolveNetworkIDsForPodNetwork resolves the networks that accept traffic from the pod CIDR range. +func (ex *Executor) resolveNetworkIDsForPodNetwork() (sets.String, error) { + var ( + networkID = ex.Config.Spec.NetworkID + networks = ex.Config.Spec.Networks + podNetworkIDs = sets.NewString() + ) + + if !isEmptyString(pointer.StringPtr(networkID)) { + podNetworkIDs.Insert(networkID) + return podNetworkIDs, nil + } + + for _, network := range networks { + var ( + resolvedNetworkID string + err error + ) + if isEmptyString(pointer.StringPtr(network.Id)) { + resolvedNetworkID, err = ex.Network.NetworkIDFromName(network.Name) + if err != nil { + return nil, err + } + } else { + resolvedNetworkID = network.Id + } + if network.PodNetwork { + podNetworkIDs.Insert(resolvedNetworkID) + } + } + return podNetworkIDs, nil +} + // DeleteMachine deletes a server based on the supplied ID or name. The machine must have the cluster/role tags for any operation to take place. // If providerID is specified it takes priority over the machineName. If no providerID is specified, DeleteMachine will // try to resolve the machineName to an appropriate server ID. @@ -465,6 +533,15 @@ func (ex *Executor) GetMachineStatus(ctx context.Context, machineName string) (s return "", err } + // Patch the server ports to allow pod network cidr + // This is a workaround in case the pod restarts between the server creation on openstack side and the patching + // of the ports during CreateMachine. + // Currently there is no way to signal that a machine is unhealthy after creation, so repeat the steps. + err = ex.patchServerPortsForPodNetwork(server.ID) + if err != nil { + return "", err + } + return EncodeProviderID(ex.Config.Spec.Region, server.ID), nil } diff --git a/pkg/driver/executor/executor_test.go b/pkg/driver/executor/executor_test.go index b577bc9a..3a6ec0bb 100644 --- a/pkg/driver/executor/executor_test.go +++ b/pkg/driver/executor/executor_test.go @@ -10,8 +10,10 @@ import ( "fmt" "github.com/golang/mock/gomock" + "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/compute/v2/servers" "github.com/gophercloud/gophercloud/openstack/networking/v2/ports" + "github.com/gophercloud/gophercloud/openstack/networking/v2/subnets" . "github.com/onsi/ginkgo" "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" @@ -19,14 +21,15 @@ import ( "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/cloudprovider" "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/openstack" - client "github.com/gardener/machine-controller-manager-provider-openstack/pkg/client" + "github.com/gardener/machine-controller-manager-provider-openstack/pkg/client" . "github.com/gardener/machine-controller-manager-provider-openstack/pkg/driver/executor" mocks "github.com/gardener/machine-controller-manager-provider-openstack/pkg/mock/openstack" ) var _ = Describe("Executor", func() { const ( - region = "eu-nl-1" + region = "eu-nl-1" + networkID = "networkID" ) var ( ctrl *gomock.Controller @@ -50,8 +53,9 @@ var _ = Describe("Executor", func() { cfg = &openstack.MachineProviderConfig{ Spec: openstack.MachineProviderConfigSpec{ - Tags: tags, - Region: region, + Tags: tags, + Region: region, + NetworkID: networkID, }, } }) @@ -119,6 +123,47 @@ var _ = Describe("Executor", func() { Expect(providerId).To(Equal(EncodeProviderID(region, serverID))) }) + It("should succeed when spec contains subnet", func() { + var ( + subnetID = "subnetID" + ) + + cfg.Spec.SubnetID = &subnetID + ex := &Executor{ + Compute: compute, + Network: network, + Config: cfg, + } + + network.EXPECT().GetSubnet(subnetID).Return(&subnets.Subnet{}, nil) + network.EXPECT().PortIDFromName(machineName).Return("", gophercloud.ErrResourceNotFound{}) + network.EXPECT().CreatePort(gomock.Any()).Return(&ports.Port{ID: portID, Name: machineName}, nil) + compute.EXPECT().ImageIDFromName(imageName).Return("imageID", nil) + compute.EXPECT().FlavorIDFromName(flavorName).Return("flavorID", nil) + compute.EXPECT().CreateServer(gomock.Any()).Return(&servers.Server{ + ID: serverID, + }, nil) + gomock.InOrder( + compute.EXPECT().GetServer(serverID).Return(&servers.Server{ + ID: serverID, + Status: client.ServerStatusBuild, + }, nil), + compute.EXPECT().GetServer(serverID).Return(&servers.Server{ + ID: serverID, + Status: client.ServerStatusActive, + }, nil)) + network.EXPECT().ListPorts(&ports.ListOpts{ + DeviceID: serverID, + }).Return([]ports.Port{{NetworkID: networkID, ID: portID}}, nil) + network.EXPECT().UpdatePort(portID, ports.UpdateOpts{ + AllowedAddressPairs: &[]ports.AddressPair{{IPAddress: podCidr}}, + }).Return(nil) + + providerId, err := ex.CreateMachine(ctx, machineName, nil) + Expect(err).To(BeNil()) + Expect(providerId).To(Equal(EncodeProviderID(region, serverID))) + }) + It("should delete the server on failure", func() { ex := &Executor{ Compute: compute, @@ -149,6 +194,10 @@ var _ = Describe("Executor", func() { _, err := ex.CreateMachine(ctx, machineName, nil) Expect(err).NotTo(BeNil()) }) + + Context("#User-managed network", func() { + It("should create the port if it does not exist", func() {}) + }) }) Context("List", func() { @@ -191,6 +240,7 @@ var _ = Describe("Executor", func() { Context("#GetMachineStatus", func() { var ( serverList []servers.Server + portID = "portID" ) BeforeEach(func() { @@ -228,6 +278,13 @@ var _ = Describe("Executor", func() { table.DescribeTable("#Status", func(name string, expectedID string, expectedErr error) { compute.EXPECT().ListServers(&servers.ListOpts{Name: name}).Return(serverList, nil) + if expectedErr == nil { + network.EXPECT().ListPorts(&ports.ListOpts{ + DeviceID: expectedID, + }).Return([]ports.Port{{NetworkID: networkID, ID: portID}}, nil) + network.EXPECT().UpdatePort(portID, gomock.Any()).Return(nil) + } + ex := Executor{ Compute: compute, Network: network, @@ -247,6 +304,30 @@ var _ = Describe("Executor", func() { table.Entry("Should return not found if name exists without matching metadata", "baz", "", ErrNotFound), table.Entry("Should detect multiple matching servers", "lorem", "", ErrMultipleFound), ) + + It("should not try to patch ports with correct addressPairs", func() { + var ( + podCidr = "10.0.0.0/16" + server = serverList[0] + ) + compute.EXPECT().ListServers(&servers.ListOpts{Name: server.Name}).Return(serverList, nil) + network.EXPECT().ListPorts(&ports.ListOpts{ + DeviceID: server.ID, + }).Return([]ports.Port{ + {NetworkID: "foo", ID: "foo"}, + {NetworkID: "bar", ID: "bar"}, + {NetworkID: networkID, ID: portID, AllowedAddressPairs: []ports.AddressPair{{IPAddress: podCidr}}}, + }, nil) + + cfg.Spec.PodNetworkCidr = podCidr + ex := Executor{ + Compute: compute, + Network: network, + Config: cfg, + } + _, err := ex.GetMachineStatus(ctx, server.Name) + Expect(err).To(BeNil()) + }) }) Context("Delete", func() {