external_cloud_audit: resource

[tobiaszheller]

This part adds external_cloud_audit as resource.
You can have only two resources as external cloud audit: draft and cluster. Draft is now mutable, and there is CRUD around it. You will be also to use it for test connection.
To use external audit in cluster, now you need to call PromoteToClusterExternalCloudAudit, which clones existing draft as cluster resource.
They are both of the same kind, only difference is name, which is hardcoded.
Depends on: #33022

Part of: https://github.com/gravitational/cloud/issues/4661
RFD: https://github.com/gravitational/cloud/blob/rfd/77-bring-your-own-bucket/rfd/0077-Bring-your-own-bucket.md

[espadolini]

What's the plan to enforce that cluster_external_audit must always refer to an existing external_audit?

[tobiaszheller]

What's the plan to enforce that cluster_external_audit must always refer to an existing external_audit?

I was planning to have following logic in grpc service:

func SetClusterExternalAudit(name) {
	got, err := storage.GetExternalAudit(ctx, name)
	if err != nil {
		// if external audit must exists before setting it as cluster external audit.
		return trace.Wrap(err)
  	}
  	if err := s.storage.SetClusterExternalAudit(ctx, name); err != nil {
    		return trace.Wrap(err)
  	}
}

func DeleteExternalAudit(name) {
  	got, err := s.storage.GetClusterExternalAudit(ctx)
	if err != nil {
		if !trace.IsNotFound(err) {
			return nil, trace.Wrap(err)
		}
		// Not found happens when we don't have any cluster external audit.
		// In that case we can remove external audit.
	}
	if got.GetName() == req.GetName() {
		return nil, status.Errorf(codes.FailedPrecondition, "cannot delete external audit used in cluster, DeleteClusterExternalAudit first")
	}
	if err := s.storage.DeleteExternalAudit(ctx, req.GetName()); err != nil {
		return nil, trace.Wrap(err)
	}
}

[espadolini]

I was planning to have following logic in grpc service:
[cut]

That's vulnerable to race conditions, you can delete the external_audit between storage.GetExternalAudit and storage.SetClusterExternalAudit.

Until we figure out transactional writes, your best bet is to put a backend lock around DeleteExternalAudit and SetClusterExternalAudit (not sure if you need the lock for DeleteClusterExternalAudit).

[espadolini]

What's the need for the multiple configuration objects, btw? Just the UX flow we decided on?

We could keep the ExternalCloudAudit objects for ui, checks and testing but then simply copy the entire configuration onto the singleton ClusterExternalCloudAudit item whenever we need to enable it.

[tobiaszheller]

What's the need for the multiple configuration objects, btw? Just the UX flow we decided on?

We could keep the ExternalCloudAudit objects for ui, checks and testing but then simply copy the entire configuration onto the singleton ClusterExternalCloudAudit item whenever we need to enable it.

Just to understand you correctly, instead of passing name to ClusterExternalCloudAudit, you want to copy it on enable, and from that time on, they will be separate entities, right?
We could do that (it will allow us to updating ExternalCloudAudit), but there is risk that someone will edit ExternalCloudAudit after it was updated to cluster and changes to cluster won't be propagated, until someone again calls EnableClusterExternalCLoudAudit. And we will probably need new name, because it's no longer enable, more like Promote? although it does not suggest that we will copy it 🤔

[tobiaszheller]

i have added new command, because we will extend it also with testConnection

[tobiaszheller]

@espadolini based on your comment with cloning new proto was spec was designed: #33022
Right now you can have only two resources as external cloud audit: draft and cluster. Draft is now mutable, and you have CRUD around it. You will be also to use it for test connection.
To use external audit in cluster, now you need to call PromoteToClusterExternalCloudConfig, which clones existing draft as cluster resource.
They are both of the same kind, only difference is name, which is hardcoded.

cc @bl-nero

[bl-nero]

I'm publishing a part of my comments, because I need to take a break, and then I'll move on to the rest.

[tobiaszheller]

@bl-nero a606832 should apply most of your requests

[public-teleport-github-review-bot]

@tobiaszheller See the table below for backport results.

Branch	Result
branch/v14	Create PR