feat: Add pprof.enabled configuration flag for /debug/pprof APIs (#505)…

… (#509)

In order to disable access in production environments, a new filter
was added to disable access when the configuration is set to any
non true value. When changing the value the filter will pass and
go tool pprof can be used directly

config/config_suite_test.go

@@ -21,9 +21,20 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
+	"knative.dev/pkg/logging"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+)
+
+var (
+	log, _ = logging.NewLogger("", "debug")
 )
 
 func TestConfig(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Config Suite")
 }
+
+var _ = BeforeSuite(func() {
+	log = zap.NewRaw(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)).Sugar()
+})

config/feature_flags.go

@@ -51,6 +51,9 @@ const (
 	// PolicyCheckEnabledFeatureKey indicates the configuration key of the policy check feature gate.
 	// If the value is true, the feature is enabled cluster-wide.
 	PolicyCheckEnabledFeatureKey = "policy.check.enabled"
+
+	// PprofEnabledKey indicates the configuration key of the /debug/pprof debugging api/
+	PprofEnabledKey = "pprof.enabled"
 )
 
 const (
@@ -89,6 +92,10 @@ const (
 	// DefaultPolicyCheckEnabled indicates the default value of the policy check feature gate.
 	// If the corresponding key does not exist, the default value is returned.
 	DefaultPolicyCheckEnabled FeatureValue = "true"
+
+	// DefaultPprofEnabled stores the default value "false" for the "pprof.enabled" /debug/pprof debugging api.
+	// If the corresponding key does not exist, the default value is returned.
+	DefaultPprofEnabled FeatureValue = "false"
 )
 
 // defaultFeatureValue defines the default value for the feature switch.
@@ -103,6 +110,7 @@ var defaultFeatureValue = map[string]FeatureValue{
 	TemplateRenderRetentionTimeKey:         DefaultTemplateRenderRetentionTime,
 	PolicyRunRetentionTimeKey:              DefaultPolicyRunRetentionTime,
 	PolicyCheckEnabledFeatureKey:           DefaultPolicyCheckEnabled,
+	PprofEnabledKey:                        DefaultPprofEnabled,
 }
 
 // FeatureFlags holds the features configurations

config/rest_filter.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2023 The Katanomi Authors.
+
+Licensed 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 config
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+
+	restful "github.com/emicklei/go-restful/v3"
+	"github.com/katanomi/pkg/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"knative.dev/pkg/logging"
+)
+
+// ConfigKeyExpectedValueFunc is a helper function to check if configmap has expected value
+// If the value is not as expected, an error is expected to be returned
+type ConfigKeyExpectedValueFunc func(ctx context.Context, req *restful.Request, key string, value FeatureValue) (err error)
+
+// ConfigFilter adds a restful filter to manager to watch configmap and and custom validation
+// according a specific key value pair.
+func ConfigFilter(ctx context.Context, manager *Manager, configKey string, expectedKeyValueFunc ConfigKeyExpectedValueFunc) func(*restful.Request, *restful.Response, *restful.FilterChain) {
+	return func(req *restful.Request, res *restful.Response, chain *restful.FilterChain) {
+		featureValue := manager.GetFeatureFlag(configKey)
+		if err := expectedKeyValueFunc(ctx, req, configKey, featureValue); err != nil {
+			log := logging.FromContext(ctx)
+			log.Debugw("Error in ConfigFilter, will return", "err", err, "code", res.StatusCode())
+			errors.HandleError(req, res, err)
+			return
+		}
+		chain.ProcessFilter(req, res)
+	}
+}
+
+// ConfigFilterNotFoundWhenNotTrue is a helper ConfigKeyExpectedValue implementation that checks if the value is a boolean true
+// value, if not true will return a standard 404 not found error
+func ConfigFilterNotFoundWhenNotTrue(ctx context.Context, req *restful.Request, key string, value FeatureValue) (err error) {
+	if ok, _ := value.AsBool(); !ok {
+		return apierrors.NewGenericServerResponse(
+			http.StatusNotFound,
+			req.Request.Method,
+			errors.RESTAPIGroupResource,
+			req.Request.URL.String(),
+			fmt.Sprintf("%s Not Found", req.Request.URL.String()),
+			0,
+			false,
+		)
+	}
+	return nil
+}

