diff --git a/pkg/client/mock/client.go b/pkg/client/mock/client.go index 43a7fbf2..32ebe3e9 100644 --- a/pkg/client/mock/client.go +++ b/pkg/client/mock/client.go @@ -16,6 +16,8 @@ type StackitClient struct { DeleteServerFunc func(ctx context.Context, projectID, region, serverID string) error ListServersFunc func(ctx context.Context, projectID, region string, labelSelector map[string]string) ([]*client.Server, error) GetNICsFunc func(ctx context.Context, projectID, region, serverID string) ([]*client.NIC, error) + ListNICsFunc func(ctx context.Context, projectID, region, networkID string) ([]*client.NIC, error) + DeleteNICFunc func(ctx context.Context, projectID, region, networkID, nicID string) error UpdateNICFunc func(ctx context.Context, projectID, region, networkID, nicID string, allowedAddresses []string) (*client.NIC, error) } @@ -64,6 +66,20 @@ func (m *StackitClient) GetNICsForServer(ctx context.Context, projectID, region, }, nil } +func (m *StackitClient) ListNICs(ctx context.Context, projectID, region, networkID string) ([]*client.NIC, error) { + if m.ListNICsFunc != nil { + return m.ListNICsFunc(ctx, projectID, region, networkID) + } + return []*client.NIC{}, nil +} + +func (m *StackitClient) DeleteNIC(ctx context.Context, projectID, region, networkID, nicID string) error { + if m.DeleteNICFunc != nil { + return m.DeleteNICFunc(ctx, projectID, region, networkID, nicID) + } + return nil +} + func (m *StackitClient) UpdateNIC(ctx context.Context, projectID, region, networkID, nicID string, allowedAddresses []string) (*client.NIC, error) { if m.UpdateNICFunc != nil { return m.UpdateNICFunc(ctx, projectID, region, networkID, nicID, allowedAddresses) diff --git a/pkg/client/sdk.go b/pkg/client/sdk.go index dea59f27..8c294dc7 100644 --- a/pkg/client/sdk.go +++ b/pkg/client/sdk.go @@ -38,6 +38,7 @@ func NewStackitClient(serviceAccountKey string) (*SdkStackitClient, error) { var ( // ErrServerNotFound indicates the server was not found (404) ErrServerNotFound = errors.New("server not found") + ErrNicNotFound = errors.New("nic not found") ) // createIAASClient creates a new STACKIT SDK IAAS API client @@ -296,6 +297,36 @@ func (c *SdkStackitClient) GetNICsForServer(ctx context.Context, projectID, regi return nics, nil } +func (c *SdkStackitClient) ListNICs(ctx context.Context, projectID, region, networkID string) ([]*NIC, error) { + res, err := c.iaasClient.DefaultAPI.ListNics(ctx, projectID, region, networkID).Execute() + if err != nil { + return nil, fmt.Errorf("SDK ListServerNICs failed: %w", err) + } + + if res.Items == nil { + return []*NIC{}, nil + } + + nics := make([]*NIC, 0, len(res.Items)) + for i := range res.Items { + nics = append(nics, convertSDKNICtoNIC(&res.Items[i])) + } + + return nics, nil +} + +func (c *SdkStackitClient) DeleteNIC(ctx context.Context, projectID, region, networkID, nicID string) error { + err := c.iaasClient.DefaultAPI.DeleteNic(ctx, projectID, region, networkID, nicID).Execute() + if err != nil { + // Check if error is 404 Not Found - this is OK (idempotent) + if isNotFoundError(err) { + return fmt.Errorf("%w: %v", ErrNicNotFound, err) + } + return fmt.Errorf("SDK DeleteNic failed: %w", err) + } + return nil +} + func (c *SdkStackitClient) UpdateNIC(ctx context.Context, projectID, region, networkID, nicID string, allowedAddresses []string) (*NIC, error) { addresses := make([]iaas.AllowedAddressesInner, len(allowedAddresses)) @@ -337,6 +368,7 @@ func convertSDKNICtoNIC(nic *iaas.NIC) *NIC { AllowedAddresses: addresses, IPv4: nic.GetIpv4(), IPv6: nic.GetIpv6(), + Name: getStringValue(nic.Name), } } @@ -361,3 +393,10 @@ func isNotFoundError(err error) bool { } return false } + +func getStringValue(s *string) string { + if s == nil { + return "" + } + return *s +} diff --git a/pkg/client/stackit.go b/pkg/client/stackit.go index 41b4810a..09c2adf5 100644 --- a/pkg/client/stackit.go +++ b/pkg/client/stackit.go @@ -25,6 +25,10 @@ type StackitClient interface { ListServers(ctx context.Context, projectID, region string, labelSelector map[string]string) ([]*Server, error) // GetNICsForServer retrieves a network interfaces for a given server GetNICsForServer(ctx context.Context, projectID, region, serverID string) ([]*NIC, error) + // ListNICs list all nics for a network + ListNICs(ctx context.Context, projectID, region, networkID string) ([]*NIC, error) + // DeleteNIC delete a given nic by ID + DeleteNIC(ctx context.Context, projectID, region, networkID, nicID string) error // UpdateNIC updates a network interface UpdateNIC(ctx context.Context, projectID, region, networkID, nicID string, allowedAddresses []string) (*NIC, error) } @@ -95,4 +99,5 @@ type NIC struct { AllowedAddresses []string `json:"allowedAddresses,omitempty"` IPv4 string `json:"ipv4,omitempty"` IPv6 string `json:"ipv6,omitempty"` + Name string `json:"name"` } diff --git a/pkg/provider/core.go b/pkg/provider/core.go index 1cfacc0c..237047ae 100644 --- a/pkg/provider/core.go +++ b/pkg/provider/core.go @@ -13,6 +13,8 @@ const ( StackitProviderName = "stackit" StackitMachineLabel = "kubernetes.io/machine" StackitMachineClassLabel = "kubernetes.io/machineclass" + + migratedMachineAnnotation = "stackit.cloud/migrated-machine" ) // GetVolumeIDs extracts volume IDs from PersistentVolume specs diff --git a/pkg/provider/create.go b/pkg/provider/create.go index 930600b5..a6eaad4c 100644 --- a/pkg/provider/create.go +++ b/pkg/provider/create.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "slices" + "strconv" "github.com/gardener/machine-controller-manager/pkg/util/provider/driver" "github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/codes" @@ -47,6 +48,10 @@ func (p *Provider) CreateMachine(ctx context.Context, req *driver.CreateMachineR return nil, status.Error(codes.InvalidArgument, err.Error()) } + if m, _ := strconv.ParseBool(req.Machine.Annotations[migratedMachineAnnotation]); m { + return nil, status.Error(codes.AlreadyExists, fmt.Errorf("create for migrated machine %s will not work", req.Machine.Name).Error()) + } + // Decode ProviderSpec from MachineClass providerSpec, err := decodeProviderSpec(req.MachineClass) if err != nil { @@ -68,12 +73,25 @@ func (p *Provider) CreateMachine(ctx context.Context, req *driver.CreateMachineR } // check if server already exists - server, err := p.getServerByName(ctx, projectID, providerSpec.Region, req.Machine.Name) + servers, err := p.getServersByName(ctx, projectID, providerSpec.Region, map[string]string{ + StackitMachineLabel: req.Machine.Name, + }) if err != nil { klog.Errorf("Failed to fetch server for machine %q: %v", req.Machine.Name, err) return nil, status.Error(codes.Unavailable, fmt.Sprintf("failed to fetch server: %v", err)) } + if len(servers) > 1 { + klog.Errorf("Multiple servers already exists for this machine %q: %v", req.Machine.Name, err) + return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("failed to fetch server: %v", err)) + } + + var server *client.Server + + if len(servers) == 1 { + server = servers[0] + } + if server == nil { // Call STACKIT API to create server server, err = p.client.CreateServer(ctx, projectID, providerSpec.Region, p.createServerRequest(req, providerSpec)) @@ -233,26 +251,18 @@ func nicAddresses(nics []*client.NIC) []corev1.NodeAddress { return addresses } -func (p *Provider) getServerByName(ctx context.Context, projectID, region, serverName string) (*client.Server, error) { +func (p *Provider) getServersByName(ctx context.Context, projectID, region string, selector map[string]string) ([]*client.Server, error) { // Check if the server got already created - labelSelector := map[string]string{ - StackitMachineLabel: serverName, - } - servers, err := p.client.ListServers(ctx, projectID, region, labelSelector) + servers, err := p.client.ListServers(ctx, projectID, region, selector) if err != nil { - return nil, fmt.Errorf("SDK ListServers with labelSelector: %v failed: %w", labelSelector, err) + return nil, fmt.Errorf("SDK ListServers with labelSelector: %v failed: %w", selector, err) } - if len(servers) > 1 { - return nil, fmt.Errorf("%v servers found for server name %v", len(servers), serverName) - } - - if len(servers) == 1 { - return servers[0], nil + if len(servers) == 0 { + return nil, nil } - // no servers found len == 0 - return nil, nil + return servers, nil } func (p *Provider) patchNetworkInterfaces(ctx context.Context, projectID, serverID string, providerSpec *api.ProviderSpec) ([]*client.NIC, error) { diff --git a/pkg/provider/create_basic_test.go b/pkg/provider/create_basic_test.go index c6a47d14..8cbdbf24 100644 --- a/pkg/provider/create_basic_test.go +++ b/pkg/provider/create_basic_test.go @@ -85,6 +85,63 @@ var _ = Describe("CreateMachine", func() { }) Context("with valid inputs", func() { + It("rejects creation of a migrated machine", func() { + machine.Annotations = map[string]string{migratedMachineAnnotation: "true"} + listServersCalled := false + mockClient.ListServersFunc = func(_ context.Context, _, _ string, _ map[string]string) ([]*client.Server, error) { + listServersCalled = true + return nil, nil + } + + _, err := provider.CreateMachine(ctx, req) + + Expect(err).To(HaveOccurred()) + statusErr, ok := status.FromError(err) + Expect(ok).To(BeTrue()) + Expect(statusErr.Code()).To(Equal(codes.AlreadyExists)) + Expect(listServersCalled).To(BeFalse()) + }) + + It("returns AlreadyExists when more than one server has the machine label", func() { + createServerCalled := false + mockClient.ListServersFunc = func(_ context.Context, projectID, region string, selector map[string]string) ([]*client.Server, error) { + Expect(projectID).To(Equal("11111111-2222-3333-4444-555555555555")) + Expect(region).To(Equal("eu01")) + Expect(selector).To(Equal(map[string]string{StackitMachineLabel: "test-machine"})) + return []*client.Server{{ID: "server-1"}, {ID: "server-2"}}, nil + } + mockClient.CreateServerFunc = func(_ context.Context, _, _ string, _ *client.CreateServerRequest) (*client.Server, error) { + createServerCalled = true + return nil, nil + } + + _, err := provider.CreateMachine(ctx, req) + + Expect(err).To(HaveOccurred()) + statusErr, ok := status.FromError(err) + Expect(ok).To(BeTrue()) + Expect(statusErr.Code()).To(Equal(codes.AlreadyExists)) + Expect(createServerCalled).To(BeFalse()) + }) + + It("reuses the sole server returned for the machine label", func() { + createServerCalled := false + mockClient.ListServersFunc = func(_ context.Context, _, _ string, selector map[string]string) ([]*client.Server, error) { + Expect(selector).To(Equal(map[string]string{StackitMachineLabel: "test-machine"})) + return []*client.Server{{ID: "existing-server", Name: "test-machine", Status: "ACTIVE"}}, nil + } + mockClient.CreateServerFunc = func(_ context.Context, _, _ string, _ *client.CreateServerRequest) (*client.Server, error) { + createServerCalled = true + return nil, nil + } + + resp, err := provider.CreateMachine(ctx, req) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.ProviderID).To(Equal("stackit://11111111-2222-3333-4444-555555555555/existing-server")) + Expect(createServerCalled).To(BeFalse()) + }) + It("should successfully create a machine", func() { resp, err := provider.CreateMachine(ctx, req) diff --git a/pkg/provider/delete.go b/pkg/provider/delete.go index ff84e93c..be7e50b6 100644 --- a/pkg/provider/delete.go +++ b/pkg/provider/delete.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "github.com/gardener/machine-controller-manager/pkg/util/provider/driver" @@ -35,65 +36,107 @@ func (p *Provider) DeleteMachine(ctx context.Context, req *driver.DeleteMachineR return nil, status.Error(codes.Unauthenticated, fmt.Sprintf("failed to initialize STACKIT client: %v", err)) } - var projectID, serverID string - var err error - if req.Machine.Spec.ProviderID != "" { - if !strings.HasPrefix(req.Machine.Spec.ProviderID, StackitProviderName) { - return nil, status.Error(codes.InvalidArgument, "providerID is not empty and does not start with stackit://") + providerSpec, err := decodeProviderSpec(req.MachineClass) + if err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } + + migrated, _ := strconv.ParseBool(req.Machine.Annotations[migratedMachineAnnotation]) + projectID, serverIDs, err := p.serverIDsForMachine(ctx, req, projectIDFromSecret, providerSpec.Region, migrated) + if err != nil { + return nil, err + } + serverAlreadyDeleted, err := p.deleteServers(ctx, projectID, providerSpec.Region, req.Machine.Name, serverIDs) + if err != nil { + return nil, err + } + if serverAlreadyDeleted { + return &driver.DeleteMachineResponse{}, nil + } + if migrated { + nicAlreadyDeleted, err := p.deleteMachineNICs(ctx, projectID, providerSpec.Region, providerSpec.Networking.NetworkID, req.Machine.Name) + if err != nil { + return nil, err } + if nicAlreadyDeleted { + return &driver.DeleteMachineResponse{}, nil + } + } + klog.V(2).Infof("Successfully deleted server for machine %q", req.Machine.Name) + + return &driver.DeleteMachineResponse{}, nil +} - // Parse ProviderID to extract projectID and serverID - projectID, serverID, err = parseProviderID(req.Machine.Spec.ProviderID) +func (p *Provider) serverIDsForMachine(ctx context.Context, req *driver.DeleteMachineRequest, secretProjectID, region string, migrated bool) (projectID string, serverIDs []string, err error) { + projectID, serverIDs = "", nil + if providerID := req.Machine.Spec.ProviderID; providerID != "" { + if !strings.HasPrefix(providerID, StackitProviderName) { + return "", nil, status.Error(codes.InvalidArgument, "providerID is not empty and does not start with stackit://") + } + + var serverID string + projectID, serverID, err = parseProviderID(providerID) if err != nil { klog.V(2).Infof("invalid ProviderID format: %v", err) } + serverIDs = append(serverIDs, serverID) } - if projectID == "" { - // use the secret as a fallback - projectID = projectIDFromSecret + projectID = secretProjectID + } + if len(serverIDs) != 0 { + return projectID, serverIDs, nil } - providerSpec, err := decodeProviderSpec(req.MachineClass) + selector := map[string]string{StackitMachineLabel: req.Machine.Name} + if migrated { + selector = nil + } + servers, err := p.getServersByName(ctx, projectID, region, selector) if err != nil { - return nil, status.Error(codes.Internal, err.Error()) + return "", nil, status.Error(codes.Internal, fmt.Sprintf("failed to find server by name: %v", err)) } - - if serverID == "" { - server, err := p.getServerByName(ctx, projectID, providerSpec.Region, req.Machine.Name) - if err != nil { - return nil, status.Error(codes.Internal, fmt.Sprintf("failed to find server by name: %v", err)) - } - - if server != nil { - serverID = server.ID + for _, server := range servers { + if server.Name == req.Machine.Name { + serverIDs = append(serverIDs, server.ID) } } + return projectID, serverIDs, nil +} - if serverID == "" { - klog.V(2).Infof("Server is already deleted for machine %q", req.Machine.Name) - return &driver.DeleteMachineResponse{}, nil +func (p *Provider) deleteServers(ctx context.Context, projectID, region, machineName string, serverIDs []string) (bool, error) { + for _, serverID := range serverIDs { + if err := p.client.DeleteServer(ctx, projectID, region, serverID); err != nil { + if errors.Is(err, client.ErrServerNotFound) { + klog.V(2).Infof("Server %q already deleted for machine %q (idempotent)", serverID, machineName) + return true, nil + } + klog.Errorf("Failed to delete server for machine %q: %v", machineName, err) + return false, status.Error(codes.Internal, fmt.Sprintf("failed to delete server: %v", err)) + } } + return false, nil +} - // Call STACKIT API to delete server - err = p.client.DeleteServer(ctx, projectID, providerSpec.Region, serverID) +func (p *Provider) deleteMachineNICs(ctx context.Context, projectID, region, networkID, machineName string) (bool, error) { + nics, err := p.client.ListNICs(ctx, projectID, region, networkID) if err != nil { - // Check if server was not found (404) - this is OK for idempotency - if errors.Is(err, client.ErrServerNotFound) { - klog.V(2).Infof("Server %q already deleted for machine %q (idempotent)", serverID, req.Machine.Name) - return &driver.DeleteMachineResponse{}, nil - } - // All other errors are internal errors - klog.Errorf("Failed to delete server for machine %q: %v", req.Machine.Name, err) - return nil, status.Error(codes.Internal, fmt.Sprintf("failed to delete server: %v", err)) + return false, err } - - if err := p.WaitUntilServerDeleted(ctx, projectID, providerSpec.Region, serverID); err != nil { - klog.Errorf("Failed waiting for server %q to be deleted for machine %q: %v", serverID, req.Machine.Name, err) - return nil, status.Error(codes.DeadlineExceeded, fmt.Sprintf("failed waiting for server to be deleted: %v", err)) + for _, nic := range nics { + if nic.Name != machineName { + continue + } + if err := p.client.DeleteNIC(ctx, projectID, region, nic.NetworkID, nic.ID); err != nil { + if errors.Is(err, client.ErrNicNotFound) { + klog.V(2).Infof("Nic %q already deleted for machine %q (idempotent)", nic.ID, machineName) + return true, nil + } + klog.Errorf("Failed to delete nic for machine %q: %v", machineName, err) + return false, status.Error(codes.Internal, fmt.Sprintf("failed to delete nic: %v", err)) + } } - - return &driver.DeleteMachineResponse{}, nil + return false, nil } func (p *Provider) WaitUntilServerDeleted(ctx context.Context, projectID, region, serverID string) error { diff --git a/pkg/provider/delete_test.go b/pkg/provider/delete_test.go index a30dae1a..7cc063f9 100644 --- a/pkg/provider/delete_test.go +++ b/pkg/provider/delete_test.go @@ -53,6 +53,9 @@ var _ = Describe("DeleteMachine", func() { MachineType: "c2i.2", ImageID: "image-uuid-123", Region: "eu01", + Networking: &api.NetworkingSpec{ + NetworkID: "770e8400-e29b-41d4-a716-446655440000", + }, } providerSpecRaw, _ := mock.EncodeProviderSpec(providerSpec) @@ -120,42 +123,18 @@ var _ = Describe("DeleteMachine", func() { Expect(capturedServerID).To(Equal("550e8400-e29b-41d4-a716-446655440000")) }) - It("should poll GetServer until server is deleted", func() { - getServerCallCount := 0 - - mockClient.DeleteServerFunc = func(_ context.Context, _, _, _ string) error { - return nil - } - mockClient.GetServerFunc = func(_ context.Context, _, _, _ string) (*client.Server, error) { - getServerCallCount++ - // First call returns server still exists, second call returns not found - if getServerCallCount == 1 { - return &client.Server{ - ID: "550e8400-e29b-41d4-a716-446655440000", - Name: "test-machine", - Status: "SHUTTING_DOWN", - }, nil - } - return nil, fmt.Errorf("%w: status 404", client.ErrServerNotFound) - } - - resp, err := provider.DeleteMachine(ctx, req) - - Expect(err).NotTo(HaveOccurred()) - Expect(resp).NotTo(BeNil()) - Expect(getServerCallCount).To(BeNumerically(">=", 2)) - }) }) Context("with missing or invalid ProviderID", func() { It("should still delete the machine when ProviderID is missing", func() { machine.Spec.ProviderID = "" - mockClient.GetServerFunc = func(_ context.Context, _, _, _ string) (*client.Server, error) { - return &client.Server{ + mockClient.ListServersFunc = func(_ context.Context, _, _ string, selector map[string]string) ([]*client.Server, error) { + Expect(selector).To(Equal(map[string]string{StackitMachineLabel: "test-machine"})) + return []*client.Server{{ ID: "550e8400-e29b-41d4-a716-446655440000", Name: "test-machine", - }, nil + }}, nil } mockClient.DeleteServerFunc = func(_ context.Context, _, _, _ string) error { return nil @@ -178,6 +157,46 @@ var _ = Describe("DeleteMachine", func() { }) }) + Context("when deleting a migrated machine", func() { + It("deletes all matching servers and NICs after an unfiltered lookup", func() { + machine.Spec.ProviderID = "" + machine.Annotations = map[string]string{migratedMachineAnnotation: "true"} + var deletedServerIDs, deletedNICIDs []string + mockClient.ListServersFunc = func(_ context.Context, _, _ string, selector map[string]string) ([]*client.Server, error) { + Expect(selector).To(BeNil()) + return []*client.Server{ + {ID: "server-1", Name: "test-machine"}, + {ID: "other-server", Name: "another-machine"}, + {ID: "server-2", Name: "test-machine"}, + }, nil + } + mockClient.DeleteServerFunc = func(_ context.Context, _, _, serverID string) error { + deletedServerIDs = append(deletedServerIDs, serverID) + return nil + } + mockClient.ListNICsFunc = func(_ context.Context, _, _, networkID string) ([]*client.NIC, error) { + Expect(networkID).To(Equal("770e8400-e29b-41d4-a716-446655440000")) + return []*client.NIC{ + {ID: "nic-1", NetworkID: networkID, Name: "test-machine"}, + {ID: "other-nic", NetworkID: networkID, Name: "another-machine"}, + {ID: "nic-2", NetworkID: networkID, Name: "test-machine"}, + }, nil + } + mockClient.DeleteNICFunc = func(_ context.Context, _, _, networkID, nicID string) error { + Expect(networkID).To(Equal("770e8400-e29b-41d4-a716-446655440000")) + deletedNICIDs = append(deletedNICIDs, nicID) + return nil + } + + resp, err := provider.DeleteMachine(ctx, req) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp).NotTo(BeNil()) + Expect(deletedServerIDs).To(ConsistOf("server-1", "server-2")) + Expect(deletedNICIDs).To(ConsistOf("nic-1", "nic-2")) + }) + }) + Context("when machine not found", func() { It("should return success if machine does not exist (idempotent)", func() { mockClient.DeleteServerFunc = func(_ context.Context, _, _, _ string) error {