tokers commented on a change in pull request #573:
URL:
https://github.com/apache/apisix-ingress-controller/pull/573#discussion_r693591456
##########
File path: Makefile
##########
@@ -106,6 +106,13 @@ ifeq ($(E2E_SKIP_BUILD), 0)
docker push $(LOCAL_REGISTRY)/jmalloc/echo-server:latest
endif
+# rebuild ingress controller image and push to the local registry for e2e
tests.
Review comment:
Why reduplicate the same contents that were implemented in another make
directive (`push-images-to-kind`)?
##########
File path: pkg/api/server.go
##########
@@ -41,23 +46,43 @@ func NewServer(cfg *config.Config) (*Server, error) {
return nil, err
}
gin.SetMode(gin.ReleaseMode)
- router := gin.New()
- router.Use(gin.Recovery(), gin.Logger())
- apirouter.Mount(router)
+ httpServer := gin.New()
+ httpServer.Use(gin.Recovery(), gin.Logger())
+ apirouter.Mount(httpServer)
srv := &Server{
- router: router,
+ httpServer: httpServer,
httpListener: httpListener,
}
-
if cfg.EnableProfiling {
srv.pprofMu = new(http.ServeMux)
srv.pprofMu.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
srv.pprofMu.HandleFunc("/debug/pprof/profile", pprof.Profile)
srv.pprofMu.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
srv.pprofMu.HandleFunc("/debug/pprof/trace", pprof.Trace)
srv.pprofMu.HandleFunc("/debug/pprof/", pprof.Index)
- router.GET("/debug/pprof/*profile",
gin.WrapF(srv.pprofMu.ServeHTTP))
+ httpServer.GET("/debug/pprof/*profile",
gin.WrapF(srv.pprofMu.ServeHTTP))
+ }
+
+ cert, err := tls.LoadX509KeyPair(cfg.CertFilePath, cfg.KeyFilePath)
Review comment:
Should it be optional? Only enable TLS if the certificate and private
key are specified simultaneously.
##########
File path: pkg/api/validation/utils.go
##########
@@ -0,0 +1,89 @@
+// 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 validation
+
+import (
+ "context"
+ "fmt"
+ "sync"
+
+ "github.com/xeipuuv/gojsonschema"
+
+ "github.com/apache/apisix-ingress-controller/pkg/apisix"
+ "github.com/apache/apisix-ingress-controller/pkg/config"
+ "github.com/apache/apisix-ingress-controller/pkg/log"
+)
+
+var (
+ once sync.Once
+ onceErr error
+ schemaClient apisix.Schema
+)
+
+// GetSchemaClient returns a Schema client in the singleton way.
+// It can query the schema of objects from APISIX.
+func GetSchemaClient() (apisix.Schema, error) {
+ once.Do(func() {
+ client, err := apisix.NewClient()
+ if err != nil {
+ onceErr = err
+ return
+ }
+
+ cfg := config.NewDefaultConfig()
+ if err := cfg.Validate(); err != nil {
+ onceErr = err
+ return
+ }
+
+ clusterOpts := &apisix.ClusterOptions{
+ Name: cfg.APISIX.DefaultClusterName,
+ AdminKey: cfg.APISIX.DefaultClusterAdminKey,
+ BaseURL: cfg.APISIX.DefaultClusterBaseURL,
+ }
+ if err := client.AddCluster(context.TODO(), clusterOpts); err
!= nil {
+ onceErr = err
+ return
+ }
+
+ schemaClient =
client.Cluster(cfg.APISIX.DefaultClusterName).Schema()
+ })
+ return schemaClient, onceErr
+}
+
+// TODO: make this helper function more generic so that it can be used by
other validating webhooks.
+func validateSchema(schema string, config interface{}) (error,
[]gojsonschema.ResultError) {
+ schemaLoader := gojsonschema.NewStringLoader(schema)
Review comment:
Schema loader can be cached (in the future).
##########
File path: pkg/api/server_test.go
##########
@@ -25,17 +27,85 @@ import (
"github.com/apache/apisix-ingress-controller/pkg/config"
)
+const (
+ certFileName = "cert.pem"
Review comment:
You may try to use `ioutil.TempFile` to generate different files,
instead of specifying filename by yourself.
##########
File path: cmd/ingress/ingress.go
##########
@@ -139,6 +139,7 @@ the apisix cluster and others are created`,
cmd.PersistentFlags().StringVar(&cfg.LogLevel, "log-level", "info",
"error log level")
cmd.PersistentFlags().StringVar(&cfg.LogOutput, "log-output", "stderr",
"error log output file")
cmd.PersistentFlags().StringVar(&cfg.HTTPListen, "http-listen",
":8080", "the HTTP Server listen address")
+ cmd.PersistentFlags().StringVar(&cfg.HTTPSListen, "https-listen",
":443", "the HTTPS Server listen address")
Review comment:
We'd better not to use the well-known `443` port here:
1. Listen on `443` requiring root permission
2. it's not consistent with the HTTP listen (`8080`).
##########
File path: pkg/api/validation/apisix_route.go
##########
@@ -0,0 +1,152 @@
+// 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 validation
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "net/http"
+ "strings"
+
+ kwhhttp "github.com/slok/kubewebhook/v2/pkg/http"
+ kwhmodel "github.com/slok/kubewebhook/v2/pkg/model"
+ kwhvalidating "github.com/slok/kubewebhook/v2/pkg/webhook/validating"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ "github.com/apache/apisix-ingress-controller/pkg/apisix"
+ v1
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v1"
+
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2alpha1"
+
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2beta1"
+ "github.com/apache/apisix-ingress-controller/pkg/log"
+)
+
+func NewPluginValidatorHandler() http.Handler {
Review comment:
Add some comments for this function.
##########
File path: pkg/api/server.go
##########
@@ -70,10 +95,29 @@ func (srv *Server) Run(stopCh <-chan struct{}) error {
if err := srv.httpListener.Close(); err != nil {
log.Errorf("failed to close http listener: %s", err)
}
+
+ if srv.admissionServer != nil {
+ if err := srv.admissionServer.Shutdown(context.TODO());
err != nil {
Review comment:
Add the timeout limitation.
##########
File path: pkg/api/validation/utils.go
##########
@@ -0,0 +1,89 @@
+// 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 validation
+
+import (
+ "context"
+ "fmt"
+ "sync"
+
+ "github.com/xeipuuv/gojsonschema"
+
+ "github.com/apache/apisix-ingress-controller/pkg/apisix"
+ "github.com/apache/apisix-ingress-controller/pkg/config"
+ "github.com/apache/apisix-ingress-controller/pkg/log"
+)
+
+var (
+ once sync.Once
+ onceErr error
+ schemaClient apisix.Schema
+)
+
+// GetSchemaClient returns a Schema client in the singleton way.
+// It can query the schema of objects from APISIX.
+func GetSchemaClient() (apisix.Schema, error) {
Review comment:
Why fetch schema in the singleton way, schema is synchronized
periodically.
##########
File path: test/e2e/ingress/webhook.go
##########
@@ -0,0 +1,67 @@
+// 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 ingress
+
+import (
+ "fmt"
+
+ "github.com/onsi/ginkgo"
+ "github.com/stretchr/testify/assert"
+
+ "github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
+)
+
+var _ = ginkgo.Describe("Enable webhooks", func() {
+ opts := &scaffold.Options{
+ Name: "default",
+ Kubeconfig: scaffold.GetKubeconfig(),
+ APISIXConfigPath: "testdata/apisix-gw-config.yaml",
+ IngressAPISIXReplicas: 1,
+ HTTPBinServicePort: 80,
+ APISIXRouteVersion: "apisix.apache.org/v2alpha1",
+ EnableWebhooks: true,
+ }
+ s := scaffold.NewScaffold(opts)
+
+ ginkgo.It("should fail to create the ApisixRoute with invalid plugin
configuration", func() {
+ backendSvc, backendPorts := s.DefaultHTTPBackend()
+ ar := fmt.Sprintf(`
+apiVersion: apisix.apache.org/v2alpha1
+kind: ApisixRoute
+metadata:
+ name: httpbin-route
+spec:
+ http:
+ - name: rule1
+ match:
+ hosts:
+ - httpbin.org
+ paths:
+ - /status/*
+ backends:
+ - serviceName: %s
+ servicePort: %d
+ resolveGranularity: service
+ plugins:
+ - name: api-breaker
+ enable: true
+ config:
+ break_response_code: 100 # should in [200, 599]
+`, backendSvc, backendPorts[0])
+
+ assert.Error(ginkgo.GinkgoT(), s.CreateResourceFromString(ar),
"Failed to create ApisixRoute")
Review comment:
Also check out the error message.
##########
File path: pkg/api/validation/apisix_route.go
##########
@@ -0,0 +1,152 @@
+// 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 validation
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "net/http"
+ "strings"
+
+ kwhhttp "github.com/slok/kubewebhook/v2/pkg/http"
+ kwhmodel "github.com/slok/kubewebhook/v2/pkg/model"
+ kwhvalidating "github.com/slok/kubewebhook/v2/pkg/webhook/validating"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ "github.com/apache/apisix-ingress-controller/pkg/apisix"
+ v1
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v1"
+
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2alpha1"
+
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2beta1"
+ "github.com/apache/apisix-ingress-controller/pkg/log"
+)
+
+func NewPluginValidatorHandler() http.Handler {
+ // Create a validating webhook.
+ wh, err := kwhvalidating.NewWebhook(kwhvalidating.WebhookConfig{
+ ID: "apisixRoute-plugin",
+ Validator: pluginValidator,
+ })
+ if err != nil {
+ log.Errorf("failed to create webhook: %s", err)
+ }
+
+ h, err := kwhhttp.HandlerFor(kwhhttp.HandlerConfig{Webhook: wh})
+ if err != nil {
+ log.Errorf("failed to create webhook handle: %s", err)
+ }
+
+ return h
+}
+
+// ErrNotApisixRoute will be used when the validating object is not
ApisixRoute.
+var ErrNotApisixRoute = errors.New("object is not ApisixRoute")
+
+type apisixRoutePlugin struct {
+ Name string
+ Config interface{}
+}
+
+// pluginValidator validates plugins in ApisixRoute.
+// When the validation of one plugin fails, it will continue to validate the
rest of plugins.
+var pluginValidator = kwhvalidating.ValidatorFunc(
+ func(ctx context.Context, review *kwhmodel.AdmissionReview, object
metav1.Object) (result *kwhvalidating.ValidatorResult, err error) {
+ log.Debug("arrive plugin validator webhook")
+
+ valid := true
+ var plugins []apisixRoutePlugin
+
+ switch ar := object.(type) {
+ case *v2beta1.ApisixRoute:
+ for _, h := range ar.Spec.HTTP {
+ for _, p := range h.Plugins {
+ // only check plugins that are enabled.
+ if p.Enable {
+ plugins = append(plugins,
apisixRoutePlugin{
+ p.Name, p.Config,
+ })
+ }
+ }
+ }
+ case *v2alpha1.ApisixRoute:
+ for _, h := range ar.Spec.HTTP {
+ for _, p := range h.Plugins {
+ if p.Enable {
+ plugins = append(plugins,
apisixRoutePlugin{
+ p.Name, p.Config,
+ })
+ }
+ }
+ }
+ case *v1.ApisixRoute:
+ for _, r := range ar.Spec.Rules {
+ for _, path := range r.Http.Paths {
+ for _, p := range path.Plugins {
+ if p.Enable {
+ plugins =
append(plugins, apisixRoutePlugin{
+ p.Name,
p.Config,
+ })
+ }
+ }
+ }
+ }
+ default:
+ return &kwhvalidating.ValidatorResult{Valid: false,
Message: ErrNotApisixRoute.Error()}, ErrNotApisixRoute
+ }
+
+ client, err := GetSchemaClient()
+ if err != nil {
+ log.Errorf("failed to get the schema client: %s", err)
+ return &kwhvalidating.ValidatorResult{Valid: false,
Message: "failed to get the schema client"}, err
+ }
+
+ var msg []string
+ for _, p := range plugins {
+ if v, m, err := validatePlugin(client, p.Name,
p.Config); !v {
+ valid = false
+ msg = append(msg, m)
+ log.Warnf("failed to validate plugin %s: %s",
p.Name, err)
+ }
+ }
+
+ return &kwhvalidating.ValidatorResult{Valid: valid, Message:
strings.Join(msg, "\n")}, nil
+ })
+
+func validatePlugin(client apisix.Schema, pluginName string, pluginConfig
interface{}) (valid bool, msg string, err error) {
+ valid = true
+
+ pluginSchema, err := client.GetPluginSchema(context.TODO(), pluginName)
+ if err != nil {
+ log.Errorf("failed to get the schema of plugin %s: %s",
pluginName, err)
+ valid = false
+ msg = fmt.Sprintf("failed to get the schema of plugin %s",
pluginName)
+ return
+ }
+
+ var errorMsg []string
+ if err, re := validateSchema(pluginSchema.Content, pluginConfig); err
!= nil {
+ valid = false
+ msg = fmt.Sprintf("%s plugin's config is invalid\n", pluginName)
+ for _, desc := range re {
+ errorMsg = append(errorMsg, desc.String())
Review comment:
Use go-multierror package.
##########
File path: conf/config-default.yaml
##########
@@ -27,7 +27,11 @@ log_output: "stderr" # the output file path of error log,
default is stderr, whe
# are marshalled in JSON format, which can be parsed by
# programs easily.
+cert_file: "/etc/webhook/certs/cert.pem" # the TLS certificate file path.
+key_file: "/etc/webhook/certs/key.pem" # the TLS key file path.
+
http_listen: ":8080" # the HTTP Server listen address, default is ":8080"
+https_listen: ":443" # the HTTPS Server listen address, default is ":443"
Review comment:
Ditto.
--
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]
