diff --git a/cmd/e2e-test/neg_test.go b/cmd/e2e-test/neg_test.go index 902f3eb978..ed08231be4 100644 --- a/cmd/e2e-test/neg_test.go +++ b/cmd/e2e-test/neg_test.go @@ -18,6 +18,7 @@ package main import ( "context" + "fmt" "testing" apps "k8s.io/api/apps/v1" @@ -386,3 +387,169 @@ func TestReadinessReflector(t *testing.T) { } }) } + +func TestNegCRDTransitions(t *testing.T) { + t.Parallel() + port80 := intstr.FromInt(80) + port443 := intstr.FromInt(443) + serviceName := "neg-service" + ctx := context.Background() + + Framework.RunWithSandbox("NEGs with custom names", t, func(t *testing.T, s *e2e.Sandbox) { + var previousNegStatus annotations.NegStatus + expectedNEGName := fmt.Sprintf("test-neg-name-%x", s.RandInt) + + for _, tc := range []struct { + desc string + annotations annotations.NegAnnotation + replicas int32 + expectedNegAttrs map[string]string + expectedGCNegPorts []string + }{ + {desc: "one NEG with custom name, one neg with generated name", + annotations: annotations.NegAnnotation{ + Ingress: false, + ExposedPorts: map[int32]annotations.NegAttributes{ + int32(port80.IntValue()): annotations.NegAttributes{Name: expectedNEGName}, + int32(port443.IntValue()): annotations.NegAttributes{}, + }}, + replicas: 2, + expectedNegAttrs: map[string]string{port80.String(): expectedNEGName, port443.String(): ""}, + }, + {desc: "remove custom name", + annotations: annotations.NegAnnotation{ + Ingress: false, + ExposedPorts: map[int32]annotations.NegAttributes{ + int32(port80.IntValue()): annotations.NegAttributes{}, + int32(port443.IntValue()): annotations.NegAttributes{}, + }}, + replicas: 2, + expectedNegAttrs: map[string]string{port80.String(): "", port443.String(): ""}, + expectedGCNegPorts: []string{port80.String()}, + }, + {desc: "add custom name", + annotations: annotations.NegAnnotation{ + Ingress: false, + ExposedPorts: map[int32]annotations.NegAttributes{ + int32(port80.IntValue()): annotations.NegAttributes{}, + int32(port443.IntValue()): annotations.NegAttributes{Name: expectedNEGName}, + }}, + replicas: 2, + expectedNegAttrs: map[string]string{port80.String(): "", port443.String(): expectedNEGName}, + expectedGCNegPorts: []string{port443.String()}, + }, + {desc: "no NEGs", + annotations: annotations.NegAnnotation{ + Ingress: false, + ExposedPorts: map[int32]annotations.NegAttributes{}}, + replicas: 2, + expectedGCNegPorts: []string{port80.String(), port443.String()}, + }, + } { + _, err := e2e.EnsureEchoService(s, serviceName, map[string]string{ + annotations.NEGAnnotationKey: tc.annotations.String()}, v1.ServiceTypeClusterIP, tc.replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + t.Logf("Echo service ensured (%s/%s)", s.Namespace, serviceName) + + if len(tc.expectedGCNegPorts) > 0 { + for _, port := range tc.expectedGCNegPorts { + if err = e2e.WaitForStandaloneNegDeletion(ctx, s.ValidatorEnv.Cloud(), s, port, previousNegStatus); err != nil { + t.Errorf("Error waiting for NEGDeletion: %v", err) + } + } + } + + negStatus, err := e2e.WaitForNegCRs(s, serviceName, tc.expectedNegAttrs) + if err != nil { + t.Fatalf("Error: e2e.WaitForNegCRs(%s,%+v) = %s, want nil", serviceName, tc.expectedNegAttrs, err) + } + + for port, negName := range negStatus.NetworkEndpointGroups { + err := e2e.WaitForNegs(ctx, Framework.Cloud, negName, negStatus.Zones, false, int(tc.replicas)) + if err != nil { + t.Fatalf("Error: e2e.WaitForNegs service %s/%s neg port/name %s/%s", serviceName, s.Namespace, port, negName) + } + } + previousNegStatus = negStatus + } + }) +} + +func TestNegCRDErrorEvents(t *testing.T) { + t.Parallel() + port80 := intstr.FromInt(80) + svc1 := "svc1" + svc2 := "svc2" + replicas := int32(2) + ctx := context.Background() + + Framework.RunWithSandbox("two services, same neg name", t, func(t *testing.T, s *e2e.Sandbox) { + expectedNEGName := fmt.Sprintf("test-neg-name-%x", s.RandInt) + annotation := annotations.NegAnnotation{ + Ingress: true, + ExposedPorts: map[int32]annotations.NegAttributes{ + int32(port80.IntValue()): annotations.NegAttributes{Name: expectedNEGName}, + }, + } + + _, err := e2e.EnsureEchoService(s, svc1, map[string]string{ + annotations.NEGAnnotationKey: annotation.String()}, v1.ServiceTypeClusterIP, replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + + // Ingress true with a custom name should cause an event + if err = e2e.WaitForSvcNegErrorEvents(s, svc1, 1); err != nil { + t.Errorf("error waiting for error events: %s", err) + } + + // Ensure service with ingress true and wait for neg to be created + annotation.Ingress = false + _, err = e2e.EnsureEchoService(s, svc1, map[string]string{ + annotations.NEGAnnotationKey: annotation.String()}, v1.ServiceTypeClusterIP, replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + t.Logf("Echo service ensured (%s/%s)", s.Namespace, svc1) + + expectedNegAttrs := map[string]string{port80.String(): expectedNEGName} + negStatus, err := e2e.WaitForNegCRs(s, svc1, expectedNegAttrs) + if err != nil { + t.Fatalf("Error: e2e.WaitForNegCRs(%s,%+v) = %s, want nil", svc1, expectedNegAttrs, err) + } + + for port, negName := range negStatus.NetworkEndpointGroups { + err := e2e.WaitForNegs(ctx, Framework.Cloud, negName, negStatus.Zones, false, int(replicas)) + if err != nil { + t.Fatalf("Error: e2e.WaitForNegs service %s/%s neg port/name %s/%s", svc1, s.Namespace, port, negName) + } + } + + // Ensure a second service requesting the same neg name + _, err = e2e.EnsureEchoService(s, svc2, map[string]string{ + annotations.NEGAnnotationKey: annotation.String()}, v1.ServiceTypeClusterIP, replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + + // Requesting the same neg name should cause an error event on the second service + if err = e2e.WaitForSvcNegErrorEvents(s, svc2, 1); err != nil { + t.Errorf("error waiting for error events: %s", err) + } + + // GC existing negs + _, err = e2e.EnsureEchoService(s, svc1, map[string]string{}, v1.ServiceTypeClusterIP, replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + + _, err = e2e.EnsureEchoService(s, svc2, map[string]string{}, v1.ServiceTypeClusterIP, replicas) + if err != nil { + t.Fatalf("error ensuring echo service: %v", err) + } + + e2e.WaitForStandaloneNegDeletion(ctx, Framework.Cloud, s, port80.String(), negStatus) + }) +} diff --git a/pkg/e2e/framework.go b/pkg/e2e/framework.go index bd9a9719fe..8182702406 100644 --- a/pkg/e2e/framework.go +++ b/pkg/e2e/framework.go @@ -41,6 +41,7 @@ import ( "k8s.io/client-go/rest" backendconfigclient "k8s.io/ingress-gce/pkg/backendconfig/client/clientset/versioned" frontendconfigclient "k8s.io/ingress-gce/pkg/frontendconfig/client/clientset/versioned" + svcnegclient "k8s.io/ingress-gce/pkg/svcneg/client/clientset/versioned" "k8s.io/klog" ) @@ -76,7 +77,12 @@ func NewFramework(config *rest.Config, options Options) *Framework { frontendConfigClient, err := frontendconfigclient.NewForConfig(config) if err != nil { - klog.Fatalf("Failed to create BackendConfig client: %v", err) + klog.Fatalf("Failed to create FrontendConfig client: %v", err) + } + + svcNegClient, err := svcnegclient.NewForConfig(config) + if err != nil { + klog.Fatalf("Failed to create SvcNeg client: %v", err) } f := &Framework{ @@ -85,6 +91,7 @@ func NewFramework(config *rest.Config, options Options) *Framework { crdClient: apiextensionsclient.NewForConfigOrDie(config), FrontendConfigClient: frontendConfigClient, BackendConfigClient: backendConfigClient, + SvcNegClient: svcNegClient, Project: options.Project, Region: options.Region, Network: options.Network, @@ -127,6 +134,7 @@ type Framework struct { crdClient *apiextensionsclient.Clientset BackendConfigClient *backendconfigclient.Clientset FrontendConfigClient *frontendconfigclient.Clientset + SvcNegClient *svcnegclient.Clientset Project string Region string Network string @@ -243,9 +251,12 @@ func (f *Framework) WithSandbox(testFunc func(*Sandbox) error) error { func (f *Framework) RunWithSandbox(name string, t *testing.T, testFunc func(*testing.T, *Sandbox)) { t.Run(name, func(t *testing.T) { f.lock.Lock() + randInt := f.Rand.Int63() sandbox := &Sandbox{ - Namespace: fmt.Sprintf("test-sandbox-%x", f.Rand.Int63()), + + Namespace: fmt.Sprintf("test-sandbox-%x", randInt), f: f, + RandInt: randInt, } for _, s := range f.sandboxes { if s.Namespace == sandbox.Namespace { diff --git a/pkg/e2e/helpers.go b/pkg/e2e/helpers.go index c8ad79f59e..d15f4fd137 100644 --- a/pkg/e2e/helpers.go +++ b/pkg/e2e/helpers.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "net" "net/http" + "reflect" "sort" "strings" "time" @@ -37,17 +38,21 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/ingress-gce/cmd/echo/app" "k8s.io/ingress-gce/pkg/annotations" + negv1beta1 "k8s.io/ingress-gce/pkg/apis/svcneg/v1beta1" "k8s.io/ingress-gce/pkg/e2e/adapter" "k8s.io/ingress-gce/pkg/fuzz" "k8s.io/ingress-gce/pkg/fuzz/features" "k8s.io/ingress-gce/pkg/fuzz/whitebox" + negtypes "k8s.io/ingress-gce/pkg/neg/types" "k8s.io/ingress-gce/pkg/utils/common" "k8s.io/klog" + utilpointer "k8s.io/utils/pointer" ) const ( @@ -60,8 +65,9 @@ const ( // is fixed. gclbDeletionTimeout = 60 * time.Minute - negPollInterval = 5 * time.Second - negPollTimeout = 2 * time.Minute + negPollInterval = 5 * time.Second + negPollTimeout = 2 * time.Minute + negGCPollTimeout = 3 * time.Minute k8sApiPoolInterval = 10 * time.Second k8sApiPollTimeout = 30 * time.Minute @@ -539,6 +545,43 @@ func CheckNegStatus(svc *v1.Service, expectSvcPors []string) (annotations.NegSta return negStatus, nil } +// CheckNameInNegStatus checks if the NEG Status annotation is present and in the expected state +// The parameter expectedNegAttrs will map a port to a neg name. If the the neg name is empty, CheckNameInNegStatus expects +// that the name is autogenerated and will check it +func CheckNameInNegStatus(svc *v1.Service, expectedNegAttrs map[string]string) (annotations.NegStatus, error) { + annotation, ok := svc.Annotations[annotations.NEGStatusKey] + if !ok && len(expectedNegAttrs) > 0 { + return annotations.NegStatus{}, fmt.Errorf("service %s/%s does not have neg status annotation: %+v", svc.Namespace, svc.Name, svc) + } else if ok && len(expectedNegAttrs) == 0 { + return annotations.NegStatus{}, fmt.Errorf("service %s/%s should not have neg status annotation: %+v", svc.Namespace, svc.Name, svc) + } else if !ok && len(expectedNegAttrs) == 0 { + return annotations.NegStatus{}, nil + } + + negStatus, err := annotations.ParseNegStatus(annotation) + if err != nil { + return negStatus, fmt.Errorf("service %s/%s has invalid neg status annotation %s: %+v", svc.Namespace, svc.Name, annotation, err) + } + + existingPorts := 0 + for port, negName := range negStatus.NetworkEndpointGroups { + expectedName, ok := expectedNegAttrs[port] + if !ok { + // Port was not expected + return annotations.NegStatus{}, fmt.Errorf("CheckCustomNegNameStatus: Service: %s, did not expect NEG with port %s", svc.Name, port) + } + if expectedName != "" && negName != expectedName { + return annotations.NegStatus{}, fmt.Errorf("CheckCustomNegNameStatus: Service: %s, with port %s was expected to have name %s", svc.Name, port, expectedName) + } + existingPorts += 1 + } + + if existingPorts != len(expectedNegAttrs) { + return negStatus, fmt.Errorf("service %s/%s does not have neg status annotation: %s, want port:name %+v", svc.Namespace, svc.Name, annotation, expectedNegAttrs) + } + return negStatus, nil +} + // CheckForAnyFinalizer asserts that an ingress finalizer exists on Ingress. func CheckForAnyFinalizer(ing *v1beta1.Ingress) error { ingFinalizers := ing.GetFinalizers() @@ -664,3 +707,194 @@ func waitForBackendConfigCRDEstablish(crdClient *apiextensionsclient.Clientset) } return nil } + +// WaitForNegCRs waits up to the gclbDeletionTimeout for neg crs that have the configurations in expectedNegs, and are owned by the given service name, +// otherwise returns an error. The parameter expectedNegs maps a port to an expected neg name or an empty string for a generated name. +func WaitForNegCRs(s *Sandbox, serviceName string, expectedNegs map[string]string) (annotations.NegStatus, error) { + var svc *v1.Service + + err := wait.Poll(negPollInterval, negPollTimeout, func() (bool, error) { + var err error + svc, err = s.f.Clientset.CoreV1().Services(s.Namespace).Get(context.TODO(), serviceName, metav1.GetOptions{}) + if svc == nil || err != nil { + return false, fmt.Errorf("failed to get service %s/%s: %v", s.Namespace, serviceName, err) + } + + svcNegs, err := s.f.SvcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(s.Namespace).List(context.Background(), metav1.ListOptions{}) + if err != nil { + return false, fmt.Errorf("failed to get negCRs : %s", err) + } + + err = CheckNegCRs(svc, svcNegs, expectedNegs) + if err != nil { + klog.Infof("WaitForCustomNameNegs(%s/%s, %v) = %v", s.Namespace, serviceName, expectedNegs, err) + return false, nil + } + + return true, nil + }) + + if err != nil { + return annotations.NegStatus{}, err + } + + return CheckNameInNegStatus(svc, expectedNegs) +} + +// CheckNegCRs will check that the provided neg cr list have negs with the expected neg attributes +func CheckNegCRs(svc *v1.Service, svcNegs *negv1beta1.ServiceNetworkEndpointGroupList, expectedNegAttrs map[string]string) error { + portsFound := 0 + for _, svcNeg := range svcNegs.Items { + port := svcNeg.Labels[negtypes.NegCRServicePortKey] + svcName := svcNeg.Labels[negtypes.NegCRServiceNameKey] + if svcName != svc.Name { + // not part of this service, so no need to keep checking + continue + } + + expectedName, ok := expectedNegAttrs[port] + if !ok { + // Port was not expected + return fmt.Errorf("CheckNegCRs: Service: %s, did not expect NEG with port %s", svc.Name, port) + } + if expectedName != "" && svcNeg.Name != expectedName { + return fmt.Errorf("CheckNegCRs: Service: %s, with port %s was expected to have name %s", svc.Name, port, expectedName) + } + err := CheckNegOwnerRef(svc, svcNeg) + if err != nil { + return fmt.Errorf("CheckNegCRs: errored checking neg owner reference: %s", err) + } + + err = CheckNegFinalizer(svcNeg) + if err != nil { + return fmt.Errorf("CheckNegCRs: errored checking neg finalizer: %s", err) + } + + portsFound += 1 + } + + if portsFound != len(expectedNegAttrs) { + return fmt.Errorf("missing one or more negs for service %s/%s found %d, want negs %+v", svc.Namespace, svc.Name, portsFound, expectedNegAttrs) + } + return nil +} + +// WaitForStandaloneNegDeletion waits for standalone NEGs and corresponding CR are deleted via GC. +func WaitForStandaloneNegDeletion(ctx context.Context, c cloud.Cloud, s *Sandbox, port string, negStatus annotations.NegStatus) error { + negName := negStatus.NetworkEndpointGroups[port] + return wait.Poll(negPollInterval, negGCPollTimeout, func() (bool, error) { + if crDeleted, err := CheckDeletedNegCRs(s, negName, port); !crDeleted { + return false, err + } + + negsDeleted, err := fuzz.CheckStandaloneNEGDeletion(ctx, c, negName, port, negStatus.Zones) + if err != nil { + return false, fmt.Errorf("WaitForStandaloneNegDeletion() error: %s", err) + } else if !negsDeleted { + return false, nil + } + return true, nil + }) +} + +// CheckDeletedNegCRs verifies that the provided neg list does not have negs that are associated with the provided neg atrributes +func CheckDeletedNegCRs(s *Sandbox, negName, port string) (bool, error) { + svcNeg, err := s.f.SvcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(s.Namespace).Get(context.Background(), negName, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } else if apierrors.IsNotFound(err) { + klog.Infof("CheckDeletedNegCR() failed querying for neg %s/%s: %s", s.Namespace, negName, err) + return false, err + } + } + + portLabel := svcNeg.Labels[negtypes.NegCRServicePortKey] + if port != portLabel { + // If port is different, not the CR that needs to be deleted + return true, nil + } + return false, nil +} + +// CheckNegOwnerRef verifies the owner reference on the provided neg cr point to the given service +func CheckNegOwnerRef(svc *v1.Service, svcNeg negv1beta1.ServiceNetworkEndpointGroup) error { + if len(svcNeg.OwnerReferences) != 1 { + return fmt.Errorf("CheckNegOwnerRef: neg %s/%s has more than one owner reference", svcNeg.Labels[negtypes.NegCRServicePortKey], svcNeg.Name) + } + + gvk := schema.GroupVersionKind{Version: "v1", Kind: "Service"} + expectedOwnerReference := metav1.NewControllerRef(svc, gvk) + expectedOwnerReference.BlockOwnerDeletion = utilpointer.BoolPtr(false) + + if !reflect.DeepEqual(*expectedOwnerReference, svcNeg.OwnerReferences[0]) { + return fmt.Errorf("CheckNegOwnerRef: neg %s/%s owner reference is %+v expected %+v", svcNeg.Namespace, svcNeg.Name, svcNeg.OwnerReferences[0], expectedOwnerReference) + } + + return nil +} + +// CheckNegFinalizer asserts that only the Neg finalizer exists on NEG CR. +func CheckNegFinalizer(svcNeg negv1beta1.ServiceNetworkEndpointGroup) error { + negFinalizers := svcNeg.GetFinalizers() + if l := len(negFinalizers); l != 1 { + return fmt.Errorf("expected 1 neg Finalizer but got %d", l) + } + if negFinalizers[0] != common.NegFinalizerKey { + return fmt.Errorf("expected neg Finalizer %q but got %q", common.NegFinalizerKey, negFinalizers[0]) + } + return nil +} + +// WaitForSvcNegErrorEvents waits for msgs messages present for namespace:name ConfigMap until timeout. +func WaitForSvcNegErrorEvents(s *Sandbox, svcName string, numMessages int) error { + svc, err := s.f.Clientset.CoreV1().Services(s.Namespace).Get(context.TODO(), svcName, metav1.GetOptions{}) + if svc == nil || err != nil { + return fmt.Errorf("failed to get service %s/%s: %v", s.Namespace, svcName, err) + } + + return wait.Poll(5*time.Second, negPollTimeout, func() (bool, error) { + eventList, err := s.f.Clientset.CoreV1().Events(s.Namespace).Search(Scheme, svc) + if err != nil { + return false, err + } + if len(eventList.Items) < numMessages { + return false, nil + } + + errEvents := 0 + for _, event := range eventList.Items { + if event.Type == v1.EventTypeWarning { + errEvents += 1 + } + } + + if errEvents != numMessages { + klog.Infof("WaitForSvcNegErrorEvents(), expected to find %d error events, found %d", numMessages, errEvents) + return false, nil + } + + return true, nil + }) +} + +// CreateNegCR creates a neg cr with the provided neg name and service port. The neg cr created will not be a valid one +// and is expected to be GC'd by the controller +func CreateNegCR(s *Sandbox, negName string, servicePort string) error { + + neg := negv1beta1.ServiceNetworkEndpointGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: negName, + Labels: map[string]string{ + negtypes.NegCRServicePortKey: servicePort, + }, + }, + } + _, err := s.f.SvcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(s.Namespace).Create(context.Background(), &neg, metav1.CreateOptions{}) + return err +} + +// DeleteNegCR sends a deletion request for the neg cr with the provided negName in the sandbox's namespace +func DeleteNegCR(s *Sandbox, negName string) error { + return s.f.SvcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(s.Namespace).Delete(context.Background(), negName, metav1.DeleteOptions{}) +} diff --git a/pkg/e2e/sandbox.go b/pkg/e2e/sandbox.go index 81a39e74c8..d0b1496325 100644 --- a/pkg/e2e/sandbox.go +++ b/pkg/e2e/sandbox.go @@ -41,6 +41,8 @@ type Sandbox struct { lock sync.Mutex f *Framework destroyed bool + //Rand int that is used to generate the Namespace name + RandInt int64 } // Create the sandbox. diff --git a/pkg/fuzz/gcp.go b/pkg/fuzz/gcp.go index f71565fdd2..febd4b9830 100644 --- a/pkg/fuzz/gcp.go +++ b/pkg/fuzz/gcp.go @@ -932,3 +932,38 @@ func NetworkEndpointsInNegs(ctx context.Context, c cloud.Cloud, name string, zon } return ret, nil } + +// CheckStandaloneNEGDeletion checks that specified NEG has been deleted +func CheckStandaloneNEGDeletion(ctx context.Context, c cloud.Cloud, negName, port string, zones []string) (bool, error) { + var foundNegs []string + + for _, zone := range zones { + key := meta.ZonalKey(negName, zone) + neg, err := c.NetworkEndpointGroups().Get(ctx, key) + if err != nil { + if e, ok := err.(*googleapi.Error); ok && e.Code == http.StatusNotFound { + continue + } + return false, err + } + + if neg.Description != "" { + desc, err := utils.NegDescriptionFromString(neg.Description) + if err == nil && desc.Port != port { + continue + } + + if err != nil { + return false, err + } + } + foundNegs = append(foundNegs, negName) + } + + if len(foundNegs) != 0 { + klog.Infof("CheckStandaloneNEGDeletion(), expected neg %s not to exist", negName) + return false, nil + } + + return true, nil +}