Copilot commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4068429082
##########
internal/adc/cache/store.go:
##########
@@ -86,6 +100,52 @@ func gatewayProxyOf(name string) (types.NamespacedNameKind,
bool) {
return gatewayProxy, true
}
+func (s *Store) recordChange(name string, owner types.NamespacedNameKind) {
+ s.revision++
+ if s.lastChange[name] == nil {
+ s.lastChange[name] = make(map[types.NamespacedNameKind]uint64)
+ }
+ s.lastChange[name][owner] = s.revision
Review Comment:
These owner entries are never removed when an individual resource is
deleted; only `DeleteAll` drops the entire per-config map. A long-lived
GatewayProxy with resource-name churn therefore retains one key (including its
strings) for every historical owner, causing unbounded controller memory
growth. Please prune tombstones once no in-flight sync can refer to their
revisions, or otherwise bound this history.
##########
internal/provider/apisix/status.go:
##########
@@ -137,13 +174,39 @@ func (d *apisixProvider) classifySyncResult(
return gatewayProxyMsgs, failedEndpoints
}
+// dropUnit maps a rejected resource to what has to be dropped for the rest to
apply. A
+// service is dropped by itself. A route, stream route or named upstream is
dropped along
+// with the service it lives in: the service is what one Kubernetes rule
translates to,
+// and a named upstream is referenced by the service's traffic-split by id, so
dropping
+// it alone would leave that reference dangling. Nested resources are found
through their
+// parent, which also tells them apart when several services embed upstreams
of the same
+// id.
+func (d *apisixProvider) dropUnit(configName string, ev adctypes.StatusEvent)
(wireKey, exclusion, bool) {
+ serviceID := ev.ResourceID
+ switch ev.ResourceType {
+ case adctypes.TypeService:
+ case adctypes.TypeRoute, adctypes.TypeStreamRoute,
adctypes.TypeUpstream:
+ serviceID = ev.ParentID
+ default:
+ return wireKey{}, exclusion{}, false
+ }
+ service, ok := d.store.Lookup(configName, adctypes.TypeService,
serviceID)
+ if !ok {
+ return wireKey{}, exclusion{}, false
+ }
+ return wireKey{adctypes.TypeService, service.ID}, exclusion{owner:
service.Owner, name: service.Name}, true
Review Comment:
For a rejected route, this attributes the dropped service to the service
owner rather than to the rejected route's owner. The cache explicitly supports
routes whose own labels differ from their containing service
(`internal/adc/cache/store.go:160-176`), so in that case the wrong resource
gets `SyncFailed`, and editing the actual bad route will not clear the
exclusion. Keep dropping the parent service, but derive the exclusion owner
from the route event when available.
##########
internal/provider/apisix/status.go:
##########
@@ -58,23 +61,29 @@ const (
// get a True the first time, and what keeps a restart from leaving a stale
False stuck
// forever: the write only ever depends on this round's actual outcome.
//
-// Resource status can't afford the same full recompute: a config's resource
set can be
-// large, and rewriting every one of them every round even when nothing
changed would be
-// wasteful. So resources keep a small persisted delta in d.resourceFailures
instead:
-// newly (or still) failing resources are written SyncFailed, and any resource
that was
-// failing last round but isn't failing this one gets its error explicitly
cleared with
-// an Accepted write.
-func (d *apisixProvider) updateStatusFromSyncResults(ctx context.Context,
results map[string]types.ADCExecutionErrors) {
+// Resource status follows the whole skip table instead, since an excluded
resource stays
+// dropped until its owner is written again. A config's resource set can be
large, so
+// resources keep a small persisted delta in d.resourceFailures rather than
being rewritten
+// every round: resources that are dropped or failing are written SyncFailed,
and any
+// resource that was last round but isn't now gets its error explicitly
cleared with an
+// Accepted write.
+//
+// It reports whether anything was newly excluded, which is what makes the
next push
+// different from the one that just failed: see sync().
+func (d *apisixProvider) updateStatusFromSyncResults(ctx context.Context,
results map[string]types.ADCExecutionErrors, revisions map[string]uint64) bool {
resourceFailures := map[types.NamespacedNameKind][]string{}
+ newlyExcluded := 0
for configName, execErrs := range results {
+ dropped := map[wireKey]exclusion{}
+ gatewayProxyMsgs, failedEndpoints :=
d.classifySyncResult(configName, execErrs, revisions[configName], dropped,
resourceFailures)
+ newlyExcluded += d.skipped.MarkFailing(configName, dropped)
Review Comment:
`ChangedSince` and `MarkFailing` are separate critical sections. If a
reconcile writes fixed content after the stale check but before this insertion,
`applyResourceState` clears the old exclusions first and this code then re-adds
the stale exclusion; with no later content change, the fixed resource remains
excluded indefinitely. Serialize classification plus insertion with the same
provider lock used by `applyResourceState`/`removeResourceState`, or make the
revision check and insertion atomic.
##########
test/e2e/ingress/ingress.go:
##########
@@ -1600,4 +1600,115 @@ spec:
})
})
+
+ Context("Bad resource isolation", func() {
+ var gatewayProxy = `
+apiVersion: apisix.apache.org/v1alpha1
+kind: GatewayProxy
+metadata:
+ name: apisix-proxy-config
+ namespace: %s
+spec:
+ provider:
+ type: ControlPlane
+ controlPlane:
+ endpoints:
+ - %s
+ auth:
+ type: AdminKey
+ adminKey:
+ value: "%s"
+`
+ var ingressClass = `
+apiVersion: networking.k8s.io/v1
+kind: IngressClass
+metadata:
+ name: %s
+spec:
+ controller: "%s"
+ parameters:
+ apiGroup: "apisix.apache.org"
+ kind: "GatewayProxy"
+ name: "apisix-proxy-config"
+ namespace: "%s"
+ scope: "Namespace"
+`
+ // The Ingress picks up its plugins from an ApisixPluginConfig
named in an
+ // annotation; limit-count refuses a count that is not greater
than 0.
+ var rejectedPluginConfig = `
+apiVersion: apisix.apache.org/v2
+kind: ApisixPluginConfig
+metadata:
+ name: rejected-plugins
+spec:
+ ingressClassName: %s
+ plugins:
+ - name: limit-count
+ enable: true
+ config:
+ count: 0
+ time_window: 60
+ rejected_code: 503
+ key: remote_addr
+`
+ var ingressTemplate = `
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+ name: rejected
+%s
+spec:
+ ingressClassName: %s
+ rules:
+ - host: rejected-ingress.example.com
+ http:
+ paths:
+ - path: /
+ pathType: Prefix
+ backend:
+ service:
+ name: httpbin-service-e2e-test
+ port:
+ number: 80
+`
+ const withRejectedPlugins = ` annotations:
+ k8s.apisix.apache.org/plugin-config-name: rejected-plugins`
+
+ It("a rejected Ingress is reported as an event", func() {
+ By("create GatewayProxy")
+ err :=
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(gatewayProxy,
s.Namespace(), s.Deployer.GetAdminEndpoint(), s.AdminKey()), s.Namespace())
+ Expect(err).NotTo(HaveOccurred(), "creating
GatewayProxy")
+
+ By("create IngressClass")
+ err =
s.CreateResourceFromStringWithNamespace(fmt.Sprintf(ingressClass,
s.Namespace(), s.GetControllerName(), s.Namespace()), "")
+ Expect(err).NotTo(HaveOccurred(), "creating
IngressClass")
+
+ By("create an Ingress whose plugins the data plane
rejects")
+ err =
s.CreateResourceFromString(fmt.Sprintf(rejectedPluginConfig, s.Namespace()))
+ Expect(err).NotTo(HaveOccurred(), "creating
ApisixPluginConfig")
+ err =
s.CreateResourceFromString(fmt.Sprintf(ingressTemplate, withRejectedPlugins,
s.Namespace()))
+ Expect(err).NotTo(HaveOccurred(), "creating Ingress")
Review Comment:
This case creates only the rejected Ingress. It will still pass if that
rejection blocks the entire GatewayProxy, because it checks the event and only
sends traffic after fixing the Ingress. Add a valid sibling Ingress in the same
initial push and assert it is served before the fix to cover Ingress isolation
rather than only reporting and recovery.
##########
test/e2e/crds/v2/isolation.go:
##########
@@ -0,0 +1,255 @@
+// 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 v2
+
+import (
+ "fmt"
+ "net/http"
+
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
+ gomegatypes "github.com/onsi/gomega/types"
+
+ "github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
+)
+
+// Every case applies a valid and a rejected resource together, so the first
sync carries
+// both: the sync converges when the rejected one is excluded and the valid
one is served.
+// Fixing the rejected one then has to bring it in too.
+//
+// What makes a resource rejected is always something the data plane checks
for every
+// backend: a value outside the schema's range, or a known plugin configured
in a way its
+// own check_schema refuses. An unknown plugin name is not used, since
apisix-standalone
+// accepts those.
+var _ = Describe("Test bad resource isolation", Label("apisix.apache.org",
"v2", "isolation"), func() {
+ s := scaffold.NewDefaultScaffold()
+
+ // rejectedPlugin is a plugin every backend loads, configured with a
count the plugin
+ // itself refuses (it has to be greater than 0).
+ const rejectedPlugin = `
+ plugins:
+ - name: limit-count
+ enable: true
+ config:
+ count: 0
+ time_window: 60
+ rejected_code: 503
+ key: remote_addr
+`
+ // A consistent-hash load balancer needs a key to hash on. The data
plane checks that
+ // in code rather than in its schema, so the configuration gets past
ADC's schema
+ // check and is rejected only by the data plane itself.
+ const rejectedHashKey = ""
+ const acceptedHashKey = " key: remote_addr"
+ const acceptedPlugin = `
+ plugins:
+ - name: limit-count
+ enable: true
+ config:
+ count: 100
+ time_window: 60
+ rejected_code: 503
+ key: remote_addr
+`
+ // routes is one ApisixRoute serving valid.example.com and one serving
+ // rejected.example.com, whose plugin configuration is filled in per
case.
+ const routes = `
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: valid
+ namespace: %s
+spec:
+ ingressClassName: %s
+ http:
+ - name: rule0
+ match:
+ hosts:
+ - valid.example.com
+ paths:
+ - /*
+ backends:
+ - serviceName: httpbin-service-e2e-test
+ servicePort: 80
+---
+apiVersion: apisix.apache.org/v2
+kind: ApisixRoute
+metadata:
+ name: rejected
+ namespace: %s
+spec:
+ ingressClassName: %s
+ http:
+ - name: rule0
+ match:
+ hosts:
+ - rejected.example.com
+ paths:
+ - /*
+ backends:
+ - serviceName: httpbin-service-e2e-test
+ servicePort: 80
+%s
+`
+
+ expectServed := func(host string) {
+ s.RequestAssert(&scaffold.RequestAssert{
+ Method: "GET",
+ Path: "/get",
+ Host: host,
+ Check: scaffold.WithExpectedStatus(http.StatusOK),
+ })
+ }
+ expectStatus := func(resource, name string, matchers
...gomegatypes.GomegaMatcher) {
+ s.RetryAssertion(func() string {
+ output, _ := s.GetOutputFromString(resource, name,
"-o", "yaml", "-n", s.Namespace())
+ return output
+ }).Should(And(matchers...))
+ }
+ apply := func(yaml string) {
+ Expect(s.CreateResourceFromString(yaml)).NotTo(HaveOccurred(),
"applying resources")
+ }
+
+ BeforeEach(func() {
+ By("create GatewayProxy")
+
Expect(s.CreateResourceFromString(s.GetGatewayProxySpec())).NotTo(HaveOccurred(),
"creating GatewayProxy")
+
+ By("create IngressClass")
+
Expect(s.CreateResourceFromStringWithNamespace(s.GetIngressClassYaml(),
"")).NotTo(HaveOccurred(), "creating IngressClass")
+ })
+
+ It("isolates a rejected route", func() {
+ By("apply a valid and a rejected ApisixRoute")
+ apply(fmt.Sprintf(routes, s.Namespace(), s.Namespace(),
s.Namespace(), s.Namespace(), rejectedPlugin))
+
+ By("the valid ApisixRoute is served and the rejected one
reports why it is not")
+ expectServed("valid.example.com")
+ // The rejected route is the only one this ApisixRoute has, so
none of it is served.
+ expectStatus("ar", "rejected",
+ ContainSubstring(`status: "False"`),
+ ContainSubstring(`reason: SyncFailed`),
+ ContainSubstring(`limit-count`),
+ )
+
+ By("fix the rejected ApisixRoute")
+ apply(fmt.Sprintf(routes, s.Namespace(), s.Namespace(),
s.Namespace(), s.Namespace(), acceptedPlugin))
+
+ By("both ApisixRoutes are served")
+ expectServed("valid.example.com")
+ expectServed("rejected.example.com")
+ expectStatus("ar", "rejected", ContainSubstring(`reason:
Accepted`))
+ })
+
+ It("isolates a route whose upstream configuration is rejected", func() {
+ const upstream = `
+apiVersion: apisix.apache.org/v2
+kind: ApisixUpstream
+metadata:
+ name: httpbin-service-e2e-test
+ namespace: %s
+spec:
+ ingressClassName: %s
+ loadbalancer:
+ type: chash
+ hashOn: vars
+%s
+`
+ By("apply a valid and a rejected ApisixRoute, the rejected one
using a rejected upstream configuration")
+ apply(fmt.Sprintf(routes, s.Namespace(), s.Namespace(),
s.Namespace(), s.Namespace(), ""))
+ apply(fmt.Sprintf(upstream, s.Namespace(), s.Namespace(),
rejectedHashKey))
Review Comment:
This setup does not actually leave a valid control route: both manifests in
`routes` use `httpbin-service-e2e-test`, and an ApisixUpstream with that name
is applied to every such backend. Consequently both routes receive the rejected
hash configuration, and before the fix the test only checks one status without
verifying that any route is served. Use a different backend for the valid route
and call `expectServed` before fixing the upstream so the case fails if the
whole sync is blocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
