From ee9458a62e634d84e31a07517a5a34752951a0df Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 22 Sep 2026 23:23:37 +0545 Subject: [PATCH] fix: reject HTTPRoutePolicy with malformed vars instead of dropping them A spec.vars item that failed to decode was logged and skipped, so the route was published with fewer match conditions than the policy declared. Translation now fails on the first malformed item, and the policy reports Accepted=False with reason Invalid. --- internal/adc/translator/httproute.go | 44 ++++-- .../adc/translator/httproutepolicy_test.go | 147 ++++++++++++++++++ internal/adc/translator/ingress.go | 4 +- internal/controller/httproutepolicy.go | 21 ++- internal/controller/httproutepolicy_test.go | 146 +++++++++++++++++ internal/controller/ingress_controller.go | 2 + 6 files changed, 347 insertions(+), 17 deletions(-) create mode 100644 internal/adc/translator/httproutepolicy_test.go create mode 100644 internal/controller/httproutepolicy_test.go diff --git a/internal/adc/translator/httproute.go b/internal/adc/translator/httproute.go index b651bc0e..3c818629 100644 --- a/internal/adc/translator/httproute.go +++ b/internal/adc/translator/httproute.go @@ -337,7 +337,7 @@ func (t *Translator) fillPluginFromHTTPRequestRedirectFilter(plugins adctypes.Pl plugin.URI = uri } -func (t *Translator) fillHTTPRoutePoliciesForHTTPRoute(tctx *provider.TranslateContext, routes []*adctypes.Route, rule gatewayv1.HTTPRouteRule) { +func (t *Translator) fillHTTPRoutePoliciesForHTTPRoute(tctx *provider.TranslateContext, routes []*adctypes.Route, rule gatewayv1.HTTPRouteRule) error { var policies []v1alpha1.HTTPRoutePolicy for _, policy := range tctx.HTTPRoutePolicies { for _, ref := range policy.Spec.TargetRefs { @@ -348,28 +348,40 @@ func (t *Translator) fillHTTPRoutePoliciesForHTTPRoute(tctx *provider.TranslateC } } - t.fillHTTPRoutePolicies(routes, policies) + return t.fillHTTPRoutePolicies(routes, policies) } -func (t *Translator) fillHTTPRoutePoliciesForIngress(tctx *provider.TranslateContext, routes []*adctypes.Route) { - t.fillHTTPRoutePolicies(routes, tctx.HTTPRoutePolicies) +func (t *Translator) fillHTTPRoutePoliciesForIngress(tctx *provider.TranslateContext, routes []*adctypes.Route) error { + return t.fillHTTPRoutePolicies(routes, tctx.HTTPRoutePolicies) } -func (t *Translator) fillHTTPRoutePolicies(routes []*adctypes.Route, policies []v1alpha1.HTTPRoutePolicy) { +// fillHTTPRoutePolicies fails on any malformed var: vars are AND-ed match +// conditions, so dropping one would widen the route. +func (t *Translator) fillHTTPRoutePolicies(routes []*adctypes.Route, policies []v1alpha1.HTTPRoutePolicy) error { for _, policy := range policies { + vars, err := ParseHTTPRoutePolicyVars(&policy) + if err != nil { + return err + } for _, route := range routes { route.Priority = policy.Spec.Priority - for _, data := range policy.Spec.Vars { - var v []adctypes.StringOrSlice - if err := json.Unmarshal(data.Raw, &v); err != nil { - t.Log.Error(err, "failed to unmarshal spec.Vars item to []StringOrSlice", "data", string(data.Raw)) - // todo: update status - continue - } - route.Vars = append(route.Vars, v) - } + route.Vars = append(route.Vars, vars...) } } + return nil +} + +// ParseHTTPRoutePolicyVars decodes spec.vars, failing on the first malformed item. +func ParseHTTPRoutePolicyVars(policy *v1alpha1.HTTPRoutePolicy) (adctypes.Vars, error) { + vars := make(adctypes.Vars, 0, len(policy.Spec.Vars)) + for i, data := range policy.Spec.Vars { + var v []adctypes.StringOrSlice + if err := json.Unmarshal(data.Raw, &v); err != nil { + return nil, fmt.Errorf("HTTPRoutePolicy %s/%s: invalid spec.vars[%d]: %w", policy.Namespace, policy.Name, i, err) + } + vars = append(vars, v) + } + return vars, nil } func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice, endpointFilter func(*discoveryv1.Endpoint) bool) adctypes.UpstreamNodes { @@ -806,7 +818,9 @@ func (t *Translator) TranslateHTTPRoute(tctx *provider.TranslateContext, httpRou } } - t.fillHTTPRoutePoliciesForHTTPRoute(tctx, routes, rule) + if err := t.fillHTTPRoutePoliciesForHTTPRoute(tctx, routes, rule); err != nil { + return nil, err + } service.Routes = routes result.Services = append(result.Services, service) diff --git a/internal/adc/translator/httproutepolicy_test.go b/internal/adc/translator/httproutepolicy_test.go new file mode 100644 index 00000000..c54f0c7e --- /dev/null +++ b/internal/adc/translator/httproutepolicy_test.go @@ -0,0 +1,147 @@ +// 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 translator + +import ( + "context" + "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" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + adctypes "github.com/apache/apisix-ingress-controller/api/adc" + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +const validPolicyVar = `["remote_addr","==","10.0.0.1"]` + +func policyWithVars(vars ...string) v1alpha1.HTTPRoutePolicy { + policy := v1alpha1.HTTPRoutePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "policy"}, + Spec: v1alpha1.HTTPRoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.GroupName, + Kind: "HTTPRoute", + Name: "route", + }, + }}, + }, + } + for _, v := range vars { + policy.Spec.Vars = append(policy.Spec.Vars, apiextensionsv1.JSON{Raw: []byte(v)}) + } + return policy +} + +func TestParseHTTPRoutePolicyVars(t *testing.T) { + policy := policyWithVars(validPolicyVar) + vars, err := ParseHTTPRoutePolicyVars(&policy) + require.NoError(t, err) + assert.Equal(t, adctypes.Vars{{{StrVal: "remote_addr"}, {StrVal: "=="}, {StrVal: "10.0.0.1"}}}, vars) + + for _, malformed := range []string{`{"remote_addr":"10.0.0.0/8"}`, `"remote_addr"`, `[{"a":"b"}]`, `[1]`} { + t.Run(malformed, func(t *testing.T) { + policy := policyWithVars(validPolicyVar, malformed) + _, err := ParseHTTPRoutePolicyVars(&policy) + assert.ErrorContains(t, err, "invalid spec.vars[1]") + }) + } +} + +func TestTranslateHTTPRoute_HTTPRoutePolicyVars(t *testing.T) { + pathType := gatewayv1.PathMatchPathPrefix + pathValue := "/" + httpRoute := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + Spec: gatewayv1.HTTPRouteSpec{ + Rules: []gatewayv1.HTTPRouteRule{{ + Matches: []gatewayv1.HTTPRouteMatch{{ + Path: &gatewayv1.HTTPPathMatch{Type: &pathType, Value: &pathValue}, + }}, + }}, + }, + } + + t.Run("valid vars are applied", func(t *testing.T) { + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.HTTPRoutePolicies = []v1alpha1.HTTPRoutePolicy{policyWithVars(validPolicyVar)} + + result, err := NewTranslator(logr.Discard(), "").TranslateHTTPRoute(tctx, httpRoute) + require.NoError(t, err) + require.Len(t, result.Services, 1) + require.Len(t, result.Services[0].Routes, 1) + assert.Contains(t, result.Services[0].Routes[0].Vars, + []adctypes.StringOrSlice{{StrVal: "remote_addr"}, {StrVal: "=="}, {StrVal: "10.0.0.1"}}) + }) + + t.Run("malformed var fails translation", func(t *testing.T) { + tctx := provider.NewDefaultTranslateContext(context.Background()) + tctx.HTTPRoutePolicies = []v1alpha1.HTTPRoutePolicy{ + policyWithVars(validPolicyVar, `{"remote_addr":"10.0.0.0/8"}`), + } + + _, err := NewTranslator(logr.Discard(), "").TranslateHTTPRoute(tctx, httpRoute) + assert.ErrorContains(t, err, "invalid spec.vars[1]") + }) +} + +func TestTranslateIngress_HTTPRoutePolicyMalformedVars(t *testing.T) { + pathType := networkingv1.PathTypePrefix + ingress := &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "ingress"}, + Spec: networkingv1.IngressSpec{ + Rules: []networkingv1.IngressRule{{ + IngressRuleValue: networkingv1.IngressRuleValue{HTTP: &networkingv1.HTTPIngressRuleValue{ + Paths: []networkingv1.HTTPIngressPath{{ + Path: "/", + PathType: &pathType, + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "svc", + Port: networkingv1.ServiceBackendPort{Number: 80}, + }, + }, + }}, + }}, + }}, + }, + } + tctx := &provider.TranslateContext{ + Services: map[types.NamespacedName]*corev1.Service{ + {Namespace: "default", Name: "svc"}: { + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "svc"}, + Spec: corev1.ServiceSpec{Ports: []corev1.ServicePort{{Port: 80}}}, + }, + }, + HTTPRoutePolicies: []v1alpha1.HTTPRoutePolicy{ + policyWithVars(validPolicyVar, `{"remote_addr":"10.0.0.0/8"}`), + }, + } + + _, err := NewTranslator(logr.Discard(), "").TranslateIngress(tctx, ingress) + assert.ErrorContains(t, err, "invalid spec.vars[1]") +} diff --git a/internal/adc/translator/ingress.go b/internal/adc/translator/ingress.go index f3e121fe..5a74bbbf 100644 --- a/internal/adc/translator/ingress.go +++ b/internal/adc/translator/ingress.go @@ -180,7 +180,9 @@ func (t *Translator) buildServiceFromIngressPath( } service.Routes = []*adctypes.Route{route} - t.fillHTTPRoutePoliciesForIngress(tctx, service.Routes) + if err := t.fillHTTPRoutePoliciesForIngress(tctx, service.Routes); err != nil { + return nil, err + } return service, nil } diff --git a/internal/controller/httproutepolicy.go b/internal/controller/httproutepolicy.go index 2a9510bc..aba6767a 100644 --- a/internal/controller/httproutepolicy.go +++ b/internal/controller/httproutepolicy.go @@ -30,6 +30,7 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/adc/translator" "github.com/apache/apisix-ingress-controller/internal/controller/indexer" "github.com/apache/apisix-ingress-controller/internal/controller/status" "github.com/apache/apisix-ingress-controller/internal/provider" @@ -83,7 +84,9 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC condition.Reason = string(gatewayv1.PolicyReasonConflicted) condition.Message = "HTTPRoutePolicy conflict with others target to the HTTPRoute" } else { + // Kept even when invalid so translation fails instead of dropping its vars. tctx.HTTPRoutePolicies = append(tctx.HTTPRoutePolicies, policy) + condition = invalidVarsCondition(&policy) } if updated := setAncestorsForHTTPRoutePolicyStatus(parentRefs, &policy, condition); updated { @@ -160,7 +163,11 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon for i := range list.Items { policy := list.Items[i] - if updated := setAncestorsForHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, condition); updated { + policyCondition := condition + if policyCondition.Status == "" { + policyCondition = invalidVarsCondition(&policy) + } + if updated := setAncestorsForHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, policyCondition); updated { tctx.StatusUpdaters = append(tctx.StatusUpdaters, status.Update{ NamespacedName: utils.NamespacedName(&policy), Resource: policy.DeepCopy(), @@ -218,6 +225,18 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Co return nil } +// invalidVarsCondition reports malformed spec.vars; zero value means accepted. +func invalidVarsCondition(policy *v1alpha1.HTTPRoutePolicy) metav1.Condition { + if _, err := translator.ParseHTTPRoutePolicyVars(policy); err != nil { + return metav1.Condition{ + Status: metav1.ConditionFalse, + Reason: string(gatewayv1.PolicyReasonInvalid), + Message: err.Error(), + } + } + return metav1.Condition{} +} + func setAncestorsForHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, condition metav1.Condition) bool { return SetAncestors(&policy.Status, parentRefs, metav1.Condition{ Type: cmp.Or(condition.Type, string(gatewayv1.PolicyConditionAccepted)), diff --git a/internal/controller/httproutepolicy_test.go b/internal/controller/httproutepolicy_test.go new file mode 100644 index 00000000..05b8e121 --- /dev/null +++ b/internal/controller/httproutepolicy_test.go @@ -0,0 +1,146 @@ +// 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" + "errors" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + + "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/indexer" + "github.com/apache/apisix-ingress-controller/internal/provider" +) + +func malformedVarsPolicy(kind, target string) *v1alpha1.HTTPRoutePolicy { + group := gatewayv1.GroupName + if kind == KindIngress { + group = networkingv1.GroupName + } + return &v1alpha1.HTTPRoutePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "policy"}, + Spec: v1alpha1.HTTPRoutePolicySpec{ + TargetRefs: []gatewayv1.LocalPolicyTargetReferenceWithSectionName{{ + LocalPolicyTargetReference: gatewayv1.LocalPolicyTargetReference{ + Group: gatewayv1.Group(group), + Kind: gatewayv1.Kind(kind), + Name: gatewayv1.ObjectName(target), + }, + }}, + Vars: []apiextensionsv1.JSON{ + {Raw: []byte(`["remote_addr","==","10.0.0.1"]`)}, + {Raw: []byte(`{"remote_addr":"10.0.0.0/8"}`)}, + }, + }, + } +} + +func policyClient(t *testing.T, objects ...client.Object) client.Client { + t.Helper() + return fake.NewClientBuilder().WithScheme(retractPluginConfigScheme(t)). + WithObjects(objects...). + WithIndex(&v1alpha1.HTTPRoutePolicy{}, indexer.PolicyTargetRefs, indexer.HTTPRoutePolicyIndexFunc). + Build() +} + +// requireInvalidPolicyStatus applies the recorded status update and checks it rejects the policy. +func requireInvalidPolicyStatus(t *testing.T, policy *v1alpha1.HTTPRoutePolicy, mutated client.Object) { + t.Helper() + got, ok := mutated.(*v1alpha1.HTTPRoutePolicy) + require.True(t, ok) + require.Len(t, got.Status.Ancestors, 1) + require.Len(t, got.Status.Ancestors[0].Conditions, 1) + cond := got.Status.Ancestors[0].Conditions[0] + assert.Equal(t, string(gatewayv1.PolicyConditionAccepted), cond.Type) + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, string(gatewayv1.PolicyReasonInvalid), cond.Reason) + assert.Contains(t, cond.Message, "invalid spec.vars[1]") + assert.Equal(t, policy.Name, got.Name) +} + +func TestHTTPRouteProcessHTTPRoutePolicies_MalformedVars(t *testing.T) { + policy := malformedVarsPolicy(KindHTTPRoute, "route") + route := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "route"}, + Spec: gatewayv1.HTTPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ParentRefs: []gatewayv1.ParentReference{{Name: "gw"}}}, + Rules: []gatewayv1.HTTPRouteRule{{}}, + }, + } + r := &HTTPRouteReconciler{Client: policyClient(t, policy.DeepCopy()), Log: logr.Discard()} + tctx := provider.NewDefaultTranslateContext(context.Background()) + + require.NoError(t, r.processHTTPRoutePolicies(tctx, route)) + + // Kept so translation fails instead of publishing the route without its vars. + require.Len(t, tctx.HTTPRoutePolicies, 1) + require.Len(t, tctx.StatusUpdaters, 1) + requireInvalidPolicyStatus(t, policy, tctx.StatusUpdaters[0].Mutator.Mutate(policy.DeepCopy())) +} + +type failingUpdateProvider struct { + recordingProvider +} + +func (p *failingUpdateProvider) Update(context.Context, *provider.TranslateContext, client.Object) error { + p.updated++ + return errors.New("translation failed") +} + +func TestIngressReconcile_ReportsMalformedPolicyVars(t *testing.T) { + policy := malformedVarsPolicy(KindIngress, "ing") + ingress := &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "ing"}, + Spec: networkingv1.IngressSpec{IngressClassName: ptrTo("apisix")}, + } + cli := policyClient(t, retractIngressClass(), ingress, policy.DeepCopy()) + updater := &recordingUpdater{} + prov := &failingUpdateProvider{} + r := &IngressReconciler{ + Client: cli, + Scheme: retractPluginConfigScheme(t), + Log: logr.Discard(), + Provider: prov, + Updater: updater, + Readier: newRetractReadier(t, cli), + } + + _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(ingress)}) + + require.Error(t, err) + assert.Equal(t, 1, prov.updated) + var found bool + for _, u := range updater.updates { + if _, ok := u.Resource.(*v1alpha1.HTTPRoutePolicy); ok { + found = true + requireInvalidPolicyStatus(t, policy, u.Mutator.Mutate(policy.DeepCopy())) + } + } + assert.True(t, found, "policy status must be reported even though the Ingress update failed") +} diff --git a/internal/controller/ingress_controller.go b/internal/controller/ingress_controller.go index 402f2770..535ff641 100644 --- a/internal/controller/ingress_controller.go +++ b/internal/controller/ingress_controller.go @@ -238,6 +238,8 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // update the ingress resources if err := r.Provider.Update(ctx, tctx, ingress); err != nil { r.Log.Error(err, "failed to update ingress resources", "ingress", ingress.Name) + // Still report policy status, e.g. an HTTPRoutePolicy with invalid vars. + UpdateStatus(r.Updater, r.Log, tctx) return ctrl.Result{}, err }