Support for existing storage containers in aws and gcp.

[tasdomas]

This pull request contains a proposal to use gomock-generated mocks for testing interactions with
cloud APIs in unit tests.

[0x2b3bfa0]

Can we merge those two so users can specify either storage or storage/prefix with a single string?

[tasdomas]

I'm hesistant to do so - it feels like separating the two explicitly is clearer. Especially since we'll most likely also need to add a container-type field to support pre-allocated buckets in k8s.

[0x2b3bfa0]

In Kubernetes, this field would probably take a user-specified StorageClass or PersistentVolume name, which is a single string. 🤔 What else could it take?

[tasdomas]

what about e.g. a minio bucket?

[0x2b3bfa0]

That's not a native storage strictly speaking, and would have the same consideration as a Google Cloud task using a Microsoft Azure storage account. It would be out of scope for us, and users would be responsible of synchronization as part of their code.

[0x2b3bfa0]

Maybe too much flexibility? I'd assume that no special options are needed to connect and, if they were, users could just provide a full rclone connection string, removing the maintenance burden on our side.

[tasdomas]

It depends on what we're aiming at. Rclone connection strings hardly make it easy to configure.

The difference is between

  storage {    
    container = "tpi-test"
    container_path = "directory"
    container_opts {
      az_storage_account = "storage-account"
      az_storage_account_key = "storage-account-key"
    }
  }

and

  storage {
    container = ":azureblob,account='storage-account',key='storage-account-key':tpi-test/directory"
  }

I'm not sure the latter is easier, especially if we consider the target users of tpi.

[0x2b3bfa0]

More precisely, the difference would be between

  storage {    
    container = "tpi-test"
    container_path = "directory"
    container_opts {
      az_storage_account = "storage-account"
      az_storage_account_key = "storage-account-key"
    }
  }

and

  storage = ":azureblob,account='storage-account',key='storage-account-key':tpi-test/directory"

[0x2b3bfa0]

None of the variants sound particularly easy to me, although the latter is already well docuemnted by rclone and, despite being a lengthy string, is fairly readable to my mind.

[0x2b3bfa0]

Alternatively, we could perhaps provide a format that is 1:1 compatible with rclone strings. E.g.

storage = {
  type    = "azureblob"
  path    = "tpi-test/directory"

  options = {
    account = "storage-account"
    key     = "storage-account-key"
  }
}

would be directly translated into

:azureblob,account='storage-account',key='storage-account-key':tpi-test/directory

Still, it seems a bit too bulky, to my mind.

[0x2b3bfa0]

I'm not sure the latter is easier, especially if we consider the target users

That's a double-edged reason: if our target users are on their own, they probably won't need to specify a custom storage and, if they do, it probably won't require any options beyond the name and path (prefix). 😸

Note that credentials usually match (or can be retrieved by using) the main credentials used to create resources; see #651 (comment) for details.

And those users who really need custom options on their connection strings are probably part of a team with opinions, and people having those opinions may as well read rclone documentation. 😈 Worst-case scenario, we can provide an interactive rclone string builder as part of our documentation.

[0x2b3bfa0]

Can we assume that there is no need for separate credentials and the storage can be accessed with the same credentials provided to manage the rest of resources? In cases where different credentials are needed (e.g. Microsoft Azure), they can be retrieved using the main credentials:

terraform-provider-iterative/task/az/resources/resource_storage_account.go

Line 70 in 66e18e4

keys, err := s.Client.Services.StorageAccounts.ListKeys(ctx, s.Dependencies.ResourceGroup.Identifier, s.Identifier, "")

[tasdomas]

So, using cloud credentials to retrieve azure storage credentials will only work if the storage is owned by the same azure account. But that is not necessarily the case. I was thinking of also supporting azure sas url's which allow granting access to storage containers by sharing a single url (albeit a lengthy one).

The other side of the issue (and probably the main reason why I moved the connection string generation out to this interface) is separation of concerns. The container resource/data source is the right component to handle generation of the rclone connection string.

[0x2b3bfa0]

So, using cloud credentials to retrieve azure storage credentials will only work if the storage is owned by the same azure account.

What do you mean by account? 🤔

[tasdomas]

By account I meant the subscription - sorry about not being clear enough.

[tasdomas]

terraform-provider-iterative/task/az/resources/resource_storage_account.go

Line 70 in 66e18e4

keys, err := s.Client.Services.StorageAccounts.ListKeys(ctx, s.Dependencies.ResourceGroup.Identifier, s.Identifier, "")

This only works if the storage account is in the same resource group. In our case (since we've just created the resource group) we'd need to pass the resource group that the already existing storage account belongs to.

[0x2b3bfa0]

It seems to me a reasonable requirement. If the storage account were in a different subscription, it would have to be treated as an user-controlled storage (e.g. DVC remote) and not as a task storage.

I'd prefer to keep task storage as local and native as possible, and leave other kinds of data synchronization to specialized tools.

[tasdomas]

ok, this still leaves the issue of the storage account and its access key

[0x2b3bfa0]

the issue of the storage account and its access key

❓

[tasdomas]

the issue of the storage account and its access key

question
Well, in order to access a blob container, you need a storage account. And to retrieve a storage account's key you need to know its resource group, so either way the user needs to supply either (storage_account, key) or (resource_group, storage_account).

[0x2b3bfa0]

Thanks! I missed that comment.

Here you're two proposed solutions:

• Specify the storage account as part of a storage string and the key through an environment variable
• Specify both the resource group and the storage account as part of the storage string

[0x2b3bfa0]

Tests are nice, although mocking cloud infrastructure is rather risky. Maybe we can rely on rclone to use functionality that it's already tested on their side or can be tested locally with the same API as cloud resources?

[tasdomas]

I don't see how mocking cloud apis (not infrastructure) is riskier than having no tests at all.

My current proposal is twofold:

1. Each resource defines limited interfaces (implemented by corresponding cloud apis) that include only the methods actually used.
2. We generate mocks for those interfaces and use them in tests to ensure conditions are handled correctly and api calls are made in the right order.

[0x2b3bfa0]

If your proposal implies accounting for a considerable amount of cloud-specific details (e.g. “limited interfaces” for S3 and EC2, others for Cloud Storage and Compute Engine, ...) I wonder if maintaining them will be any easier than testing the code against actual cloud APIs, even if it's noticeably slower.

[tasdomas]

The thing with unit tests is that we can test for various edge cases in a repeatable and deterministic way. And while they are in no way a replacement for integration tests, accounting for edge cases in integration tests will either require developing extensive test suites or exhaustive checklists, completing which will take considerable time.

[0x2b3bfa0]

None of the review comments are specific to AWS or GCP. Feel free to merge into the feature branch. 🌳