Skip to content

feat: resolve image codes and their environment variables - #187

Open
Ebunwadi wants to merge 12 commits into
mainfrom
feat/imagecode-env-vars
Open

Ebunwadi wants to merge 12 commits into
mainfrom
feat/imagecode-env-vars

Conversation

@Ebunwadi

@Ebunwadi Ebunwadi commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

🛠️ Pull Request

✨ Feature

Description:

Lets a researcher write {{@imageCode}} in an executor's image field instead of a real container image, and have the TRE resolve it — along with the environment variables that image needs — at dispatch time.

Resolves TREFX-673.

How it works

A submission's executors, as the researcher writes them:

executors[0]   image: "{{@sky-create}}"
executors[1]   image: "ubuntu"

What the TRE does with that:

Credentials process          once per submission
                             evaluates the environment variables DMN
                             stores each variable in Vault:

                               tre/{user}/{sub}/{project}               shared
                               tre/{user}/{sub}/{project}/@sky-create   scoped to that image

Agent                        once per executor

  [0]  asks the ImageCode DMN  ->  dare.sky-create:1.0.1
       injects the shared variables AND @sky-create's own

  [1]  no image code, image left as written
       injects the shared variables only

Dispatch to TESK

Executor 1 not receiving @sky-create's variables is the point of the feature:
each executor gets only what belongs to the image it is about to run.

Two DMN tables:

Inputs Outputs Vault
Image Codes (new, imagecodes.dmn) project, user, imageCode image no
Environment Variables (existing credentials.dmn, extended) project, user, submissionId env, value, tag, imageCode, isSecret, vaultPath yes, on isSecret rows

Admin UI

A decision table selector on the DMN rule manager (treUI, port 8989). The page was already fully dynamic — it builds columns and form fields from whatever the table declares — so the new environment variable columns needed no change. Only the means to switch tables was missing. The table list comes from the API rather than being hardcoded, and failing to load it hides the selector and loads the default, so a partial deploy degrades rather than breaks.

⚠️ Before deploying

The DMN files sit on the tre_process_models named volume, and Docker only seeds a named volume from the image when it is empty. imagecodes.dmn will not appear on any environment that has already run the stack, so ImageCodesDMN never deploys and image resolution fails. The volume needs recreating, or the file copying in.

Tested on the demo stack

Scenario Result
Both DMN tables deploy to Zeebe Pass
Image resolution - {{@sky-create}} reaches TESK as the real image Pass
Per-executor scoping - an executor without the image code does not receive its variables Pass
One row naming two image codes reaches both executors Pass
Secret resolved from Vault into the container Pass
Unreadable vaultPath fails the submission, and writes no partial paths Pass
Unresolvable image code fails with "Could not resolve image code(s)", nothing dispatched Pass
Dispatch log shows variable names only, no values Pass
Admin UI switches tables; new columns render and save Pass
Invalid FEEL in a rule is rejected and leaves credentials.dmn untouched Pass
Vault cleanup removes every path for a submission, including per-image ones Pass

Found while testing:

  • Funnel logs the full docker run line, values included, into its system_logs,
    which are returned through the TES API. This branch stops the Agent writing
    credentials to Seq; Funnel writes the same ones a step later.
  • Revoke soft-deletes in Vault (DELETE .../data/...), so an "ephemeral"
    credential stays recoverable with vault kv undelete.

📝 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?

* feat: add image code resolution DMN table

* Update imagecodes.dmn
* feat: let the DMN admin API address more than one table

The DMN endpoints were fixed to credentials.dmn, so there was no way for a TRE
Admin to view or edit the new image code table. They now take an optional table
selector, and the tables themselves are described in one place rather than being
hardcoded wherever they are needed.

* feat: let TRE Admins choose which DMN table to edit

Adds a decision table selector to the DMN rule manager, so the image code table
can be managed alongside the environment variables one.
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

✒️ PR Title Commitlint - ✔️ Lint success!

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

🛠️ 🐳 Build PR Container - agent-api-container - 🏷️ Commit: 4ca794f - ⚙️ Workflow: 41335757760

harbor.federated-analytics.ac.uk/5s-tes/agent-api:pr-187
harbor.federated-analytics.ac.uk/5s-tes/agent-api:pr-187-4ca794f
sha256:f21a7fefbc815fd745aa63fc79bdb550421304769e5166de310972bf196b9499

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

🛠️ 🐳 Build PR Container - agent-ui-container - 🏷️ Commit: 4ca794f - ⚙️ Workflow: 41335757760

harbor.federated-analytics.ac.uk/5s-tes/agent-ui:pr-187
harbor.federated-analytics.ac.uk/5s-tes/agent-ui:pr-187-4ca794f
sha256:f80de7fc7e02b093ddcc8f820ba753e9571579126c8dfad3ea94a6dfdd90f85b

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

🛠️ 🐳 Build PR Container - agent-web-container - 🏷️ Commit: 4ca794f - ⚙️ Workflow: 41335757760

harbor.federated-analytics.ac.uk/5s-tes/agent-web:pr-187
harbor.federated-analytics.ac.uk/5s-tes/agent-web:pr-187-4ca794f
sha256:74154bfb44817898b6e79e80a3a05b69cc581e5274810c87fbd576ee427bdbf1

@Ebunwadi
Ebunwadi marked this pull request as ready for review October 8, 2026 11:19
else
{
// Nothing read, so allow a later poll to try this path again.
fetchedPaths.Remove(record.VaultPath);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we encounter an exception when calling _vaultService.GetCredentialAsync(record.VaultPath) on line 1237, we'll exit the try block and skip over this line, meaning this vault path doesn't get removed from fetchedPaths. Could the try catch be moved inside the loop so that the vault paths can be removed from the hash set in the catch block too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch. Its fixed now


/// <summary>Variables keyed by the image code they belong to.</summary>
public Dictionary<string, Dictionary<string, string>> ByImageCode { get; } =
new(StringComparer.Ordinal);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SharedByType uses OrdinalIgnoreCase, should ByImageCode also ignore case? In CreateTreCredentialsHandler line 40, case doesn't get changed, but in ImageCodeResolver lines 89 and 166, ToLowerInvariant is being used. If codes can end up in this dictionary from both those sources then there could be a mismatch in casing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch. Its fixed now

@sonarqubeserver-fed-a

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants