Repository: apache/apisix-ingress-controller

Issue:
bug: Block request method with Apisix Ingress Controller annotation
### Issue description

After implementing the annotation `k8s.apisix.apache.org/http-allow-methods` and `k8s.apisix.apache.org/http-block-method`, the affected Apisix Route has a plugin configuration as follows:

```
    "response-rewrite": {
      "body_base64": false,
      "status_code": 405,
      "vars": [
        [
          "request_method",
          "!",
          "in",
          [
            "GET",
            "POST",
            "PUT",
            "PATCH",
            "HEAD"
          ]
        ]
      ]
    }
```

The plugin is `response-rewrite`. This plugin just overwrites the response from the Upstream. Meaning that, any DELETE will succeed, and Apisix will just overwrite the HTTP to a 405.

Can [request-validation](https://apisix.apache.org/docs/apisix/plugins/request-validation/) plugin be used instead?

---
Related [issue](https://github.com/apache/apisix-ingress-controller/issues/1463)

### Environment

- your apisix-ingress-controller version (output of apisix-ingress-controller version --long):
```
/ingress-apisix # /ingress-apisix/apisix-ingress-controller version --long
Version: 1.6.0
Git SHA: no-git-module
Go Version: go1.19.4
Building OS/Arch: linux/amd64
Running OS/Arch: linux/amd64
```

- your Kubernetes cluster version (output of kubectl version):
```
Client Version: v1.25.4
Kustomize Version: v4.5.7
Server Version: v1.21.14-gke.3000
```

- if you run apisix-ingress-controller in Bare-metal environment, also show your OS version (uname -a):


### Minimal test code / Steps to reproduce

1. Create an ingress using Allow [http methods annotations](https://apisix.apache.org/docs/ingress-controller/concepts/annotations/#allow-http-methods)
2. Attempt to use an HTTP method that is it white listed

### Actual result

curl -I  -X DELETE https://my-domain.io/my-service/1
HTTP/2 405 
date: Thu, 19 Jan 2023 19:11:55 GMT
content-type: application/vnd.schemaregistry.v1+json
content-length: 4

### Error log

curl -I  -X DELETE https://my-domain.io/my-service/1
{"error_code":40404,"message":"Subject 1 was soft deleted.Set permanent=true to delete permanently"}

### Expected result

The  https://my-domain.io/my-service/1 should be present since it is not possible to DELETE

Discussion:
@tao12345666333: You mean the request should not be sent upstream, right?@hndoss: > You mean the request should not be sent upstream, right?

Yes, that is correct.@incubator4: Hello @hndoss , I am contributor of this feature, and I've already knew about this issue and would fix it soon later.@tao12345666333: Thanks @incubator4 @tao12345666333: @incubator4 just a friendly ping, do you have any updates?@hndoss: Hello! Is there any update on this?

Solution patch:
diff --git a/api/adc/types.go b/api/adc/types.go
index 8c2a9506..b18e6fdb 100644
--- a/api/adc/types.go
+++ b/api/adc/types.go
@@ -659,6 +659,15 @@ type ResponseRewriteConfig struct {
 	Filters      []map[string]string `json:"filters,omitempty" yaml:"filters,omitempty"`
 }
 
+type FaultInjectionConfig struct {
+	Abort *FaultInjectionAbortConfig `json:"abort,omitempty" yaml:"abort,omitempty"`
+}
+
+type FaultInjectionAbortConfig struct {
+	HTTPStatus int           `json:"http_status" yaml:"http_status"`
+	Vars       [][]expr.Expr `json:"vars,omitempty" yaml:"vars,omitempty"`
+}
+
 type ResponseHeaders struct {
 	Set    map[string]string `json:"set,omitempty" yaml:"set,omitempty"`
 	Add    []string          `json:"add,omitempty" yaml:"add,omitempty"`
diff --git a/internal/adc/translator/annotations/plugins/fault-injection.go b/internal/adc/translator/annotations/plugins/fault-injection.go
new file mode 100644
index 00000000..9e5ad09b
--- /dev/null
+++ b/internal/adc/translator/annotations/plugins/fault-injection.go
@@ -0,0 +1,64 @@
+// 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 plugins
+
+import (
+	"net/http"
+
+	"github.com/incubator4/go-resty-expr/expr"
+
+	adctypes "github.com/apache/apisix-ingress-controller/api/adc"
+	"github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations"
+)
+
+type FaultInjection struct{}
+
+// FaultInjection to APISIX fault-injection plugin.
+func NewFaultInjectionHandler() PluginAnnotationsHandler {
+	return &FaultInjection{}
+}
+
+func (h FaultInjection) PluginName() string {
+	return "fault-injection"
+}
+
+func (f FaultInjection) Handle(e annotations.Extractor) (any, error) {
+	var plugin adctypes.FaultInjectionConfig
+
+	allowMethods := e.GetStringsAnnotation(annotations.AnnotationsHttpAllowMethods)
+	blockMethods := e.GetStringsAnnotation(annotations.AnnotationsHttpBlockMethods)
+	if len(allowMethods) == 0 && len(blockMethods) == 0 {
+		return nil, nil
+	}
+	abort := &adctypes.FaultInjectionAbortConfig{
+		HTTPStatus: http.StatusMethodNotAllowed,
+	}
+	if len(allowMethods) > 0 {
+		abort.Vars = [][]expr.Expr{{
+			expr.StringExpr("request_method").Not().In(
+				expr.ArrayExpr(expr.ExprArrayFromStrings(allowMethods)...),
+			),
+		}}
+	} else {
+		abort.Vars = [][]expr.Expr{{
+			expr.StringExpr("request_method").In(
+				expr.ArrayExpr(expr.ExprArrayFromStrings(blockMethods)...),
+			),
+		}}
+	}
+	plugin.Abort = abort
+	return &plugin, nil
+}
diff --git a/internal/adc/translator/annotations/plugins/plugins.go b/internal/adc/translator/annotations/plugins/plugins.go
index fb0f0b27..2dd8e5f8 100644
--- a/internal/adc/translator/annotations/plugins/plugins.go
+++ b/internal/adc/translator/annotations/plugins/plugins.go
@@ -39,6 +39,7 @@ var (
 		NewRedirectHandler(),
 		NewCorsHandler(),
 		NewCSRFHandler(),
+		NewFaultInjectionHandler(),
 	}
 )
 
diff --git a/internal/adc/translator/apisixroute.go b/internal/adc/translator/apisixroute.go
index 1e130538..f5a509e1 100644
--- a/internal/adc/translator/apisixroute.go
+++ b/internal/adc/translator/apisixroute.go
@@ -486,6 +486,7 @@ func (t *Translator) translateStreamRule(tctx *provider.TranslateContext, ar *ap
 	svc.Name = adc.ComposeServiceNameWithStream(ar.Namespace, ar.Name, part.Name, part.Protocol)
 	svc.ID = id.GenID(svc.Name)
 	svc.StreamRoutes = append(svc.StreamRoutes, sr)
+	svc.Labels = label.GenLabel(ar)
 
 	upstream, err := t.translateApisixRouteStreamBackend(tctx, ar, part.Backend)
 	if err != nil {
diff --git a/internal/webhook/v1/ingress_webhook.go b/internal/webhook/v1/ingress_webhook.go
index 6f7a6229..78bb4908 100644
--- a/internal/webhook/v1/ingress_webhook.go
+++ b/internal/webhook/v1/ingress_webhook.go
@@ -57,8 +57,6 @@ var unsupportedAnnotations = []string{
 	"k8s.apisix.apache.org/auth-client-headers",
 	"k8s.apisix.apache.org/allowlist-source-range",
 	"k8s.apisix.apache.org/blocklist-source-range",
-	"k8s.apisix.apache.org/http-allow-methods",
-	"k8s.apisix.apache.org/http-block-methods",
 	"k8s.apisix.apache.org/auth-type",
 	"k8s.apisix.apache.org/svc-namespace",
 }



Test patch:
diff --git a/internal/adc/translator/annotations/plugins/fault_injection_test.go b/internal/adc/translator/annotations/plugins/fault_injection_test.go
new file mode 100644
index 00000000..4c7ab615
--- /dev/null
+++ b/internal/adc/translator/annotations/plugins/fault_injection_test.go
@@ -0,0 +1,61 @@
+// 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 plugins
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations"
+)
+
+func TestFaultInjectionHttpAllowMethods(t *testing.T) {
+	handler := NewFaultInjectionHandler()
+	assert.Equal(t, "fault-injection", handler.PluginName())
+
+	extractor := annotations.NewExtractor(map[string]string{
+		annotations.AnnotationsHttpAllowMethods: "GET,POST",
+	})
+
+	plugin, err := handler.Handle(extractor)
+	assert.NoError(t, err)
+	assert.NotNil(t, plugin)
+
+	data, err := json.Marshal(plugin)
+	assert.NoError(t, err)
+	assert.JSONEq(t, `{"abort":{"http_status":405,"vars":[[["request_method","!","in",["GET","POST"]]]]}}`, string(data))
+}
+
+func TestFaultInjectionHttpBlockMethods(t *testing.T) {
+	handler := NewFaultInjectionHandler()
+	assert.Equal(t, "fault-injection", handler.PluginName())
+
+	extractor := annotations.NewExtractor(map[string]string{
+		annotations.AnnotationsHttpBlockMethods: "GET,POST",
+	})
+
+	plugin, err := handler.Handle(extractor)
+	assert.NoError(t, err)
+	assert.NotNil(t, plugin)
+
+	data, err := json.Marshal(plugin)
+	assert.NoError(t, err)
+	assert.JSONEq(t, `{"abort":{"http_status":405,"vars":[[["request_method","in",["GET","POST"]]]]}}`, string(data))
+}
diff --git a/internal/adc/translator/annotations_test.go b/internal/adc/translator/annotations_test.go
index 4ff6bfe8..c94416d0 100644
--- a/internal/adc/translator/annotations_test.go
+++ b/internal/adc/translator/annotations_test.go
@@ -19,6 +19,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/incubator4/go-resty-expr/expr"
 	"github.com/stretchr/testify/assert"
 
 	adctypes "github.com/apache/apisix-ingress-controller/api/adc"
@@ -216,6 +217,46 @@ func TestTranslateIngressAnnotations(t *testing.T) {
 				EnableWebsocket: true,
 			},
 		},
+		{
+			name: "fault injection by allowed http methods",
+			anno: map[string]string{
+				annotations.AnnotationsHttpAllowMethods: "GET,POST",
+			},
+			expected: &IngressConfig{
+				Plugins: adctypes.Plugins{
+					"fault-injection": &adctypes.FaultInjectionConfig{
+						Abort: &adctypes.FaultInjectionAbortConfig{
+							HTTPStatus: 405,
+							Vars: [][]expr.Expr{{
+								expr.StringExpr("request_method").Not().In(
+									expr.ArrayExpr(expr.ExprArrayFromStrings([]string{"GET", "POST"})...),
+								),
+							}},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "fault injection by blocked http methods",
+			anno: map[string]string{
+				annotations.AnnotationsHttpBlockMethods: "DELETE",
+			},
+			expected: &IngressConfig{
+				Plugins: adctypes.Plugins{
+					"fault-injection": &adctypes.FaultInjectionConfig{
+						Abort: &adctypes.FaultInjectionAbortConfig{
+							HTTPStatus: 405,
+							Vars: [][]expr.Expr{{
+								expr.StringExpr("request_method").In(
+									expr.ArrayExpr(expr.ExprArrayFromStrings([]string{"DELETE"})...),
+								),
+							}},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/test/e2e/crds/v2/streamroute.go b/test/e2e/crds/v2/streamroute.go
index 47c1c0d0..d06105c7 100644
--- a/test/e2e/crds/v2/streamroute.go
+++ b/test/e2e/crds/v2/streamroute.go
@@ -18,6 +18,7 @@
 package v2
 
 import (
+	"context"
 	"fmt"
 	"time"
 
@@ -86,6 +87,20 @@ spec:
 					scaffold.WithExpectedBodyContains("x-my-value"),
 				},
 			})
+
+			err = s.DeleteResourceFromString(fmt.Sprintf(apisixRoute, s.Namespace()))
+			Expect(err).ShouldNot(HaveOccurred(), "deleting ApisixRoute")
+
+			s.RetryAssertion(func() error {
+				svcs, err := s.DefaultDataplaneResource().Service().List(context.Background())
+				if err != nil {
+					return err
+				}
+				if len(svcs) != 0 {
+					return fmt.Errorf("expected 0 services, got %d", len(svcs))
+				}
+				return nil
+			}).ShouldNot(HaveOccurred(), "waiting for services to be deleted")
 		})
 	})
 
diff --git a/test/e2e/ingress/annotations.go b/test/e2e/ingress/annotations.go
index 0a4857f5..ad559528 100644
--- a/test/e2e/ingress/annotations.go
+++ b/test/e2e/ingress/annotations.go
@@ -339,6 +339,49 @@ spec:
             name: httpbin-service-e2e-test
             port:
               number: 80
+`
+			allowMethods = `
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: allow-methods
+  annotations:
+    k8s.apisix.apache.org/http-allow-methods: "GET,POST"
+spec:
+  ingressClassName: %s
+  rules:
+  - host: httpbin.example
+    http:
+      paths:
+      - path: /anything
+        pathType: Exact
+        backend:
+          service:
+            name: httpbin-service-e2e-test
+            port:
+              number: 80
+`
+
+			blockMethods = `
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: block-methods
+  annotations:
+    k8s.apisix.apache.org/http-block-methods: "DELETE"
+spec:
+  ingressClassName: %s
+  rules:
+  - host: httpbin2.example
+    http:
+      paths:
+      - path: /anything
+        pathType: Exact
+        backend:
+          service:
+            name: httpbin-service-e2e-test
+            port:
+              number: 80
 `
 		)
 		BeforeEach(func() {
@@ -496,5 +539,76 @@ spec:
 			Expect(err).NotTo(HaveOccurred(), "unmarshalling echo plugin config")
 			Expect(echoConfig["body"]).To(Equal("hello from plugin config"), "checking echo plugin body")
 		})
+		It("methods", func() {
+			Expect(s.CreateResourceFromString(fmt.Sprintf(allowMethods, s.Namespace()))).ShouldNot(HaveOccurred(), "creating Ingress")
+			Expect(s.CreateResourceFromString(fmt.Sprintf(blockMethods, s.Namespace()))).ShouldNot(HaveOccurred(), "creating Ingress")
+
+			tets := []*scaffold.RequestAssert{
+				{
+					Method: "GET",
+					Path:   "/anything",
+					Host:   "httpbin.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "POST",
+					Path:   "/anything",
+					Host:   "httpbin.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "PUT",
+					Path:   "/anything",
+					Host:   "httpbin.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusMethodNotAllowed),
+				},
+				{
+					Method: "PATCH",
+					Path:   "/anything",
+					Host:   "httpbin.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusMethodNotAllowed),
+				},
+				{
+					Method: "DELETE",
+					Path:   "/anything",
+					Host:   "httpbin.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusMethodNotAllowed),
+				},
+				{
+					Method: "GET",
+					Path:   "/anything",
+					Host:   "httpbin2.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "POST",
+					Path:   "/anything",
+					Host:   "httpbin2.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "PUT",
+					Path:   "/anything",
+					Host:   "httpbin2.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "PATCH",
+					Path:   "/anything",
+					Host:   "httpbin2.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusOK),
+				},
+				{
+					Method: "DELETE",
+					Path:   "/anything",
+					Host:   "httpbin2.example",
+					Check:  scaffold.WithExpectedStatus(http.StatusMethodNotAllowed),
+				},
+			}
+
+			for _, test := range tets {
+				s.RequestAssert(test)
+			}
+		})
 	})
 })
diff --git a/test/e2e/scaffold/assertion.go b/test/e2e/scaffold/assertion.go
index a7a18246..543780d7 100644
--- a/test/e2e/scaffold/assertion.go
+++ b/test/e2e/scaffold/assertion.go
@@ -18,6 +18,7 @@
 package scaffold
 
 import (
+	"encoding/json"
 	"fmt"
 	"io"
 	"net"
@@ -62,25 +63,26 @@ type HTTPResponse struct {
 }
 
 type BasicAuth struct {
-	Username string
-	Password string
+	Username string `json:"username"`
+	Password string `json:"password"`
 }
 
 type RequestAssert struct {
-	Client    *httpexpect.Expect
-	Method    string
-	Path      string
-	Host      string
-	Query     map[string]any
-	Headers   map[string]string
-	Body      []byte
-	BasicAuth *BasicAuth
-
-	Timeout  time.Duration
-	Interval time.Duration
-
-	Check  ResponseCheckFunc
-	Checks []ResponseCheckFunc
+	Method    string            `json:"method,omitempty"`
+	Path      string            `json:"path,omitempty"`
+	Host      string            `json:"host,omitempty"`
+	Query     map[string]any    `json:"query,omitempty"`
+	Headers   map[string]string `json:"headers,omitempty"`
+	Body      []byte            `json:"body,omitempty"`
+	BasicAuth *BasicAuth        `json:"basic_auth,omitempty"`
+
+	Client *httpexpect.Expect `json:"-"`
+
+	Timeout  time.Duration `json:"-"`
+	Interval time.Duration `json:"-"`
+
+	Check  ResponseCheckFunc   `json:"-"`
+	Checks []ResponseCheckFunc `json:"-"`
 }
 
 func (c *RequestAssert) request(method, path string, body []byte) *httpexpect.Request {
@@ -308,7 +310,8 @@ func (s *Scaffold) RequestAssert(r *RequestAssert) bool {
 
 		for _, check := range r.Checks {
 			if err := check(resp); err != nil {
-				return fmt.Errorf("response check failed: %w", err)
+				req, _ := json.MarshalIndent(r, "", "  ")
+				return fmt.Errorf("response check failed for request %s: %v", string(req), err)
 			}
 		}
 		return nil