diff --git a/internal/controller/grpcroute_controller.go b/internal/controller/grpcroute_controller.go index 11a5a2ca..3968f619 100644 --- a/internal/controller/grpcroute_controller.go +++ b/internal/controller/grpcroute_controller.go @@ -94,7 +94,7 @@ func (r *GRPCRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index d2e507aa..200e9f01 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -116,7 +116,7 @@ func (r *HTTPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true @@ -200,7 +200,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if len(gateways) == 0 { if unresolvedParents { - // A missing Gateway or GatewayClass leaves ownership unknown rather than + // A missing GatewayClass leaves ownership unknown rather than // disproven. GatewayClass is cluster-scoped, so while one is absent every // route under it resolves empty and deleting would drain the data plane. return ctrl.Result{}, nil diff --git a/internal/controller/tcproute_controller.go b/internal/controller/tcproute_controller.go index 8428a55e..94350850 100644 --- a/internal/controller/tcproute_controller.go +++ b/internal/controller/tcproute_controller.go @@ -86,7 +86,7 @@ func (r *TCPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true diff --git a/internal/controller/tlsroute_controller.go b/internal/controller/tlsroute_controller.go index 1346c3ef..81286819 100644 --- a/internal/controller/tlsroute_controller.go +++ b/internal/controller/tlsroute_controller.go @@ -86,7 +86,7 @@ func (r *TLSRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true diff --git a/internal/controller/udproute_controller.go b/internal/controller/udproute_controller.go index a9475941..846ce3c8 100644 --- a/internal/controller/udproute_controller.go +++ b/internal/controller/udproute_controller.go @@ -86,7 +86,7 @@ func (r *UDPRouteReconciler) SetupWithManager(mgr ctrl.Manager) error { return false }, DeleteFunc: func(e event.DeleteEvent) bool { - return false + return true }, CreateFunc: func(e event.CreateEvent) bool { return true diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 732a7414..0b961e5a 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -339,10 +339,9 @@ func parentRefTargetsListenerExplicitly(parentRef gatewayv1.ParentReference) boo // ParseRouteParentRefs resolves the parentRefs of a route to the Gateways this // controller manages. The second return value reports that at least one -// parentRef could not be resolved, because its Gateway or that Gateway's -// GatewayClass does not exist. An empty gateway list then means "ownership -// unknown", not "owned by another controller", and callers must not act on the -// route's data plane configuration. +// parentRef points to an existing Gateway whose GatewayClass does not exist. +// Callers preserve the existing configuration while GatewayClass ownership +// cannot be determined. Missing Gateways do not keep stale routes active. func ParseRouteParentRefs( ctx context.Context, mgrc client.Client, @@ -369,7 +368,7 @@ func ParseRouteParentRefs( Name: name, }, &gateway); err != nil { if client.IgnoreNotFound(err) == nil { - unresolved = true + // A deleted Gateway no longer authorizes its routes to serve traffic. continue } return nil, false, fmt.Errorf("failed to retrieve gateway for route: %w", err) diff --git a/internal/controller/utils_parentref_test.go b/internal/controller/utils_parentref_test.go index f6e1ffa4..9d026aef 100644 --- a/internal/controller/utils_parentref_test.go +++ b/internal/controller/utils_parentref_test.go @@ -233,10 +233,9 @@ func TestParseRouteParentRefs_ConflictingTLSModePort(t *testing.T) { } // TestParseRouteParentRefs_UnresolvedParents verifies the second return value -// distinguishes "no parentRef named this controller" from "a parentRef could not -// be resolved". Callers delete data plane configuration on an empty gateway list, -// which is only correct in the first case: a GatewayClass is cluster-scoped, so -// while one is missing every route under it resolves to an empty list at once. +// distinguishes missing Gateways from unresolved GatewayClass ownership. +// Missing Gateways require route cleanup, while a missing cluster-scoped +// GatewayClass must not drain every route under it. func TestParseRouteParentRefs_UnresolvedParents(t *testing.T) { scheme := parentRefTestScheme(t) @@ -266,10 +265,10 @@ func TestParseRouteParentRefs_UnresolvedParents(t *testing.T) { wantUnresolved bool }{ { - name: "gateway missing", + name: "gateway missing retracts stale routes", objects: []client.Object{newParentRefGatewayClass()}, parentRef: "gw", - wantUnresolved: true, + wantUnresolved: false, }, { name: "gatewayclass missing", diff --git a/internal/provider/api7ee/provider.go b/internal/provider/api7ee/provider.go index 72cdcbea..3ba66db8 100644 --- a/internal/provider/api7ee/provider.go +++ b/internal/provider/api7ee/provider.go @@ -205,7 +205,9 @@ func (d *api7eeProvider) Delete(ctx context.Context, obj client.Object) error { resourceTypes = append(resourceTypes, "service") labels = label.GenLabel(obj) case *gatewayv1.Gateway: - // delete all resources + // Gateways sharing a GatewayProxy only own their own certificates. + resourceTypes = append(resourceTypes, "ssl") + labels = label.GenLabel(obj) case *networkingv1.Ingress: resourceTypes = append(resourceTypes, "service", "ssl") labels = label.GenLabel(obj) diff --git a/internal/provider/api7ee/provider_test.go b/internal/provider/api7ee/provider_test.go index 8ef49365..a63ccf77 100644 --- a/internal/provider/api7ee/provider_test.go +++ b/internal/provider/api7ee/provider_test.go @@ -19,8 +19,12 @@ package api7ee import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "strings" "testing" + "time" "github.com/go-logr/logr" "github.com/go-logr/logr/funcr" @@ -34,8 +38,10 @@ import ( adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/api/v1alpha1" apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + adcclient "github.com/apache/apisix-ingress-controller/internal/adc/client" "github.com/apache/apisix-ingress-controller/internal/controller/label" "github.com/apache/apisix-ingress-controller/internal/provider" + "github.com/apache/apisix-ingress-controller/internal/types" "github.com/apache/apisix-ingress-controller/internal/utils" ) @@ -133,3 +139,59 @@ func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { require.Len(t, resources.Services, 1) assert.Equal(t, "last-known-good", resources.Services[0].Name) } + +func TestDeleteGatewayPreservesSharedProxyResources(t *testing.T) { + var received adcclient.ADCServerRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&received)) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + cli, err := adcclient.New(logr.Discard(), "api7ee", time.Second) + require.NoError(t, err) + d := &api7eeProvider{client: cli, log: logr.Discard()} + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + other := gateway.DeepCopy() + other.Name = "remaining" + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }} + for _, gw := range []*gatewayv1.Gateway{gateway, other} { + labels := label.GenLabel(gw) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(gw), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeSSL}, + Resources: &adctypes.Resources{SSLs: []*adctypes.SSL{{Metadata: adctypes.Metadata{ID: gw.Name, Labels: labels}}}}, + })) + } + labels := label.GenLabel(route) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(route), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeService}, + Resources: &adctypes.Resources{Services: []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "route", Labels: labels}}}}, + })) + require.NoError(t, d.Delete(context.Background(), gateway)) + resources, err := cli.GetResources(proxy.String()) + require.NoError(t, err) + require.Len(t, resources.SSLs, 1) + assert.Equal(t, "remaining", resources.SSLs[0].ID) + require.Len(t, resources.Services, 1) + assert.Equal(t, "route", resources.Services[0].ID) + assert.Equal(t, []string{adctypes.TypeSSL}, received.Task.Opts.IncludeResourceType) + assert.Equal(t, map[string]string{label.LabelResourceKey: label.GenLabel(gateway)[label.LabelResourceKey]}, received.Task.Opts.LabelSelector) + _, err = cli.Sync(context.Background()) + require.NoError(t, err) + assert.Equal(t, resources.SSLs, received.Task.Config.SSLs) + assert.Equal(t, resources.Services, received.Task.Config.Services) + assert.Empty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(gateway))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(other))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(route))) +} diff --git a/internal/provider/apisix/provider.go b/internal/provider/apisix/provider.go index e6b8c04d..addd9f51 100644 --- a/internal/provider/apisix/provider.go +++ b/internal/provider/apisix/provider.go @@ -202,7 +202,9 @@ func (d *apisixProvider) Delete(ctx context.Context, obj client.Object) error { resourceTypes = append(resourceTypes, adctypes.TypeService) labels = label.GenLabel(obj) case *gatewayv1.Gateway: - // delete all resources + // Gateways sharing a GatewayProxy only own their own certificates. + resourceTypes = append(resourceTypes, adctypes.TypeSSL) + labels = label.GenLabel(obj) case *networkingv1.Ingress: resourceTypes = append(resourceTypes, adctypes.TypeService, adctypes.TypeSSL) labels = label.GenLabel(obj) diff --git a/internal/provider/apisix/provider_test.go b/internal/provider/apisix/provider_test.go index 50d9e420..214bdbda 100644 --- a/internal/provider/apisix/provider_test.go +++ b/internal/provider/apisix/provider_test.go @@ -19,6 +19,9 @@ package apisix import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -170,3 +173,58 @@ func TestUpdateKeepsLastKnownGoodStateWhenL4PolicyCannotRender(t *testing.T) { require.Len(t, resources.Services, 1) assert.Equal(t, "last-known-good", resources.Services[0].Name) } + +func TestDeleteGatewayPreservesSharedProxyResources(t *testing.T) { + var received adcclient.ADCServerRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&received)) + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(adctypes.SyncResult{Status: adctypes.StatusSuccess}) + })) + defer server.Close() + t.Setenv("ADC_SERVER_URL", server.URL) + cli, err := adcclient.New(logr.Discard(), ProviderTypeAPISIX, time.Second) + require.NoError(t, err) + d := &apisixProvider{client: cli, syncCh: make(chan struct{}, 1), log: logr.Discard()} + gateway := &gatewayv1.Gateway{ + TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "deleted"}, + } + other := gateway.DeepCopy() + other.Name = "remaining" + route := &gatewayv1.HTTPRoute{ + TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: gatewayv1.GroupVersion.String()}, + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + } + proxy := types.NamespacedNameKind{Kind: "GatewayProxy", Namespace: "default", Name: "proxy"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: { + Name: proxy.String(), BackendType: "apisix", ServerAddrs: []string{"http://apisix:9180"}, + }} + for _, gw := range []*gatewayv1.Gateway{gateway, other} { + labels := label.GenLabel(gw) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(gw), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeSSL}, + Resources: &adctypes.Resources{SSLs: []*adctypes.SSL{{Metadata: adctypes.Metadata{ID: gw.Name, Labels: labels}}}}, + })) + } + labels := label.GenLabel(route) + require.NoError(t, cli.UpdateConfig(context.Background(), adcclient.Task{ + Key: utils.NamespacedNameKind(route), Configs: configs, Labels: labels, ResourceTypes: []string{adctypes.TypeService}, + Resources: &adctypes.Resources{Services: []*adctypes.Service{{Metadata: adctypes.Metadata{ID: "route", Labels: labels}}}}, + })) + require.NoError(t, d.Delete(context.Background(), gateway)) + resources, err := cli.GetResources(proxy.String()) + require.NoError(t, err) + require.Len(t, resources.SSLs, 1) + assert.Equal(t, "remaining", resources.SSLs[0].ID) + require.Len(t, resources.Services, 1) + assert.Equal(t, "route", resources.Services[0].ID) + require.Len(t, d.syncCh, 1, "Gateway deletion must schedule the remaining shared snapshot") + _, err = cli.Sync(context.Background()) + require.NoError(t, err) + assert.Equal(t, resources.SSLs, received.Task.Config.SSLs) + assert.Equal(t, resources.Services, received.Task.Config.Services) + assert.Empty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(gateway))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(other))) + assert.NotEmpty(t, cli.ConfigManager.Get(utils.NamespacedNameKind(route))) +} diff --git a/test/conformance/conformance_test.go b/test/conformance/conformance_test.go index 223a0a73..9d794ab9 100644 --- a/test/conformance/conformance_test.go +++ b/test/conformance/conformance_test.go @@ -39,15 +39,8 @@ var skippedTestsForKnownGaps = []string{ tests.HTTPRouteListenerHostnameMatching.ShortName, tests.GRPCRouteListenerHostnameMatching.ShortName, - // A backendRef that cannot be resolved must still produce a route that - // answers 500; today no route is generated at all, so the request 404s. - // These consistently pass in standalone mode and fail against the APISIX - // admin API, and which member of the group trips is not stable between - // runs, so all four are skipped together rather than one at a time. + // Omitted backendRefs do not yet inject the required HTTP 500 response. tests.HTTPRouteNoBackendRefs.ShortName, - tests.HTTPRouteInvalidBackendRefUnknownKind.ShortName, - tests.HTTPRouteInvalidCrossNamespaceBackendRef.ShortName, - tests.HTTPRouteInvalidNonExistentBackendRef.ShortName, // Terminate mode itself is covered by TLSRouteListenerTerminateSupportedKinds // and by the e2e TLSRoute suite. This provisional test additionally requires a // standalone Gateway with no GatewayProxy attached to reach Accepted=True, and diff --git a/test/e2e/crds/v2/tls.go b/test/e2e/crds/v2/tls.go index 5a64cebc..c12588e1 100644 --- a/test/e2e/crds/v2/tls.go +++ b/test/e2e/crds/v2/tls.go @@ -131,13 +131,15 @@ spec: assert.ElementsMatch(GinkgoT(), []string{host}, tls[0].Snis) By("test HTTPS request to dataplane") - Eventually(func() int { - return s.NewAPISIXHttpsClient("api6.com"). - GET("/get"). - WithHost("api6.com"). - Expect(). - Raw().StatusCode - }).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK)) + s.RequestAssert(&scaffold.RequestAssert{ + Client: s.NewAPISIXHttpsClient("api6.com"), + Method: "GET", + Path: "/get", + Host: "api6.com", + Check: scaffold.WithExpectedStatus(http.StatusOK), + Timeout: 30 * time.Second, + Interval: time.Second, + }) s.NewAPISIXHttpsClient("api6.com"). GET("/get"). diff --git a/test/e2e/ingress/annotations.go b/test/e2e/ingress/annotations.go index 622d0cb8..2f813c88 100644 --- a/test/e2e/ingress/annotations.go +++ b/test/e2e/ingress/annotations.go @@ -623,19 +623,24 @@ spec: Check: scaffold.WithExpectedStatus(http.StatusPermanentRedirect), }) - _ = s.NewAPISIXClient(). - GET("/get"). - WithHost("httpbin.example"). - Expect(). - Status(http.StatusMovedPermanently). - Header("Location").IsEqual("https://httpbin.example:9443/get") - - _ = s.NewAPISIXClient(). - GET("/ip"). - WithHost("httpbin.example"). - Expect(). - Status(http.StatusPermanentRedirect). - Header("Location").IsEqual("/anything/ip") + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/get", + Host: "httpbin.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusMovedPermanently), + scaffold.WithExpectedHeader("Location", "https://httpbin.example:9443/get"), + }, + }) + s.RequestAssert(&scaffold.RequestAssert{ + Method: "GET", + Path: "/ip", + Host: "httpbin.example", + Checks: []scaffold.ResponseCheckFunc{ + scaffold.WithExpectedStatus(http.StatusPermanentRedirect), + scaffold.WithExpectedHeader("Location", "/anything/ip"), + }, + }) }) It("csrf", func() { diff --git a/test/e2e/scaffold/apisix_deployer.go b/test/e2e/scaffold/apisix_deployer.go index a08b3885..f841b64d 100644 --- a/test/e2e/scaffold/apisix_deployer.go +++ b/test/e2e/scaffold/apisix_deployer.go @@ -282,6 +282,12 @@ func (s *APISIXDeployer) deployDataplane(opts *APISIXDeployOptions) *corev1.Serv LabelSelector: "app.kubernetes.io/name=apisix", }) Expect(err).ToNot(HaveOccurred(), "waiting for gateway pod ready") + } else { + // A terminating pod can still be Ready and selected by a new port-forward. + err = k8s.WaitUntilNumPodsCreatedE(s.GinkgoT, kubectlOpts, metav1.ListOptions{ + LabelSelector: "app.kubernetes.io/name=apisix", + }, 0, 30, time.Second) + Expect(err).ToNot(HaveOccurred(), "waiting for gateway pods to terminate") } Eventually(func() bool {