External Audit Storage: Connection Tester

[logand22]

This PR adds a test connection methods for the currently configured draft external cloud audit resource.

Additionally adds an additional connection tester for the test for kind for externalcloudaudit kind. This will help facilitate the connection testing from teleport UI portion that @mcbattirola is working on.

Related:

• https://github.com/gravitational/teleport.e/pull/2682

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[rosstimothy]

It's a pretty uncommon pattern to pass around an error to be handled by another function. Can each of these helper functions make the appropriate call to test and handle the error instead of splitting the error handling out into its own function?

[logand22]

So I copied this pattern from the kube conn test shown here:

teleport/lib/client/conntest/kube.go

Lines 117 to 121 in b32d944

	tlsCfg, err := s.genKubeRestTLSClientConfig(ctx, mfaResponse, connectionDiagnosticID, req.ResourceName, currentUser.GetName())
	diag, diagErr := s.handleUserGenCertsErr(ctx, req.ResourceName, connectionDiagnosticID, err)
	if err != nil || diagErr != nil {
	return diag, diagErr
	}

. However, I've reworked it based on your feedback in this commit: b4e4510.

Let me know what you think.

[rosstimothy]

#34278 (comment)

[rosstimothy]

Do we need to return both errors? Can we use trace.NewAggregate to combine them into a single error?

[logand22]

I'm thinking it through. We return early on either a diagnostic error or a test connection error. We wrap the test connection error or success into the diagnostic and return the diagnostic error, if any, that occurs when we perform the test connection.

If we aggregate the error then we could simplify the conditional check but it would cause the return error to be non-nil when the test connection was non-nil. We currently return just the diagErr because the test connection error is wrapped into the diagnostics.

I don't know if trace.NewAggregate would allow us to return a nil error regardless of test connection, but then return the diagError if it occurs.

If we are looking for simpler logic, I could create a struct like this:

type ConnectionError struct{
    DiagErr error
    Err error
}

func (c ConnectionError) Error() string {
    return trace.NewAggregate(c.DiagErr, c.Err)
}

and then use it like:

func (s ExternalAuditStorageConnectionTester) handleBucketsTest(ctx context.Context, connectionDiagnosticID string) (types.ConnectionDiagnostic, error) {
...
diag, err := s.handleBucketsTest(ctx, connectionDiagnosticID)
if err != nil {
    return diag, err.(ConnectionError).DiagErr
}

[logand22]

Or just return the ConnectionError directly instead of return the error.

[nklaassen]

Hey @rosstimothy it's a bit confusing but I think the original is the cleanest way to handle this, we can just rename the test error

	testErr = client.TestDraftExternalCloudAuditBuckets(ctx)
	diag, diagErr := s.handleErrFromBucketsTest(ctx, connectionDiagnosticID, testErr)
	if testErr != nil || diagErr != nil {
		return diag, diagErr
	}

testErr is not really an error for the sake of this function, it's just the subtest result, so I don't really consider it passing of error handling to another function. handleErrFromBucketsTest tries to append a diagnostic trace about whether the subtest passed or failed, but may itself fail to add the trace. We want to return early if either the subtest failed, or we failed to add the trace.

We could even name it testResult and handleResultFromBucketsTest

[rosstimothy]

I suppose if that's the established pattern then I'm okay following it here. Though it is a curious pattern.

[public-teleport-github-review-bot]

@logand22 See the table below for backport results.

Branch	Result
branch/v14	Create PR