config/rest_filter_test.go

@@ -0,0 +1,128 @@
+/*
+Copyright 2023 The Katanomi Authors.
+
+Licensed 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 config
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+
+	restful "github.com/emicklei/go-restful/v3"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"knative.dev/pkg/logging"
+)
+
+var _ = Describe("ConfigFilter", func() {
+
+	var (
+		manager              *Manager
+		ctx                  context.Context
+		request              *restful.Request
+		response             *restful.Response
+		chain                *restful.FilterChain
+		recorder             *httptest.ResponseRecorder
+		key                  string
+		expectedKeyValueFunc ConfigKeyExpectedValueFunc
+	)
+
+	BeforeEach(func() {
+		ctx = context.Background()
+		ctx = logging.WithLogger(ctx, log)
+		req := &http.Request{
+			Header: map[string][]string{
+				restful.HEADER_AcceptEncoding: []string{restful.MIME_JSON},
+			},
+		}
+		testUrl, _ := url.Parse("http://test.example/some/path")
+		req.URL = testUrl
+		request = &restful.Request{Request: req}
+		recorder = httptest.NewRecorder()
+		response = &restful.Response{ResponseWriter: recorder}
+		response.SetRequestAccepts(restful.MIME_JSON)
+		chain = &restful.FilterChain{
+			Filters: []restful.FilterFunction{},
+			Target: func(req *restful.Request, resp *restful.Response) {
+				resp.WriteHeader(http.StatusOK)
+			},
+		}
+
+		manager = &Manager{Config: &Config{Data: map[string]string{"test": "test"}}}
+		key = "test"
+	})
+
+	JustBeforeEach(func() {
+		request.Request = request.Request.WithContext(ctx)
+
+		ConfigFilter(ctx, manager, key, expectedKeyValueFunc)(request, response, chain)
+	})
+
+	Context("Uses a \"test\" key with \"test\" value using some basic ConfigKeyExpectedValueFunc implementation", func() {
+		BeforeEach(func() {
+			expectedKeyValueFunc = func(ctx context.Context, req *restful.Request, key string, value FeatureValue) (err error) {
+				ok, err := value.AsBool()
+				if err != nil {
+					return err
+				} else if !ok {
+					return fmt.Errorf("value is not true: %v", value)
+				}
+				return nil
+			}
+		})
+		It("should have a internal error as status code with api error in response body", func() {
+			Expect(recorder.Code).To(Equal(http.StatusInternalServerError))
+			Expect(strings.TrimSpace(recorder.Body.String())).To(Equal(`{"metadata":{},"status":"Failure","message":"Internal error occurred: failed parsing feature flags config \"test\": strconv.ParseBool: parsing \"test\": invalid syntax","reason":"InternalError","details":{"causes":[{"message":"failed parsing feature flags config \"test\": strconv.ParseBool: parsing \"test\": invalid syntax"}]},"code":500}`))
+		})
+
+		Context("Uses a \"test\" key with \"true\" value using some basic ConfigKeyExpectedValueFunc implementation", func() {
+			BeforeEach(func() {
+				manager.Config.Data["test"] = "true"
+			})
+			It("should pass filter", func() {
+				Expect(recorder.Code).To(Equal(http.StatusOK))
+			})
+		})
+	})
+
+	Context("Uses ConfigFilterNotFoundWhenNotTrue with false value", func() {
+		BeforeEach(func() {
+			expectedKeyValueFunc = ConfigFilterNotFoundWhenNotTrue
+			manager.Config.Data["test"] = "false"
+		})
+
+		It("should have a not found error as status code with api error in response body", func() {
+			Expect(recorder.Code).To(Equal(http.StatusNotFound))
+			Expect(strings.TrimSpace(recorder.Body.String())).To(Equal(`{"metadata":{},"status":"Failure","message":"the server could not find the requested resource ( API.katanomi.dev http://test.example/some/path)","reason":"NotFound","details":{"name":"http://test.example/some/path","group":"katanomi.dev","kind":"API"},"code":404}`))
+		})
+
+	})
+
+	Context("Uses ConfigFilterNotFoundWhenNotTrue with true value", func() {
+		BeforeEach(func() {
+			expectedKeyValueFunc = ConfigFilterNotFoundWhenNotTrue
+			manager.Config.Data["test"] = "true"
+		})
+
+		It("should pass filter", func() {
+			Expect(recorder.Code).To(Equal(http.StatusOK))
+		})
+
+	})
+})

errors/errorhandling.go

@@ -31,6 +31,9 @@ import (
 // RESTClientGroupResource fake GroupResource to use errors api
 var RESTClientGroupResource = schema.GroupResource{Group: "katanomi.dev", Resource: "RESTfulClient"}
 
+// RESTAPIGroupResource fake GroupResource to use errors api
+var RESTAPIGroupResource = schema.GroupResource{Group: "katanomi.dev", Resource: "API"}
+
 // AsAPIError returns an error as a apimachinary api error
 func AsAPIError(err error) error {
 	reason := errors.ReasonForError(err)

plugin/plugin.go

@@ -44,6 +44,9 @@ type plugin struct {
 
 type ShutdownFunc func() error
 
+// NewPlugin method never used. was supposed to initiate plugins
+// Deprecated: This plugin initializer has never been used and is not recommended
+// TODO: cleanup unnecessary code
 func NewPlugin() *plugin {
 	plugin := &plugin{
 		container: restful.NewContainer(),
@@ -78,7 +81,9 @@ func (p *plugin) prepare() {
 		}
 	}
 
-	p.container.Add(route.NewDefaultService())
+	// this plugin initialization process was never really used in production
+	// and is not recommended, so here we just give any context to the constructor
+	p.container.Add(route.NewDefaultService(context.Background()))
 
 	for _, each := range p.clients {
 		ws, err := route.NewService(each)

plugin/route/healthz.go

@@ -17,6 +17,7 @@ limitations under the License.
 package route
 
 import (
+	"context"
 	"net/http"
 
 	"github.com/emicklei/go-restful/v3"
@@ -26,7 +27,7 @@ type healthz struct {
 }
 
 // NewHealthz basic health check service
-func NewHealthz() Route {
+func NewHealthz(ctx context.Context) Route {
 	return &healthz{}
 }
 

plugin/route/helper.go

@@ -35,3 +35,8 @@ func wrapperH(handler http.Handler) restful.RouteFunction {
 		handler.ServeHTTP(response.ResponseWriter, request.Request)
 	}
 }
+
+// NoOpFilter creates a default no operation filter
+func NoOpFilter(req *restful.Request, res *restful.Response, chain *restful.FilterChain) {
+	chain.ProcessFilter(req, res)
+}

plugin/route/route.go

@@ -499,10 +499,10 @@ func NewService(c client.Interface, filters ...restful.FilterFunction) (*restful
 }
 
 // NewDefaultService default service included with metrics,pprof
-func NewDefaultService() *restful.WebService {
+func NewDefaultService(ctx context.Context) *restful.WebService {
 	routes := []Route{
-		NewSystem(),
-		NewHealthz(),
+		NewSystem(ctx),
+		NewHealthz(ctx),
 	}
 
 	ws := &restful.WebService{}

plugin/route/system.go

@@ -18,17 +18,25 @@ limitations under the License.
 package route
 
 import (
+	"context"
 	"net/http/pprof"
 
 	"github.com/emicklei/go-restful/v3"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+
+	"github.com/katanomi/pkg/config"
 )
 
 type system struct {
+	Context       context.Context
+	ConfigManager *config.Manager
 }
 
-func NewSystem() Route {
-	return &system{}
+func NewSystem(ctx context.Context) Route {
+	return &system{
+		Context:       ctx,
+		ConfigManager: config.KatanomiConfigManager(ctx),
+	}
 }
 
 func (s *system) Register(ws *restful.WebService) {
@@ -37,15 +45,21 @@ func (s *system) Register(ws *restful.WebService) {
 
 	// golang profile
 	ppprofPath := "/debug/pprof"
-	ws.Route(ws.GET(ppprofPath).To(wrapperF(pprof.Index)))
-	ws.Route(ws.GET(ppprofPath + "/cmdline").To(wrapperF(pprof.Index)))
-	ws.Route(ws.GET(ppprofPath + "/profile").To(wrapperF(pprof.Profile)))
-	ws.Route(ws.GET(ppprofPath + "/symbol").To(wrapperF(pprof.Symbol)))
-	ws.Route(ws.GET(ppprofPath + "/trace").To(wrapperF(pprof.Trace)))
-	ws.Route(ws.GET(ppprofPath + "/allocs").To(wrapperH(pprof.Handler("allocs"))))
-	ws.Route(ws.GET(ppprofPath + "/block").To(wrapperH(pprof.Handler("block"))))
-	ws.Route(ws.GET(ppprofPath + "/goroutine").To(wrapperH(pprof.Handler("goroutine"))))
-	ws.Route(ws.GET(ppprofPath + "/heap").To(wrapperH(pprof.Handler("heap"))))
-	ws.Route(ws.GET(ppprofPath + "/mutex").To(wrapperH(pprof.Handler("mutex"))))
-	ws.Route(ws.GET(ppprofPath + "/threadcreate").To(wrapperH(pprof.Handler("threadcreate"))))
+
+	configFilter := NoOpFilter
+	if s.ConfigManager != nil && s.Context != nil {
+		configFilter = config.ConfigFilter(s.Context, s.ConfigManager, config.PprofEnabledKey, config.ConfigFilterNotFoundWhenNotTrue)
+	}
+
+	ws.Route(ws.GET(ppprofPath).Filter(configFilter).To(wrapperF(pprof.Index)))
+	ws.Route(ws.GET(ppprofPath + "/cmdline").Filter(configFilter).To(wrapperF(pprof.Index)))
+	ws.Route(ws.GET(ppprofPath + "/profile").Filter(configFilter).To(wrapperF(pprof.Profile)))
+	ws.Route(ws.GET(ppprofPath + "/symbol").Filter(configFilter).To(wrapperF(pprof.Symbol)))
+	ws.Route(ws.GET(ppprofPath + "/trace").Filter(configFilter).To(wrapperF(pprof.Trace)))
+	ws.Route(ws.GET(ppprofPath + "/allocs").Filter(configFilter).To(wrapperH(pprof.Handler("allocs"))))
+	ws.Route(ws.GET(ppprofPath + "/block").Filter(configFilter).To(wrapperH(pprof.Handler("block"))))
+	ws.Route(ws.GET(ppprofPath + "/goroutine").Filter(configFilter).To(wrapperH(pprof.Handler("goroutine"))))
+	ws.Route(ws.GET(ppprofPath + "/heap").Filter(configFilter).To(wrapperH(pprof.Handler("heap"))))
+	ws.Route(ws.GET(ppprofPath + "/mutex").Filter(configFilter).To(wrapperH(pprof.Handler("mutex"))))
+	ws.Route(ws.GET(ppprofPath + "/threadcreate").Filter(configFilter).To(wrapperH(pprof.Handler("threadcreate"))))
 }

sharedmain/app.go

@@ -604,7 +604,7 @@ func (a *AppBuilder) Run(startFuncs ...func(context.Context) error) error {
 	// adds a http server if there are any endpoints registered
 	if a.container != nil {
 		// adds profiling and health checks
-		a.container.Add(route.NewDefaultService())
+		a.container.Add(route.NewDefaultService(a.Context))
 
 		if len(a.container.RegisteredWebServices()) > 0 {
 			a.container.Add(route.NewDocService(a.container.RegisteredWebServices()...))