From c21aa6c7b1d2618078e1ed19ae6d888f370754dd Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 10 Sep 2026 08:30:41 +0800 Subject: [PATCH] fix: reject an ApisixRoute servicePort that cannot resolve Backports apache/apisix-ingress-controller#2860. An empty servicePort is accepted by the API server, reported as Accepted=True with an empty message, and answers 503. It is compared against Service port names, so it either matches nothing, or silently matches a single-port Service that omits its port name. validateHTTPBackend logs and returns nil when the port does not match, so the Service never reaches tctx.Services, the translator then reports "service not found" for a Service that exists, and buildUpstream swallows that error and publishes an upstream with no nodes. Reject it in validateHTTPBackend before the reference is resolved, so the empty value cannot match an unnamed Service port by accident, and in getPortFromService so the translator cannot make that match through another path. A Service that resolves but has no such port now reports InvalidSpec and names the port rather than the Service. A missing Service is left alone, so applying a route alongside its Service still works. Upstream first tried a CEL rule on the CRD; the API server refuses it because servicePort is x-kubernetes-int-or-string with no maxLength, so the cost estimator prices any string operation against the maximum request size. The CRD is unchanged here. The reconciler test carries its own provider and updater stubs: the recordingProvider on master does not count Update calls and there is no shared updater stub, and inventing a cross-PR dependency for twenty lines is not worth it. --- internal/adc/translator/apisixroute.go | 5 + internal/controller/apisixroute_controller.go | 24 +- .../apisixroute_serviceport_test.go | 209 ++++++++++++++++++ 3 files changed, 232 insertions(+), 6 deletions(-) create mode 100644 internal/controller/apisixroute_serviceport_test.go diff --git a/internal/adc/translator/apisixroute.go b/internal/adc/translator/apisixroute.go index 8b6a932b..85ba2d29 100644 --- a/internal/adc/translator/apisixroute.go +++ b/internal/adc/translator/apisixroute.go @@ -334,6 +334,11 @@ func getPortFromService(svc *corev1.Service, backendSvcPort intstr.IntOrString) if backendSvcPort.Type == intstr.Int { port = int32(backendSvcPort.IntValue()) } else { + // A Service port may omit its name, so an empty name would match it by + // accident instead of being reported as the invalid reference it is. + if backendSvcPort.StrVal == "" { + return 0, errors.Errorf("service port must not be empty for service %s", svc.Name) + } found := false for _, servicePort := range svc.Spec.Ports { if servicePort.Name == backendSvcPort.StrVal { diff --git a/internal/controller/apisixroute_controller.go b/internal/controller/apisixroute_controller.go index ca281416..7424b110 100644 --- a/internal/controller/apisixroute_controller.go +++ b/internal/controller/apisixroute_controller.go @@ -435,6 +435,16 @@ func (r *ApisixRouteReconciler) validateHTTPBackend(tctx *provider.TranslateCont } ) + // An empty port never resolves, and an empty name would otherwise match a + // Service port that omits its name, which is allowed for a single-port Service. + // Reject it before the reference is resolved: no ordering makes it valid. + if backend.ServicePort.Type == intstr.String && backend.ServicePort.StrVal == "" { + return types.ReasonError{ + Reason: string(apiv2.ConditionReasonInvalidSpec), + Message: fmt.Sprintf("servicePort must not be empty, Service: %s", serviceNN), + } + } + if err := r.Get(tctx, serviceNN, &service); err != nil { if k8serrors.IsNotFound(err) { r.Log.Info("service not found", "Service", serviceNN) @@ -482,12 +492,14 @@ func (r *ApisixRouteReconciler) validateHTTPBackend(tctx *provider.TranslateCont } return false }) { - r.Log.Error(errors.New("service port not found"), - "failed to match service port", - "Service", serviceNN, - "ServicePort", backend.ServicePort, - ) - return nil + // The Service resolves but has no such port. Reporting this as accepted + // publishes a route with no upstream node, which answers 503 while the + // status claims the spec is fine. + return types.ReasonError{ + Reason: string(apiv2.ConditionReasonInvalidSpec), + Message: fmt.Sprintf("service port not found: Service %s has no port %s", + serviceNN, backend.ServicePort.String()), + } } tctx.Services[serviceNN] = &service diff --git a/internal/controller/apisixroute_serviceport_test.go b/internal/controller/apisixroute_serviceport_test.go new file mode 100644 index 00000000..a94fbf24 --- /dev/null +++ b/internal/controller/apisixroute_serviceport_test.go @@ -0,0 +1,209 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package controller + +import ( + "context" + "net/http" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + apiv2 "github.com/apache/apisix-ingress-controller/api/v2" + "github.com/apache/apisix-ingress-controller/internal/controller/config" + "github.com/apache/apisix-ingress-controller/internal/controller/status" + "github.com/apache/apisix-ingress-controller/internal/manager/readiness" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +// servicePortProvider and servicePortUpdater are this file's own stubs. Upstream +// shares recordingProvider and recordingUpdater across the reconciler tests; the +// copy of recordingProvider here counts Update calls, which the shared one does +// not, and there is no shared updater stub to reuse. Fold them together when the +// upstream test scaffolding is backported. +type servicePortProvider struct { + updated int + deleted []k8stypes.NamespacedName + deleteErr error +} + +func (p *servicePortProvider) Register(string, *http.ServeMux) {} + +func (p *servicePortProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + p.updated++ + return nil +} + +func (p *servicePortProvider) Delete(_ context.Context, obj client.Object) error { + p.deleted = append(p.deleted, k8stypes.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}) + return p.deleteErr +} + +func (p *servicePortProvider) Start(context.Context) error { return nil } + +func (p *servicePortProvider) NeedLeaderElection() bool { return true } + +type servicePortUpdater struct { + updates []status.Update +} + +func (u *servicePortUpdater) Update(update status.Update) { u.updates = append(u.updates, update) } + +const servicePortTestNamespace = "default" + +var servicePortRouteKey = k8stypes.NamespacedName{Namespace: servicePortTestNamespace, Name: "route"} + +// newServicePortFixture wires an ApisixRoute whose single backend names port +// against a Service exposing servicePorts. +func newServicePortFixture( + t *testing.T, + port intstr.IntOrString, + servicePorts []corev1.ServicePort, +) (*ApisixRouteReconciler, *servicePortProvider, *servicePortUpdater) { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, apiv2.AddToScheme(scheme)) + + ingressClass := &networkingv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{Name: "apisix"}, + Spec: networkingv1.IngressClassSpec{Controller: config.GetControllerName()}, + } + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Namespace: servicePortTestNamespace, Name: "backend"}, + Spec: corev1.ServiceSpec{ClusterIP: "10.0.0.1", Ports: servicePorts}, + } + route := &apiv2.ApisixRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: servicePortTestNamespace, Name: "route"}, + Spec: apiv2.ApisixRouteSpec{ + IngressClassName: "apisix", + HTTP: []apiv2.ApisixRouteHTTP{{ + Name: "rule", + Match: apiv2.ApisixRouteHTTPMatch{Hosts: []string{"crd.test"}, Paths: []string{"/*"}}, + Backends: []apiv2.ApisixRouteHTTPBackend{{ServiceName: "backend", ServicePort: port}}, + }}, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects([]client.Object{ingressClass, service, route}...). + WithStatusSubresource(route). + Build() + + readier := readiness.NewReadinessManager(cli, logr.Discard()) + require.NoError(t, readier.Start(context.Background())) + + prov := &servicePortProvider{} + updater := &servicePortUpdater{} + return &ApisixRouteReconciler{ + Client: cli, + Scheme: scheme, + Log: logr.Discard(), + Provider: prov, + Updater: updater, + Readier: readier, + }, prov, updater +} + +// acceptedCondition applies the recorded status update and returns the Accepted +// condition it would have written. +func acceptedCondition(t *testing.T, updater *servicePortUpdater) metav1.Condition { + t.Helper() + require.Len(t, updater.updates, 1, "the reconcile must report a status") + mutated, ok := updater.updates[0].Mutator.Mutate(&apiv2.ApisixRoute{}).(*apiv2.ApisixRoute) + require.True(t, ok) + require.Len(t, mutated.Status.Conditions, 1) + return mutated.Status.Conditions[0] +} + +// An empty servicePort is compared against Service port names, so it silently +// matches a single-port Service that omits its port name. Nothing in the CRD +// schema rejects it, so validateHTTPBackend is the only thing standing between +// this value and a published route; the admission webhook runs the same check. +func TestApisixRouteReconcile_EmptyServicePortIsRejected(t *testing.T) { + for name, ports := range map[string][]corev1.ServicePort{ + "named port": {{Name: "http", Port: 80, TargetPort: intstr.FromInt32(8080)}}, + "unnamed port": {{Port: 80, TargetPort: intstr.FromInt32(8080)}}, + } { + t.Run(name, func(t *testing.T) { + r, prov, updater := newServicePortFixture(t, intstr.FromString(""), ports) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: servicePortRouteKey}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "servicePort must not be empty") + assert.Zero(t, prov.updated, "a route with an unresolvable port must not be published") + + cond := acceptedCondition(t, updater) + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Contains(t, cond.Message, "servicePort must not be empty") + }) + } +} + +// A Service that exists but has no such port used to be reported as accepted and +// published with no upstream node, which answers 503. The message also has to name +// the port, not claim the Service is missing. +func TestApisixRouteReconcile_UnknownServicePortIsRejected(t *testing.T) { + r, prov, updater := newServicePortFixture(t, intstr.FromString("https"), + []corev1.ServicePort{{Name: "http", Port: 80, TargetPort: intstr.FromInt32(8080)}}) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: servicePortRouteKey}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "service port not found") + assert.NotContains(t, err.Error(), "service not found", + "the Service resolves; only the port does not") + assert.Zero(t, prov.updated) + + cond := acceptedCondition(t, updater) + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, string(apiv2.ConditionReasonInvalidSpec), cond.Reason) +} + +// A port that does resolve must still be published. +func TestApisixRouteReconcile_ResolvableServicePortIsPublished(t *testing.T) { + for name, port := range map[string]intstr.IntOrString{ + "by number": intstr.FromInt32(80), + "by name": intstr.FromString("http"), + } { + t.Run(name, func(t *testing.T) { + r, prov, updater := newServicePortFixture(t, port, + []corev1.ServicePort{{Name: "http", Port: 80, TargetPort: intstr.FromInt32(8080)}}) + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: servicePortRouteKey}) + + require.NoError(t, err) + assert.Equal(t, 1, prov.updated) + assert.Equal(t, metav1.ConditionTrue, acceptedCondition(t, updater).Status) + }) + } +}