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 }