Skip to content

Commit

Permalink
Merge pull request #594 from jetstack/VC-36510-compress-requests
Browse files Browse the repository at this point in the history 
VC-36510: Key Pair and Venafi Connection modes now use gzip compression
  • Loading branch information
@tfadeyi
tfadeyi authored Oct 28, 2024
2 parents 8731003 + 620a050 commit 1f00f09
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 27 deletions.
29 changes: 26 additions & 3 deletions pkg/agent/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import (
"github.com/jetstack/preflight/pkg/version"
)

// 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"`
Expand Down Expand Up @@ -159,6 +160,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) {
Expand Down Expand Up @@ -285,6 +291,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
Expand Down Expand Up @@ -326,6 +338,9 @@ type CombinedConfig struct {
VenConnName string
VenConnNS string

// VenafiCloudKeypair and VenafiCloudVenafiConnection modes only.
DisableCompression bool

// Only used for testing purposes.
OutputPath string
InputPath string
Expand Down Expand Up @@ -558,6 +573,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
}
Expand Down Expand Up @@ -652,7 +675,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)
}
Expand Down Expand Up @@ -710,7 +733,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)
Expand Down
146 changes: 144 additions & 2 deletions pkg/agent/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package agent

import (
"bytes"
"compress/gzip"
"context"
"fmt"
"io"
Expand Down Expand Up @@ -373,6 +374,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, "--install-namespace", "venafi"))
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) {
t.Setenv("POD_NAMESPACE", "venafi")
got, _, err := ValidateAndCombineConfig(discardLogs(),
Expand Down Expand Up @@ -632,6 +646,83 @@ 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, "--install-namespace", "venafi"),
)
require.NoError(t, err)
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, "--install-namespace", "venafi"),
)
require.NoError(t, err)
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
Expand Down Expand Up @@ -711,8 +802,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)
Expand All @@ -724,6 +819,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) {
Expand Down
54 changes: 43 additions & 11 deletions pkg/client/client_venafi_cloud.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package client

import (
"bytes"
"compress/gzip"
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
Expand Down Expand Up @@ -50,6 +51,8 @@ type (
jwtSigningAlg jwt.SigningMethod
lock sync.RWMutex

disableCompression bool

// Made public for testing purposes.
Client *http.Client
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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
}

Expand Down Expand Up @@ -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")

Expand Down
Loading

0 comments on commit 1f00f09

Please sign in to comment.