Skip to content

Commit

Permalink
remove logic behind --disable-compression but keep it to avoid breakage
Browse files Browse the repository at this point in the history
We decided to entirely remove the logic behind --disable-compression
because the validation logic was looking super weird; we don't think
folks should be using --disable-compression=false anyways.

If we want to re-introduce a similar flag later on, we can create a new,
for example --no-compression.
  • Loading branch information
maelvls committed Nov 22, 2024
1 parent eb3c30a commit 20d9b6f
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 230 deletions.
12 changes: 5 additions & 7 deletions pkg/agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ func InitAgentCmdFlags(c *cobra.Command, cfg *AgentCmdFlags) {
&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.",
"Deprecated. No longer has an effect.",
)
}

Expand Down Expand Up @@ -346,7 +346,6 @@ type CombinedConfig struct {
VenConnNS string

// VenafiCloudKeypair and VenafiCloudVenafiConnection modes only.
DisableCompression bool
ExcludeAnnotationKeysRegex []*regexp.Regexp
ExcludeLabelKeysRegex []*regexp.Regexp

Expand Down Expand Up @@ -588,10 +587,9 @@ func ValidateAndCombineConfig(log logr.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))
if flags.DisableCompression {
log.Info("The flag --disable-compression has been deprecated an no longer has any effect.")
}
res.DisableCompression = flags.DisableCompression
}

// Validation of the config fields exclude_annotation_keys_regex and
Expand Down Expand Up @@ -709,7 +707,7 @@ func validateCredsAndCreateClient(log logr.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, cfg.DisableCompression)
preflightClient, err = client.NewVenConnClient(restCfg, metadata, cfg.InstallNS, cfg.VenConnName, cfg.VenConnNS, nil)
if err != nil {
errs = multierror.Append(errs, err)
}
Expand Down Expand Up @@ -767,7 +765,7 @@ func createCredentialClient(log logr.Logger, credentials client.Credentials, cfg
log.Info("Loading upload_path from \"venafi-cloud\" configuration.")
uploadPath = cfg.UploadPath
}
return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath, cfg.DisableCompression)
return client.NewVenafiCloudClient(agentMetadata, creds, cfg.Server, uploaderID, uploadPath)

case *client.OAuthCredentials:
return client.NewOAuthClient(agentMetadata, creds, cfg.Server)
Expand Down
159 changes: 19 additions & 140 deletions pkg/agent/config_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
package agent

import (
"bytes"
"compress/gzip"
"context"
"fmt"
"io"
"net/http"
"os"
"testing"
Expand Down Expand Up @@ -165,6 +162,25 @@ func Test_ValidateAndCombineConfig(t *testing.T) {
assert.Equal(t, true, got.StrictMode)
})

t.Run("--disable-compression is deprecated and doesn't do anything", func(t *testing.T) {
path := withFile(t, `{"user_id":"[email protected]","user_secret":"foo","client_id": "k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo","client_secret": "f39w_3KT9Vp0VhzcPzvh-uVbudzqCFmHER3Huj0dvHgJwVrjxsoOQPIw_1SDiCfa","auth_server_domain":"auth.jetstack.io"}`)
log, b := recordLogs(t)
_, _, err := ValidateAndCombineConfig(log,
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.NoError(t, err)
assert.Equal(t, testutil.Undent(`
INFO Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud.
INFO Using period from config period="1h0m0s"
INFO The flag --disable-compression has been deprecated an no longer has any effect.
`), b.String())
})

t.Run("error when no auth method specified", func(t *testing.T) {
_, cl, err := ValidateAndCombineConfig(discardLogs(),
withConfig(testutil.Undent(`
Expand Down Expand Up @@ -375,19 +391,6 @@ 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":"[email protected]","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 @@ -647,83 +650,6 @@ 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 @@ -820,53 +746,6 @@ 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: 11 additions & 43 deletions pkg/client/client_venafi_cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package client

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

disableCompression bool

// Made public for testing purposes.
Client *http.Client
}
Expand Down Expand Up @@ -87,7 +84,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, disableCompression bool) (*VenafiCloudClient, error) {
func NewVenafiCloudClient(agentMetadata *api.AgentMetadata, credentials *VenafiSvcAccountCredentials, baseURL string, uploaderID string, uploadPath string) (*VenafiCloudClient, error) {
if err := credentials.Validate(); err != nil {
return nil, fmt.Errorf("cannot create VenafiCloudClient: %w", err)
}
Expand All @@ -110,16 +107,15 @@ 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,
disableCompression: disableCompression,
agentMetadata: agentMetadata,
credentials: credentials,
baseURL: baseURL,
accessToken: &venafiCloudAccessToken{},
Client: &http.Client{Timeout: time.Minute},
uploaderID: uploaderID,
uploadPath: uploadPath,
privateKey: privateKey,
jwtSigningAlg: jwtSigningAlg,
}, nil
}

Expand Down Expand Up @@ -264,39 +260,11 @@ func (c *VenafiCloudClient) Post(path string, body io.Reader) (*http.Response, e
return nil, err
}

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)
req, err := http.NewRequest(http.MethodPost, fullURL(c.baseURL, path), body)
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 20d9b6f

Please sign in to comment.