bzp2010 commented on code in PR #2890:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2890#discussion_r4080935628
##########
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:
Fixed in 63068bfa: MarkFailing now takes a stale callback and checks it
inside the same lock that performs the write, instead of checking ChangedSince
beforehand in a separate critical section.
##########
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:
Fixed in 63068bfa: dropUnit now looks up the route's own owner via
store.Lookup(configName, TypeRoute, ev.ResourceID) and uses it when the route
has labels of its own, falling back to the service's owner otherwise.
TypeStreamRoute/TypeUpstream still fall back to the service's owner since there
is no per-nested-owner lookup for those yet; tracked separately.
##########
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:
Fixed in 63068bfa: the rejected route now gets its own backend (an alias
Service, same pattern as streamroute.go's Bad resource isolation test) instead
of sharing httpbin-service-e2e-test with the valid route, and the test asserts
the valid route stays served right after applying the bad upstream config,
before the fix.
##########
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:
Fixed in 63068bfa: added a sibling valid Ingress created alongside the
rejected one, and the test asserts it stays served both while the rejected
Ingress is failing and after it is fixed.
--
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]
