Enhance `pre/post` cleanup by bypassing GitHub-hosted runners #431

MoChilia · 2024-04-09T08:51:45Z

The purpose of pre/post cleanup is to ensure no Azure account remains active before and after a job containing azure/login. This measure prevents incorrect operations on unexpected Azure accounts and protects against the disclosure of Azure accounts.

However, certain scenarios are ephemeral for only one job and don't need pre/post cleanup. Two main scenarios fall into this category:

GitHub-hosted runners: A GitHub-hosted runner is on a VM that gets re-imaged for every job. Once the job finishes, the VM is automatically decommissioned (see Using a GitHub-hosted runner). Since the VM is ephemeral, we can bypass this scenario to save time for our users. This bypass will be implemented in this PR. To detect whether a runner is GitHub-hosted, I refer to the solution here.
Running an ephemeral container on a self-hosted runner: This scenario, as described in (1.6.0 versions Pre login Fails on self hosted runners #403 (comment), involves running a container on a self-hosted runner where az is not pre-installed. We address this scenario by implementing PR Fix #403: Catch the error thrown in pre and post steps #407.

With this PR merged, it is assumed that pre/post cleanup will only take effect in scenarios where it is truly required.

Close Make pre and post action cleanup (az account clear) optional for better performance on ephemeral runners #426

HowardvanRooijen · 2024-04-10T14:09:48Z

Yes please... this would shave a few minutes off every single one of our builds...

maskati · 2024-04-17T15:12:21Z

Thanks for this PR. A note for 2. Running an ephemeral container on a self-hosted runner, my understanding is that #407 will not fix the case of ephemeral self-hosted runners that already have Azure CLI installed.

It might be quite difficult to determine if a job is running on an ephemeral runner. This information is in the runner config, but does not seem to be exposed to the running job. The easiest option would probably be to allow control of this through an action input parameter e.g. disable-cleanup.

YanaXu · 2024-06-06T02:30:00Z

Let's take the following questions to consideration:

Is it a good decision to only bypass GitHub-Hosted runners?
- Even if we do descide to only bypass GitHub-Hosted runners, is there a better solution than checking the runner's name? Refer to runner-context.
Can we make it configurable? E.g. an env variable?

kylefossum · 2024-06-17T16:40:55Z

Hi @MoChilia , can we get this patched up and merged? We waste a ton of build time on this.

I agree with Maskati that another input disable-cleanup with a default value of false would be good.

Then, skipping pre and post steps if runner.environment == github-hosted OR disable-cleanup == true

damianh · 2024-08-14T09:44:11Z

What's the delay here? This us costing is $$$ and is pure wastage.

phj-incom · 2024-09-08T08:00:07Z

Any news here?
Spending 30 seconds doing this pre job is stupid.

HowardvanRooijen · 2024-09-08T10:02:46Z

We got fed up of waiting, forked the repo and reference the fix in our workflows:

uses: endjin/login@4ae41681a03e0285d0a85d578968b6ba937c23ee     # forked version including hotfix from https://github.com/Azure/login/pull/431

YanaXu · 2024-09-09T07:04:35Z

Hi @HowardvanRooijen, @phj-incom , @damianh , we're sorry for the late response. We're reconsidering the design. If you're blocked by this issue, please consider fork the repo and take the PR change for a workaround, like @HowardvanRooijen did.

YanaXu · 2024-09-14T08:08:22Z

Implement the fix in #484. Close this PR now.

MoChilia added 2 commits April 9, 2024 15:48

cleanup bypass github-hosted runner

2d52a1a

fix syntax error

2e05c53

MoChilia requested a review from YanaXu April 9, 2024 08:51

MoChilia self-assigned this Apr 9, 2024

MoChilia temporarily deployed to Automation test April 9, 2024 08:51 — with GitHub Actions Inactive

MoChilia linked an issue Jun 6, 2024 that may be closed by this pull request

azure/login pre step takes up to 1 minute to execute #456

Open

JamesDawson added a commit to endjin/login that referenced this pull request Jul 10, 2024

Hotfix change from Azure#431

9f3bfea

JamesDawson added a commit to endjin/login that referenced this pull request Jul 10, 2024

Corrected hotfix change from Azure#431

4ae4168

jsoref mentioned this pull request Jul 19, 2024

Remove pre from the main action and move it to azure/login/clear-credentials #474

Closed

use env to disable pre/post

f6557e8

MoChilia temporarily deployed to Automation test July 22, 2024 03:54 — with GitHub Actions Inactive