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) + }) + } +}