[VC-43793] Add debug roundtripper to discovery and identity clients for easier debugging #683
Conversation
wallrj-cyberark
changed the title
wallrj-cyberark
force-pushed
the
VC-43403-inventory-api-debug-roundtrippers
branch
from
f13784c to
8cefefd
Compare
| @@ -46,7 +46,7 @@ func NewCyberArkClient(trustedCAs *x509.CertPool, baseURL string, authenticateRe | |||
| if trustedCAs != nil { | |||
| tr.TLSClientConfig.RootCAs = trustedCAs | |||
| } | |||
| cyberClient.Transport = transport.DebugWrappers(tr) | |||
| cyberClient.Transport = transport.NewDebuggingRoundTripper(tr, transport.DebugByContext) | |||
There was a problem hiding this comment.
The DebugWrappers function is better in a way because it only wraps the transport if the global logger has been configured at level >=6....which avoids the inefficiency of looking up the log level for each request if the log level is at its default level.
But it makes it harder to control the logging verbosity from tests because the wrapper doesn't know about the verbosity setting on the ktesting logger.
I might revert back to DebugWrapper in future, but for now this makes it easier for me to see what's going on in the test logs.
| @@ -12,6 +12,7 @@ import ( | |||
| "github.com/stretchr/testify/require" | |||
| "k8s.io/klog/v2" | |||
| "k8s.io/klog/v2/ktesting" | |||
| _ "k8s.io/klog/v2/ktesting/init" | |||
There was a problem hiding this comment.
This registers the -testing.v and -testing.vmodule command line args for the tests.
| // To enable verbose request logging: | ||
| // | ||
| // go test ./pkg/internal/cyberark/dataupload/... \ | ||
| // -v -count 1 -run TestPostDataReadingsWithOptionsWithRealAPI -args -testing.v 6 |
There was a problem hiding this comment.
I pasted some examples in the PR description.
| @@ -137,7 +143,7 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) { | |||
| return | |||
| } | |||
|
|
|||
| logger := ktesting.NewLogger(t, ktesting.NewConfig()) | |||
| logger := ktesting.NewLogger(t, ktesting.DefaultConfig) | |||
There was a problem hiding this comment.
DefaultConfig has the flag names which are registered by importing the init package (above)
| @@ -165,7 +171,7 @@ func TestPostDataReadingsWithOptionsWithRealAPI(t *testing.T) { | |||
| cyberArkClient, err := dataupload.NewCyberArkClient(nil, serviceURL, identityClient.AuthenticateRequest) | |||
| require.NoError(t, err) | |||
|
|
|||
| err = cyberArkClient.PostDataReadingsWithOptions(t.Context(), api.DataReadingsPost{}, dataupload.Options{ | |||
| err = cyberArkClient.PostDataReadingsWithOptions(ctx, api.DataReadingsPost{}, dataupload.Options{ | |||
There was a problem hiding this comment.
This was a mistake I made in #681
…ebugging Signed-off-by: Richard Wall <[email protected]>
wallrj-cyberark
force-pushed
the
VC-43403-inventory-api-debug-roundtrippers
branch
from
8cefefd to
102e566
Compare
| @@ -17,6 +17,8 @@ import ( | |||
| "github.com/jetstack/preflight/pkg/internal/cyberark/dataupload" | |||
| "github.com/jetstack/preflight/pkg/internal/cyberark/identity" | |||
| "github.com/jetstack/preflight/pkg/internal/cyberark/servicediscovery" | |||
|
|
|||
| _ "k8s.io/klog/v2/ktesting/init" | |||
There was a problem hiding this comment.
This sets up the -testing.v and -testing.vmodule command line flags for the test.
Closes: https://venafi.atlassian.net/browse/VC-43793
I want an easy way to see what requests are being made to the CyberArk API endpoints.
The dataupload wrapper already used transport.DebugRoundTripper, but the identity and discovery wrappers didn't.
In future perhaps we need to make all the API wrappers accept an HTTP client and pass in the venafi-connection-lib standard client. That will be in a followup PR.
Testing
Default:
Level 6: Responses are logged
Level 7: Request headers are logged, but tokens are masked.