Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VC-37264: Disable compression to give us time to work on a fix #628

Merged
merged 4 commits into from
Nov 22, 2024
+49 −241
Merged

VC-37264: Disable compression to give us time to work on a fix #628

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20d9b6f
remove logic behind --disable-compression but keep it to avoid breakage
maelvls Nov 22, 2024
695b95c
rm-disable-compression: hide the flag --disable-compression
maelvls Nov 22, 2024
537c1e6
rm-disable-compression: use pflag's MarkDeprecated
maelvls Nov 22, 2024
4a8bccd
rm-disable-compression: fix test
maelvls Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions pkg/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,8 @@ 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 (--disable-compression) is deprecated and no longer
// has an effect.
DisableCompression bool
}

Expand Down Expand Up @@ -302,8 +301,9 @@ 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.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional. Consider using the pflag.MarkDeprecated method instead, which will hide the flag from the --help and therefore from the documented CLI help.

Copy link
Member Author

@maelvls maelvls Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that would be even better.

I've tried this:

c.PersistentFlags().MarkDeprecated("disable-compression", "no longer has an effect")

But it uses logrus' logger, so it doesn't show as JSON (first line of the below snippet):

Flag --disable-compression has been deprecated, no longer has an effect
I1122 18:09:24.792410   25556 run.go:59] "Starting" logger="Run" version="development" commit=""
I1122 18:09:24.793470   25556 config.go:404] "Using the Venafi Cloud Key Pair Service Account auth mode since --client-id and --private-key-path were specified." logger="Run"
I1122 18:09:24.793480   25556 config.go:540] "Using period from config" logger="Run" period="5m0s"
I1122 18:09:24.793486   25556 config.go:592] "The flag --disable-compression has been deprecated an no longer has any effect." logger="Run"

Is that OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use MarkHidden instead?

Copy link
Member Author

@maelvls maelvls Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to use MarkDeprecated as you initially suggested. It seemed more appropriate and I was able to remove all traces of the DisableCompression variable, so even better.

I don't think this "plain text" line is a big deal; no one will ever see it anyways, and I can fix it later if I can find a way to set up pflag's logger.

)
c.PersistentFlags().MarkHidden("disable-compression")
}

type AuthMode string
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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ go run ./ agent --one-shot --api-token should-not-be-required --install-namespace venafi --output-path /dev/null --agent-config-file examples/cert-manager-agent.yaml  --disable-compression
I1122 17:06:03.444467  284739 run.go:59] "Starting" logger="Run" version="development" commit=""
I1122 17:06:03.444747  284739 config.go:403] "Using the Jetstack Secure API Token auth mode since --api-token was specified." logger="Run"
I1122 17:06:03.444766  284739 config.go:421] "Using deprecated Endpoint configuration. User Server instead." logger="Run"
I1122 17:06:03.444773  284739 config.go:591] "The flag --disable-compression has been deprecated an no longer has any effect." logger="Run"
I1122 17:06:03.444805  284739 run.go:117] "Healthz endpoints enabled" logger="Run.APIServer" addr=":8081" path="/healthz"
I1122 17:06:03.444816  284739 run.go:121] "Readyz endpoints enabled" logger="Run.APIServer" addr=":8081" path="/readyz"
E1122 17:06:03.445471  284739 run.go:269] "Error messages will not show in the pod's events because the POD_NAME environment variable is empty" logger="Run"
I1122 17:06:03.753982  284739 run.go:322] "Data saved to local file" logger="Run.gatherAndOutputData" outputPath="/dev/null"


// 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
View file
Open in desktop
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
View file
Open in desktop
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