From fa2955513c22930930d397ec813caeb14d15514f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 15 Oct 2024 09:24:57 +0200 Subject: [PATCH] gzip: key pair and venafi connection modes now use gzip compression For context, we have added gzip compression for the request bodies of data sent to the Venafi Control Plane API because we have hit 413 errors in NGINX and want to reduce the network load caused by the Agent for intermediate proxies and for our NGINX frontend (it buffers requests at the moment). --- pkg/agent/config.go | 29 +++++- pkg/agent/config_test.go | 144 +++++++++++++++++++++++++++++- pkg/client/client_venafi_cloud.go | 54 ++++++++--- pkg/client/client_venconn.go | 52 ++++++++--- pkg/client/client_venconn_test.go | 2 + 5 files changed, 254 insertions(+), 27 deletions(-) diff --git a/pkg/agent/config.go b/pkg/agent/config.go index 6f8186cc..4954deb6 100644 --- a/pkg/agent/config.go +++ b/pkg/agent/config.go @@ -27,7 +27,8 @@ const ( inClusterNamespacePath = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" ) -// Config wraps the options for a run of the agent. +// Config defines the YAML configuration file that you can pass using +// `--config-file` or `-c`. type Config struct { // Deprecated: Schedule doesn't do anything. Use `period` instead. Schedule string `yaml:"schedule"` @@ -164,6 +165,11 @@ type AgentCmdFlags struct { // Prometheus (--enable-metrics) enables the Prometheus metrics server. Prometheus bool + + // DisableCompression (--disable-compression) disables the GZIP compression + // when uploading the data. Useful for debugging purposes, or when an + // intermediate proxy doesn't like compressed data. + DisableCompression bool } func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { @@ -291,6 +297,12 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) { "Enables Prometheus metrics server on the agent (port: 8081).", ) + c.PersistentFlags().BoolVar( + &cfg.DisableCompression, + "disable-compression", + false, + "Disables GZIP compression when uploading the data. Useful for debugging purposes or when an intermediate proxy doesn't like compressed data.", + ) } type AuthMode string @@ -332,6 +344,9 @@ type CombinedConfig struct { VenConnNS string InstallNS string + // VenafiCloudKeypair and VenafiCloudVenafiConnection modes only. + DisableCompression bool + // Only used for testing purposes. OutputPath string InputPath string @@ -564,6 +579,14 @@ func ValidateAndCombineConfig(log *log.Logger, cfg Config, flags AgentCmdFlags) } } + // Validation of --disable-compression. + { + if flags.DisableCompression && res.AuthMode != VenafiCloudKeypair && res.AuthMode != VenafiCloudVenafiConnection { + errs = multierror.Append(errs, fmt.Errorf("--disable-compression can only be used with the %s and %s modes", VenafiCloudKeypair, VenafiCloudVenafiConnection)) + } + res.DisableCompression = flags.DisableCompression + } + if errs != nil { return CombinedConfig{}, nil, errs } @@ -658,7 +681,7 @@ func validateCredsAndCreateClient(log *log.Logger, flagCredentialsPath, flagClie break // Don't continue with the client if kubeconfig wasn't loaded. } - preflightClient, err = client.NewVenConnClient(restCfg, metadata, cfg.InstallNS, cfg.VenConnName, cfg.VenConnNS, nil) + preflightClient, err = client.NewVenConnClient(restCfg, metadata, cfg.InstallNS, cfg.VenConnName, cfg.VenConnNS, nil, cfg.DisableCompression) if err != nil { errs = multierror.Append(errs, err) } @@ -716,7 +739,7 @@ func createCredentialClient(log *log.Logger, credentials client.Credentials, cfg log.Println("Loading upload_path from \"venafi-cloud\" configuration.") uploadPath = cfg.UploadPath } - return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath) + return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath, cfg.DisableCompression) case *client.OAuthCredentials: return client.NewOAuthClient(agentMetadata, creds, cfg.Server) diff --git a/pkg/agent/config_test.go b/pkg/agent/config_test.go index c726a598..a934ff1d 100644 --- a/pkg/agent/config_test.go +++ b/pkg/agent/config_test.go @@ -2,6 +2,7 @@ package agent import ( "bytes" + "compress/gzip" "context" "fmt" "io" @@ -341,6 +342,19 @@ func Test_ValidateAndCombineConfig(t *testing.T) { assert.IsType(t, &client.OAuthClient{}, cl) }) + t.Run("jetstack-secure-oauth-auth: can't use --disable-compression", func(t *testing.T) { + path := withFile(t, `{"user_id":"fpp2624799349@affectionate-hertz6.platform.jetstack.io","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`) + _, _, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: https://api.venafi.eu + period: 1h + organization_id: foo + cluster_id: bar + `)), + withCmdLineFlags("--disable-compression", "--credentials-file", path)) + require.EqualError(t, err, "1 error occurred:\n\t* --disable-compression can only be used with the Venafi Cloud Key Pair Service Account and Venafi Cloud VenafiConnection modes\n\n") + }) + t.Run("jetstack-secure-oauth-auth: --credential-file used but file is missing", func(t *testing.T) { got, _, err := ValidateAndCombineConfig(discardLogs(), withConfig(testutil.Undent(` @@ -604,6 +618,81 @@ func Test_ValidateAndCombineConfig_VenafiCloudKeyPair(t *testing.T) { err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) require.NoError(t, err) }) + + t.Run("the request body is compressed", func(t *testing.T) { + srv, cert, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + if gotReq.URL.Path == "/v1/oauth/token/serviceaccount" { + return + } + assert.Equal(t, "/v1/tlspk/upload/clusterdata/no", gotReq.URL.Path) + + // Let's check that the body is compressed as expected. + assert.Equal(t, "gzip", gotReq.Header.Get("Content-Encoding")) + uncompressR, err := gzip.NewReader(gotReq.Body) + require.NoError(t, err, "body might not be compressed") + defer uncompressR.Close() + uncompressed, err := io.ReadAll(uncompressR) + require.NoError(t, err) + assert.Contains(t, string(uncompressed), `{"agent_metadata":{"version":"development","cluster_id":"test cluster name"}`) + }) + privKeyPath := withFile(t, fakePrivKeyPEM) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: `+srv.URL+` + period: 1h + cluster_id: "test cluster name" + venafi-cloud: + uploader_id: no + upload_path: /v1/tlspk/upload/clusterdata + `)), + withCmdLineFlags("--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", privKeyPath), + ) + testutil.TrustCA(t, cl, cert) + assert.Equal(t, VenafiCloudKeypair, got.AuthMode) + require.NoError(t, err) + + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) + require.NoError(t, err) + }) + + t.Run("--disable-compression works", func(t *testing.T) { + srv, cert, setVenafiCloudAssert := testutil.FakeVenafiCloud(t) + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + // Only care about /v1/tlspk/upload/clusterdata/:uploader_id?name= + if gotReq.URL.Path == "/v1/oauth/token/serviceaccount" { + return + } + + assert.Equal(t, "/v1/tlspk/upload/clusterdata/no", gotReq.URL.Path) + + // Let's check that the body isn't compressed. + assert.Equal(t, "", gotReq.Header.Get("Content-Encoding")) + b := new(bytes.Buffer) + _, err := b.ReadFrom(gotReq.Body) + require.NoError(t, err) + assert.Contains(t, b.String(), `{"agent_metadata":{"version":"development","cluster_id":"test cluster name"}`) + }) + + privKeyPath := withFile(t, fakePrivKeyPEM) + got, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: `+srv.URL+` + period: 1h + cluster_id: "test cluster name" + venafi-cloud: + uploader_id: no + upload_path: /v1/tlspk/upload/clusterdata + `)), + withCmdLineFlags("--disable-compression", "--client-id", "5bc7d07c-45da-11ef-a878-523f1e1d7de1", "--private-key-path", privKeyPath), + ) + testutil.TrustCA(t, cl, cert) + assert.Equal(t, VenafiCloudKeypair, got.AuthMode) + require.NoError(t, err) + + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: "test cluster name"}) + require.NoError(t, err) + }) } // Slower test cases due to envtest. That's why they are separated from the @@ -683,8 +772,12 @@ func Test_ValidateAndCombineConfig_VenafiConnection(t *testing.T) { }) cfg, cl, err := ValidateAndCombineConfig(discardLogs(), - Config{Server: "http://this-url-should-be-ignored", Period: 1 * time.Hour, ClusterID: "test cluster name"}, - AgentCmdFlags{VenConnName: "venafi-components", InstallNS: "venafi"}) + withConfig(testutil.Undent(` + server: http://this-url-should-be-ignored + period: 1h + cluster_id: test cluster name + `)), + withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi")) require.NoError(t, err) testutil.VenConnStartWatching(t, cl) @@ -696,6 +789,53 @@ func Test_ValidateAndCombineConfig_VenafiConnection(t *testing.T) { err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: cfg.ClusterID}) require.NoError(t, err) }) + + t.Run("the request is compressed by default", func(t *testing.T) { + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + // Let's check that the body is compressed as expected. + assert.Equal(t, "gzip", gotReq.Header.Get("Content-Encoding")) + uncompressR, err := gzip.NewReader(gotReq.Body) + require.NoError(t, err, "body might not be compressed") + defer uncompressR.Close() + uncompressed, err := io.ReadAll(uncompressR) + require.NoError(t, err) + assert.Contains(t, string(uncompressed), `{"agent_metadata":{"version":"development","cluster_id":"test cluster name"}`) + }) + cfg, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + period: 1h + cluster_id: test cluster name + `)), + withCmdLineFlags("--venafi-connection", "venafi-components", "--install-namespace", "venafi")) + require.NoError(t, err) + testutil.VenConnStartWatching(t, cl) + testutil.TrustCA(t, cl, cert) + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: cfg.ClusterID}) + require.NoError(t, err) + }) + + t.Run("--disable-compression works", func(t *testing.T) { + setVenafiCloudAssert(func(t testing.TB, gotReq *http.Request) { + // Let's check that the body isn't compressed. + assert.Equal(t, "", gotReq.Header.Get("Content-Encoding")) + b := new(bytes.Buffer) + _, err := b.ReadFrom(gotReq.Body) + require.NoError(t, err) + assert.Contains(t, b.String(), `{"agent_metadata":{"version":"development","cluster_id":"test cluster name"}`) + }) + cfg, cl, err := ValidateAndCombineConfig(discardLogs(), + withConfig(testutil.Undent(` + server: `+srv.URL+` + period: 1h + cluster_id: test cluster name + `)), + withCmdLineFlags("--disable-compression", "--venafi-connection", "venafi-components", "--install-namespace", "venafi")) + require.NoError(t, err) + testutil.VenConnStartWatching(t, cl) + testutil.TrustCA(t, cl, cert) + err = cl.PostDataReadingsWithOptions(nil, client.Options{ClusterName: cfg.ClusterID}) + require.NoError(t, err) + }) } func Test_ParseConfig(t *testing.T) { diff --git a/pkg/client/client_venafi_cloud.go b/pkg/client/client_venafi_cloud.go index 7e317faf..ae1920d8 100644 --- a/pkg/client/client_venafi_cloud.go +++ b/pkg/client/client_venafi_cloud.go @@ -2,6 +2,7 @@ package client import ( "bytes" + "compress/gzip" "crypto" "crypto/ecdsa" "crypto/ed25519" @@ -50,6 +51,8 @@ type ( jwtSigningAlg jwt.SigningMethod lock sync.RWMutex + disableCompression bool + // Made public for testing purposes. Client *http.Client } @@ -84,7 +87,7 @@ const ( // NewVenafiCloudClient returns a new instance of the VenafiCloudClient type that will perform HTTP requests using a bearer token // to authenticate to the backend API. -func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiSvcAccountCredentials, baseURL string, uploaderID string, uploadPath string) (*VenafiCloudClient, error) { +func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiSvcAccountCredentials, baseURL string, uploaderID string, uploadPath string, disableCompression bool) (*VenafiCloudClient, error) { if err := credentials.Validate(); err != nil { return nil, fmt.Errorf("cannot create VenafiCloudClient: %w", err) } @@ -107,15 +110,16 @@ func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiS } return &VenafiCloudClient{ - agentMetadata: agentMetadata, - credentials: credentials, - baseURL: baseURL, - accessToken: &venafiCloudAccessToken{}, - Client: &http.Client{Timeout: time.Minute}, - uploaderID: uploaderID, - uploadPath: uploadPath, - privateKey: privateKey, - jwtSigningAlg: jwtSigningAlg, + agentMetadata: agentMetadata, + credentials: credentials, + baseURL: baseURL, + accessToken: &venafiCloudAccessToken{}, + Client: &http.Client{Timeout: time.Minute}, + uploaderID: uploaderID, + uploadPath: uploadPath, + privateKey: privateKey, + jwtSigningAlg: jwtSigningAlg, + disableCompression: disableCompression, }, nil } @@ -260,11 +264,39 @@ func (c *VenafiCloudClient) Post(path string, body io.Reader) (*http.Response, e return nil, err } - req, err := http.NewRequest(http.MethodPost, fullURL(c.baseURL, path), body) + var encodedBody io.Reader + if c.disableCompression { + encodedBody = body + } else { + compressed := new(bytes.Buffer) + gz := gzip.NewWriter(compressed) + if _, err := io.Copy(gz, body); err != nil { + return nil, err + } + err := gz.Close() + if err != nil { + return nil, err + } + encodedBody = compressed + } + + req, err := http.NewRequest(http.MethodPost, fullURL(c.baseURL, path), encodedBody) if err != nil { return nil, err } + // We have noticed that NGINX, which is Venafi Control Plane's API gateway, + // has a limit on the request body size we can send (client_max_body_size). + // On large clusters, the agent may exceed this limit, triggering the error + // "413 Request Entity Too Large". Although this limit has been raised to + // 1GB, NGINX still buffers the requests that the agent sends because + // proxy_request_buffering isn't set to off. To reduce the strain on NGINX' + // memory and disk, to avoid further 413s, and to avoid reaching the maximum + // request body size of customer's proxies, we have decided to enable GZIP + // compression. Ref: https://venafi.atlassian.net/browse/VC-36434. + if !c.disableCompression { + req.Header.Set("Content-Encoding", "gzip") + } req.Header.Set("Accept", "application/json") req.Header.Set("Content-Type", "application/json") diff --git a/pkg/client/client_venconn.go b/pkg/client/client_venconn.go index f614ecd0..32f61e0e 100644 --- a/pkg/client/client_venconn.go +++ b/pkg/client/client_venconn.go @@ -2,6 +2,7 @@ package client import ( "bytes" + "compress/gzip" "context" "crypto/x509" "encoding/base64" @@ -34,6 +35,8 @@ type VenConnClient struct { venConnName string // Name of the VenafiConnection resource to use. venConnNS string // Namespace of the VenafiConnection resource to use. + disableCompression bool + // Used to make HTTP requests to Venafi Cloud. This field is public for // testing purposes so that we can configure trusted CAs; there should be a // way to do that without messing with the client directly (e.g., a flag to @@ -53,7 +56,7 @@ type VenConnClient struct { // empty. `venConnName` and `venConnNS` must not be empty either. The passed // `restcfg` is not mutated. `trustedCAs` is only used for connecting to Venafi // Cloud and Vault and can be left nil. -func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, installNS, venConnName, venConnNS string, trustedCAs *x509.CertPool) (*VenConnClient, error) { +func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, installNS, venConnName, venConnNS string, trustedCAs *x509.CertPool, disableCompression bool) (*VenConnClient, error) { // TODO(mael): The rest of the codebase uses the standard "log" package, // venafi-connection-lib uses "go-logr/logr", and client-go uses "klog". We // should standardize on one of them, probably "slog". @@ -113,12 +116,13 @@ func NewVenConnClient(restcfg *rest.Config, agentMetadata *api.AgentMetadata, in } return &VenConnClient{ - agentMetadata: agentMetadata, - connHandler: handler, - installNS: installNS, - venConnName: venConnName, - venConnNS: venConnNS, - Client: vcpClient, + agentMetadata: agentMetadata, + connHandler: handler, + installNS: installNS, + venConnName: venConnName, + venConnNS: venConnNS, + Client: vcpClient, + disableCompression: disableCompression, }, nil } @@ -157,19 +161,45 @@ func (c *VenConnClient) PostDataReadingsWithOptions(readings []*api.DataReading, DataGatherTime: time.Now().UTC(), DataReadings: readings, } - data, err := json.Marshal(payload) - if err != nil { - return err + + encodedBody := &bytes.Buffer{} + if c.disableCompression { + err = json.NewEncoder(encodedBody).Encode(payload) + if err != nil { + return err + } + } else { + gz := gzip.NewWriter(encodedBody) + err = json.NewEncoder(gz).Encode(payload) + if err != nil { + return err + } + err := gz.Close() + if err != nil { + return err + } } // The path parameter "no" is a dummy parameter to make the Venafi Cloud // backend happy. This parameter, named `uploaderID` in the backend, is not // actually used by the backend. - req, err := http.NewRequest(http.MethodPost, fullURL(token.BaseURL, "/v1/tlspk/upload/clusterdata/no"), bytes.NewBuffer(data)) + req, err := http.NewRequest(http.MethodPost, fullURL(token.BaseURL, "/v1/tlspk/upload/clusterdata/no"), encodedBody) if err != nil { return err } + // We have noticed that NGINX, which is Venafi Control Plane's API gateway, + // has a limit on the request body size we can send (client_max_body_size). + // On large clusters, the agent may exceed this limit, triggering the error + // "413 Request Entity Too Large". Although this limit has been raised to + // 1GB, NGINX still buffers the requests that the agent sends because + // proxy_request_buffering isn't set to off. To reduce the strain on NGINX' + // memory and disk, to avoid further 413s, and to avoid reaching the maximum + // request body size of customer's proxies, we have decided to enable GZIP + // compression. Ref: https://venafi.atlassian.net/browse/VC-36434. + if !c.disableCompression { + req.Header.Set("Content-Encoding", "gzip") + } req.Header.Set("Content-Type", "application/json") req.Header.Set("User-Agent", fmt.Sprintf("venafi-kubernetes-agent/%s", version.PreflightVersion)) diff --git a/pkg/client/client_venconn_test.go b/pkg/client/client_venconn_test.go index f765f1ad..c696c8ad 100644 --- a/pkg/client/client_venconn_test.go +++ b/pkg/client/client_venconn_test.go @@ -233,6 +233,7 @@ func run_TestVenConnClient_PostDataReadingsWithOptions(restcfg *rest.Config, kcl // Let's make sure we didn't forget to add the arbitrary "/no" // (uploader_id) path segment to /v1/tlspk/upload/clusterdata. assert.Equal(t, "/v1/tlspk/upload/clusterdata/no", r.URL.Path) + assert.Equal(t, "gzip", r.Header.Get("Content-Encoding")) }) certPool := x509.NewCertPool() @@ -246,6 +247,7 @@ func run_TestVenConnClient_PostDataReadingsWithOptions(restcfg *rest.Config, kcl "venafi-components", // Name of the VenafiConnection. testNameToNamespace(t), // Namespace of the VenafiConnection. certPool, + false, // disableCompression ) require.NoError(t, err)