microsoft / ga4gh-tes

C# implementation of the GA4GH TES API; provides distributed batch task execution on Microsoft Azure
MIT License
32 stars 26 forks source link

WX-1594 Support for private ACR access in Terra #715

Closed jgainerdewar closed 1 month ago

jgainerdewar commented 2 months ago

I welcome C# style feedback, since I'm new to it.

This change adds two new config inputs: Sam URL and billing profile id. If they're provided, the batch scheduler uses these to fetch a distinct managed identity to use to pull the Docker image for the task. This identity is passed down to the TES Runner. Changes elsewhere will ensure that this identity is assigned to the BatchPool. If it isn't, the log contains a useful error message about the identity not being assigned.

BMurri commented 2 months ago

@jgainerdewar I don't know what is or isn't visible to you with regards to the checks, but the format check produced the following diagnostics:

/src/Tes.ApiClients/TerraSamApiClient.cs(59,19): error WHITESPACE: Fix whitespace formatting. Replace 18 characters with '\n\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s'. /src/Tes.ApiClients/TerraSamApiClient.cs(60,97): error WHITESPACE: Fix whitespace formatting. Replace 22 characters with '\n\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s'. /src/Tes.ApiClients/TerraSamApiClient.cs(61,128): error WHITESPACE: Fix whitespace formatting. Replace 10 characters with '\n\s\s\s\s\s\s\s\s'.

/src/Tes.ApiClients.Tests/TerraApiStubData.cs(36,45): error WHITESPACE: Fix whitespace formatting. Replace 10 characters with '\n\s\s\s\s\s\s\s\s'. /src/Tes.ApiClients.Tests/TerraApiStubData.cs(291,7): error WHITESPACE: Fix whitespace formatting. Insert '\s\s'.

/src/Tes.ApiClients.Tests/TerraSamApiClientTests.cs(42,181): error WHITESPACE: Fix whitespace formatting. Replace 18 characters with '\n\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s'. /src/Tes.ApiClients.Tests/TerraSamApiClientTests.cs(52,2): error FINALNEWLINE: Fix final newline. Insert '\n'. /src/Tes.ApiClients.Tests/TerraSamApiClientTests.cs(1,1): error CHARSET: Fix file encoding.

/src/Tes.ApiClients/Models/Terra/SamActionManagedIdentityApiResponse.cs(1,1): error CHARSET: Fix file encoding.

So far I've only perused this PR, but I'm asking a couple of clarifications:

The SAM API is called to obtain an azure managed identity that has Acr Pull permissions in one or more azure container registries from which docker images can be retrieved, correct? Will that MI ever be different than the one passed in workflow_execution_identity?

jgainerdewar commented 2 months ago

@BMurri Luckily I can see the format check output, but I think you'll need to trigger it to run again with the latest changes, which should have fixed most of the issues (but may have created new ones!).

The SAM API is called to obtain an azure managed identity that has Acr Pull permissions in one or more azure container registries from which docker images can be retrieved, correct?

Yes, correct.

Will that MI ever be different than the one passed in workflow_execution_identity?

Yes, when it exists at all it will always be different. The workflow_execution_identity is associated with a single user, and the identity we'll use to pull ACR images acts more like a system-level identity. Customers will grant that single identity ACR Pull on their ACRs, and Terra will manage which users in that customer organization are permitted to use that identity to pull images. Happy to meet and talk in more detail about how this works if that helps.

BMurri commented 2 months ago

@jgainerdewar I sent you an email

BMurri commented 2 months ago

/azr run

jgainerdewar commented 2 months ago

@BMurri Still doing some final testing and planning to add more unit testing, but this is ready for another look.

jgainerdewar commented 2 months ago

@BMurri This is complete and ready for review, though I'm trying to figure out why, when running with this image, I get tasks failing in Batch with ExitCode 10 any time I have more than one running at once.