From 0714d4bc120518be4a1e446adcf864f9de00d1cd Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Sun, 14 Jan 2024 12:25:24 -0700 Subject: [PATCH] always return lbnotfound from getNodeBalancerForService --- cloud/linode/loadbalancers.go | 21 +++--- cloud/linode/loadbalancers_test.go | 10 +-- e2e/test/ccm_e2e_test.go | 104 ++++++++++++++++++++++++++++- e2e/test/framework/service.go | 4 ++ 4 files changed, 122 insertions(+), 17 deletions(-) diff --git a/cloud/linode/loadbalancers.go b/cloud/linode/loadbalancers.go index e9293fbf..e3346cf8 100644 --- a/cloud/linode/loadbalancers.go +++ b/cloud/linode/loadbalancers.go @@ -64,23 +64,13 @@ func newLoadbalancers(client Client, zone string) cloudprovider.LoadBalancer { } func (l *loadbalancers) getNodeBalancerForService(ctx context.Context, service *v1.Service) (*linodego.NodeBalancer, error) { - rawID, _ := getServiceAnnotation(service, annLinodeNodeBalancerID) + rawID := service.GetAnnotations()[annLinodeNodeBalancerID] id, idErr := strconv.Atoi(rawID) hasIDAnn := idErr == nil && id != 0 if hasIDAnn { sentry.SetTag(ctx, "load_balancer_id", rawID) - nb, err := l.getNodeBalancerByID(ctx, service, id) - switch err.(type) { - case nil: - return nb, nil - - case lbNotFoundError: - return nil, fmt.Errorf("%s annotation points to a NodeBalancer that does not exist: %s", annLinodeNodeBalancerID, err) - - default: - return nil, err - } + return l.getNodeBalancerByID(ctx, service, id) } return l.getNodeBalancerByStatus(ctx, service) } @@ -199,6 +189,13 @@ func (l *loadbalancers) EnsureLoadBalancer(ctx context.Context, clusterName stri nb, err = l.getNodeBalancerForService(ctx, service) switch err.(type) { case lbNotFoundError: + if service.GetAnnotations()[annLinodeNodeBalancerID] != "" { + // a load balancer annotation has been created so a NodeBalancer is coming, error out and retry later + klog.Infof("NodeBalancer created but not available yet, waiting...") + sentry.CaptureError(ctx, err) + return nil, err + } + if nb, err = l.buildLoadBalancerRequest(ctx, clusterName, service, nodes); err != nil { sentry.CaptureError(ctx, err) return nil, err diff --git a/cloud/linode/loadbalancers_test.go b/cloud/linode/loadbalancers_test.go index fd20129b..67ca7bb5 100644 --- a/cloud/linode/loadbalancers_test.go +++ b/cloud/linode/loadbalancers_test.go @@ -1776,10 +1776,12 @@ func testGetNodeBalancerForServiceIDDoesNotExist(t *testing.T, client *linodego. t.Fatal("expected getNodeBalancerForService to return an error") } - expectedErr := fmt.Sprintf("%s annotation points to a NodeBalancer that does not exist: LoadBalancer (%s) not found for service (%s)", - annLinodeNodeBalancerID, bogusNodeBalancerID, getServiceNn(svc)) - - if err.Error() != expectedErr { + nbid, _ := strconv.Atoi(bogusNodeBalancerID) + expectedErr := lbNotFoundError{ + serviceNn: getServiceNn(svc), + nodeBalancerID: nbid, + } + if err.Error() != expectedErr.Error() { t.Errorf("expected error to be '%s' but got '%s'", expectedErr, err) } } diff --git a/e2e/test/ccm_e2e_test.go b/e2e/test/ccm_e2e_test.go index 7d4373a9..d5ce41ba 100644 --- a/e2e/test/ccm_e2e_test.go +++ b/e2e/test/ccm_e2e_test.go @@ -6,6 +6,9 @@ import ( "fmt" "os/exec" "strconv" + "time" + + "k8s.io/apimachinery/pkg/api/errors" "github.com/linode/linodego" . "github.com/onsi/ginkgo/v2" @@ -90,6 +93,14 @@ var _ = Describe("e2e tests", func() { Eventually(watcher.ResultChan()).Should(Receive(EnsuredService())) } + ensureServiceWasDeleted := func() { + err := func() error { + _, err := f.LoadBalancer.GetService() + return err + } + Eventually(err).WithTimeout(10 * time.Second).Should(MatchError(errors.IsNotFound, "IsNotFound")) + } + createServiceWithSelector := func(selector map[string]string, ports []core.ServicePort, isSessionAffinityClientIP bool) { Expect(f.LoadBalancer.CreateService(selector, nil, ports, isSessionAffinityClientIP)).NotTo(HaveOccurred()) Eventually(f.LoadBalancer.GetServiceEndpoints).Should(Not(BeEmpty())) @@ -942,7 +953,7 @@ var _ = Describe("e2e tests", func() { By("Creating new NodeBalancer") nbID := createNodeBalancer() - By("Waiting for currenct NodeBalancer to be ready") + By("Waiting for current NodeBalancer to be ready") checkNodeBalancerID(framework.TestServerResourceName, nodeBalancerID) By("Annotating service with new NodeBalancer ID") @@ -957,6 +968,97 @@ var _ = Describe("e2e tests", func() { }) }) + Context("Deleted Service when NodeBalancer not present", func() { + var ( + pods []string + labels map[string]string + annotations map[string]string + servicePorts []core.ServicePort + + nodeBalancerID int + ) + + BeforeEach(func() { + pods = []string{"test-pod-1"} + ports := []core.ContainerPort{ + { + Name: "http-1", + ContainerPort: 8080, + }, + } + servicePorts = []core.ServicePort{ + { + Name: "http-1", + Port: 80, + TargetPort: intstr.FromInt(8080), + Protocol: "TCP", + }, + } + + labels = map[string]string{ + "app": "test-loadbalancer-with-nodebalancer-id", + } + + By("Creating NodeBalancer") + nodeBalancerID = createNodeBalancer() + + annotations = map[string]string{ + annLinodeNodeBalancerID: strconv.Itoa(nodeBalancerID), + } + + By("Creating Pod") + createPodWithLabel(pods, ports, framework.TestServerImage, labels, false) + + By("Creating Service") + createServiceWithAnnotations(labels, annotations, servicePorts, false) + }) + + AfterEach(func() { + By("Deleting the Pods") + deletePods(pods) + + err := root.Recycle() + Expect(err).NotTo(HaveOccurred()) + }) + + It("should use the specified NodeBalancer", func() { + By("Checking the NodeBalancerID") + checkNodeBalancerID(framework.TestServerResourceName, nodeBalancerID) + }) + + It("should use the newly specified NodeBalancer ID", func() { + By("Creating new NodeBalancer") + nbID := createNodeBalancer() + + By("Waiting for current NodeBalancer to be ready") + checkNodeBalancerID(framework.TestServerResourceName, nodeBalancerID) + + By("Annotating service with new NodeBalancer ID") + annotations[annLinodeNodeBalancerID] = strconv.Itoa(nbID) + updateServiceWithAnnotations(labels, annotations, servicePorts, false) + + By("Checking the NodeBalancer ID") + checkNodeBalancerID(framework.TestServerResourceName, nbID) + + By("Checking old NodeBalancer was deleted") + checkNodeBalancerNotExists(nodeBalancerID) + }) + + It("should delete the service with no NodeBalancer present", func() { + By("Deleting the NodeBalancer") + deleteNodeBalancer(nodeBalancerID) + + By("Checking old NodeBalancer was deleted") + checkNodeBalancerNotExists(nodeBalancerID) + + By("Deleting the Service") + deleteService() + + By("Checking if the service was deleted") + ensureServiceWasDeleted() + }) + }) + Context("With Preserve Annotation", func() { var ( pods []string diff --git a/e2e/test/framework/service.go b/e2e/test/framework/service.go index 2f0b8aac..e1c1d8be 100644 --- a/e2e/test/framework/service.go +++ b/e2e/test/framework/service.go @@ -69,6 +69,10 @@ func (i *lbInvocation) GetServiceWatcher() (watch.Interface, error) { return watcher, nil } +func (i *lbInvocation) GetService() (*core.Service, error) { + return i.kubeClient.CoreV1().Services(i.Namespace()).Get(context.TODO(), TestServerResourceName, metav1.GetOptions{}) +} + func (i *lbInvocation) CreateService(selector, annotations map[string]string, ports []core.ServicePort, isSessionAffinityClientIP bool) error { return i.createOrUpdateService(selector, annotations, ports, isSessionAffinityClientIP, true) }