Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Azure Cleanroom based CCF #215

Merged
merged 28 commits into from
Dec 2, 2024
Merged

Add support for Azure Cleanroom based CCF #215

merged 28 commits into from
Dec 2, 2024

Conversation

DomAyre
Copy link
Collaborator

@DomAyre DomAyre commented Nov 21, 2024

Motivation

This allows us to test against a production-like mCCF replacement, and will allow us to validate it against a realistic use case for us

Changes

  • Add an scripts/az-cleanroom-aci dir which installs the extension and brings up the network
  • Provide a deployment name to conftest.py for deploying the network
  • Handle up script having logs other than the resulting env
  • Implement a way to remove all resources deployed by up.sh (maybe az cleanroom ccf network down?)
  • Add runs of this to CI
  • Split JWT issuer workspace and KMS workspace
  • Move scheduled testing to 01:00 and add a cleanup job at 00:00

Current compromises:

  • Tests currently run sequentially which is slow when each test needs to deploy azure resources, Run az-cleanroom-aci tests in parallel #224 will address this but it's currently blocked on az cleanroom so keeping separate so CI on this PR passes
  • Looks like az cleanroom doesn't specify a subject alt name so the service cert isn't usable, so I suggest for now we remove SSL cert verification in the endpoint scripts (i.e. add -k). I will raise an issue with them and when it's fixed we can remove -k

@DomAyre DomAyre marked this pull request as draft November 21, 2024 15:07
.

TMP: Use permitted sub

.

.

.

Avoid races setting up ccf providers

.

.

.

Fix permissions on setup.sh

Pre-start jwt issuer to prevent races

Run tests sequentially

Fix JWT test
@DomAyre DomAyre changed the base branch from az-cleanroom to main November 27, 2024 17:27
@DomAyre DomAyre marked this pull request as ready for review November 28, 2024 19:00
on:
workflow_dispatch:
schedule:
- cron: "0 0 * * *"
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this every night is problematic. Could we do the cleanup on Sunday?
Scenario:
We have large builds/tests. Start a full build/test before shutting down the day to inspect the next morning.

Or maybe we can do cleanup for containers older than e.g. 2 days

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nightly cleanup is how all of our other repos work, including privacy-sandbox-dev which has about an order of magnitude more expensive builds/tests than this repo. What are you envisioning taking overnight and relying on having something deployed in ACI for that whole time?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a plead for not doing nightly cleanups for any of our repos. At least wait one day so if you come back in the morning you can still inspect the failing tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The cleanup is before the nightly testing, so you can inspect the resources deployed by a given failing run for a full work day before it's cleaned up. And of course the logs in actions itself should have a reasonable amount of information to be useful too and that lasts much longer

source ./scripts/jwt_issuer/up.sh
pytest -sv \
test/system-test/test_${{ inputs.endpoint }}.py \
-n ${{ matrix.config.threads }}
Copy link
Contributor

Choose a reason for hiding this comment

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

cool. Can we use -n on our other CI's?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If by other CI's you mean using the sandbox_local environment, no because it's currently got hardcoded endpoint ports so if you tried to run multiple concurrently they would clash.

@@ -1,5 +1,5 @@
# Core Environment Variables
export SUBSCRIPTION=85c61f94-8912-4e82-900e-6ab44de9bdf8
export SUBSCRIPTION=7ca35580-fc67-469c-91a7-68b38569ca6e
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a secret?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why? It's just an ID you can't do anything with it without sufficient role based access

Copy link
Contributor

Choose a reason for hiding this comment

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

OK fair

@DomAyre DomAyre merged commit 930d594 into main Dec 2, 2024
21 checks passed
@DomAyre DomAyre deleted the az-cleanroom-2 branch January 8, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants