feat: add method to miniohelper add remove setpolicy to a secret #940

mahadi99xy · 2025-07-04T11:31:21Z

🚧 Suggest a change

A clear and concise description of what you are changing.

📝 Pre-merge checklist

Ready to merge? Do not merge until all checks are satisfied.

💹 Have all required CI checks passed on the most recent commit?
✒️ Is the PR title a valid and meaningful conventional-commit message? ie. type(scope): summary
💥 Are breaking changes declared in the PR title in conventional-commit style? ie. type!(scope): summary
🎨 Does new code follow the code style of this project?
🔍 Has new code been spellchecked and linted?
📖 Have docs been updated where necessary?
💩 Have commits been checked for accidental file inclusions?

github-actions · 2025-07-04T11:32:01Z

✒️ PR Title Commitlint - ✔️ Lint success!

beforan · 2025-07-08T12:21:37Z

I couldn't find an item in JIRA for this PR, so I'm not really sure I fully understand the context.

Do these endpoints really belong on the SubmissionsController?
What are these credentials used for and why do we need to set them programmatically?

I get that Minio have painted us into a corner with having to use the admin cli if we do want to do this stuff programmatically (I'm really mad the REST API for this is undocumented), but I still don't like it as an approach and I feel like we have to do a lot design wise to mitigate future issues.

This PR's implementation is obviously docker specific and relies on a bunch of stuff:

A container for the admin cli existing
knowing the container name upfront
Having rights to run docker exec from the app's container (including the docker socket being mounted)
Communicating all that to anyone who wants to deploy it in a different way to our sample compose files.

But we also know that within the next few months we will want these applications running in Kubernetes.

We should have a plan for what that looks like (sidecar that works like this does? run a Job via the k8s api?) and therefore what the code looks like for doing the cli stuff in multiple completely different environments.

It's also completely Minio specific which is fine for this implementation but not from a standard API point of view. So that's a small consideration (Captured in my other comments).

beforan · 2025-07-08T12:23:07Z

src/BL/Services/IMinioHelper.cs

@@ -28,5 +29,8 @@ public interface IMinioHelper
        Task<bool> BucketPolicySetPublic(string bucketName);

        Task DeleteObject(string bucketName, string objectKey);
+        Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);


Suggested change

Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);

Task<MinIOOperationResult> CreateUserAsync(string accessKey, string secretKey);

WithMc is an implementation detail.

Arguably put that it's used in a doc comment to inform people how it works, but that should go on the implementation not the interface.

beforan · 2025-07-08T12:23:12Z

src/BL/Services/IMinioHelper.cs

@@ -28,5 +29,8 @@ public interface IMinioHelper
        Task<bool> BucketPolicySetPublic(string bucketName);

        Task DeleteObject(string bucketName, string objectKey);
+        Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);


Suggested change

Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);

Task<MinIOOperationResult> CreateUserAsync(string accessKey, string secretKey);

WithMc is an implementation detail.

Arguably put that it's used in a doc comment to inform people how it works, but that should go on the implementation not the interface.

beforan · 2025-07-08T12:25:53Z

src/BL/Services/IMinioHelper.cs

@@ -28,5 +29,8 @@ public interface IMinioHelper
        Task<bool> BucketPolicySetPublic(string bucketName);

        Task DeleteObject(string bucketName, string objectKey);
+        Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);
+        Task<MinIOOperationResult> SetUserPolicyAsync(string accessKey, string policyName);
+        Task<MinIOOperationResult> RemoveUserWithMcAsync(string accessKey);


Suggested change

Task<MinIOOperationResult> RemoveUserWithMcAsync(string accessKey);

Task<MinIOOperationResult> RemoveUserAsync(string accessKey);

As above, WithMc is an implementation detail

beforan · 2025-07-08T12:31:11Z

src/BL/Services/MinioHelper.cs

+                // Adjust container name based on your Docker Compose setup
+                var containerName = _minioSettings.ClientContainerName ?? "minio-client";
+                var dockerCommand = $"docker exec {containerName} {command}";


So this only works for docker, with some caveats:

We know the container name up front

This container (or environment) has access to the docker command, the docker socket (where this container is running), and permission to docker exec into the target container

We do know however that we are going to want to deploy this application on k8s at a minimum soon.

I guess just wanted to put it on the radar that we need to start designing for what this code looks like when we support both.

beforan · 2025-07-08T12:32:32Z

src/BL/Services/MinioHelper.cs

+                // Linux/macOS - use bash
+                return new ProcessStartInfo
+                {
+                    FileName = "/bin/bash",


Fine for now but it should be noted that this limits the app to running in containers where bash isn't available.

e.g. If someone wanted to build their own alpine container image for this app, this would stop them.

beforan · 2025-07-08T12:37:07Z

src/TRE-API/Controllers/SubmissionController.cs

+        /// </summary>
+        /// <param name="accessKey">Access Key identifier</param>
+        /// <param name="secretKey">Secret Key identifier</param>
+        [HttpPost("create-minio-secret/{accessKey}/{secretKey}")]


Should this be on SubmissionController?

it's not directly about handling a Submission, and I'm not sure I fully know the context for the functionality.

The end point is very RPC. It's an action create-minio-secret whereas really a Minio secret should be a resource, and a route to create one would be something like POST /api/v1/minio-secret

But also it shouldn't have minio in the route because that's an implementation detail, and we intend this to be a standard API (unless this isn't part of the standard)

So maybe storage-secret or something? Though that's too general and unhelpful. Like I say, I don't really understand what this is for.

beforan · 2025-07-08T12:37:55Z

src/TRE-API/Controllers/SubmissionController.cs

+        /// </summary>
+        /// <param name="accessKey">Access Key identifier</param>
+        /// <param name="policyName">Secret Key identifier</param>
+        [HttpPost("set-policy-minio-secret/{accessKey}/{policyName}")]


As above, not sure this should be on SubmissionController, and it shouldn't explicitly reference minio unless it's not intended to be part of the standard API.

beforan · 2025-07-08T12:38:43Z

src/TRE-API/Controllers/SubmissionController.cs

+        /// Remove secret for minio
+        /// </summary>
+        /// <param name="accessKey">Access Key identifier</param>
+        [HttpPost("remove-minio-secret/{accessKey}")]


Same as other endpoints

…tpolicy-to-a-secret

feat: add method to miniohelper add remove setpolicy to a secret

fc8cc2d

mahadi99xy requested a review from LockhartJones July 4, 2025 11:31

mahadi99xy added the patch: containers label Jul 4, 2025

mahadi99xy requested review from jaybeeelsdon, beforan and a team as code owners July 4, 2025 11:31

beforan reviewed Jul 8, 2025

View reviewed changes

Merge branch 'main' into feat/add-method-to-miniohelper-add-remove-se…

204a0e4

…tpolicy-to-a-secret

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add method to miniohelper add remove setpolicy to a secret #940

feat: add method to miniohelper add remove setpolicy to a secret #940

Uh oh!

mahadi99xy commented Jul 4, 2025

Uh oh!

github-actions bot commented Jul 4, 2025

Uh oh!

beforan commented Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025 •

edited

Loading

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Uh oh!

Uh oh!

	Task<MinIOOperationResult> CreateUserWithMcAsync(string accessKey, string secretKey);
	Task<MinIOOperationResult> CreateUserAsync(string accessKey, string secretKey);

	Task<MinIOOperationResult> RemoveUserWithMcAsync(string accessKey);
	Task<MinIOOperationResult> RemoveUserAsync(string accessKey);

feat: add method to miniohelper add remove setpolicy to a secret #940

Are you sure you want to change the base?

feat: add method to miniohelper add remove setpolicy to a secret #940

Uh oh!

Conversation

mahadi99xy commented Jul 4, 2025

🚧 Suggest a change

📝 Pre-merge checklist

Uh oh!

github-actions bot commented Jul 4, 2025

✒️ PR Title Commitlint - ✔️ Lint success!

Uh oh!

beforan commented Jul 8, 2025

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

beforan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beforan Jul 8, 2025 •

edited

Loading