Closed jemrobinson closed 2 weeks ago
Click to see where and how coverage changed
File Statements Missing Coverage Coverage
(new stmts)Lines missing
data_safe_haven/commands
pulumi.py
sre.py
data_safe_haven/external/api
credentials.py
graph_api.py
data_safe_haven/infrastructure
project_manager.py
data_safe_haven/infrastructure/components/composite
__init__.py
entra_application.py
23-30, 53, 78, 102-151
data_safe_haven/infrastructure/components/dynamic
__init__.py
data_safe_haven/infrastructure/programs
declarative_sre.py
156
imperative_shm.py
data_safe_haven/infrastructure/programs/sre
entra.py
28-30, 43-47, 57-121
identity.py
39-40
remote_desktop.py
58-59
data_safe_haven/provisioning
sre_provisioning_manager.py
data_safe_haven/types
enums.py
Project Total
This report was generated by python-coverage-comment-action
@JimMadge : My biggest worry here is that we add an entry for the Microsoft Graph Service Principal to our state file. This contains a huge amount of data (which more than doubles the size of the state file). Since we're only using it to get the GUIDs for various named permissions, we could replace the bits we need with an Enum or lookup-table. However, this would mean that we would be out-of-sync if any of these GUIDs changed (unlikely to happen as this would break a lot of third-party code).
What do you think?
@jemrobinson is that the long version of the note about msgraph_permissions
in the PR?
How large is it exactly, does it cause problems? I'd be happy to leave it. Might actually provide some protection against Entra configuration changing.
@jemrobinson is that the long version of the note about
msgraph_permissions
in the PR?
Yes
How large is it exactly, does it cause problems? I'd be happy to leave it. Might actually provide some protection against Entra configuration changing.
About 12k lines (of which we use maybe 4-5 lines) of a 30k JSON file for a full SRE deployment. Also, we should note that the GraphApi class already has a partial copy of this (see below), so abstracting it into a common class could actually simplify our current structure.
@jemrobinson That sounds like a good idea then, if the stack doesn't really need the full set :+1:.
I'm not super worried though because it is just a few kb in a file we don't expect people to read or edit by hand.
@JimMadge : this is actually a bigger refactor than I'd realised. Let's merge this as-is (if you're happy).
:white_check_mark: Checklist
Enable foobar integration
rather than515 foobar
).develop
.:vertical_traffic_light: Depends on
n/a
:arrow_heading_up: Summary
Replace creation of SRE Entra applications through the SDK with creation through
pulumi-azuread
.Note we still need one SDK-created Entra application in the SHM which is used to authenticate
pulumi-azuread
.The
msgraph_permissions
map looks up the GUIDs for all possible permissions. We could consider replacing this with a static lookup table that only covers the ones we need.:closed_umbrella: Related issues
Closes #2215
:microscope: Tests
Tested on a fresh SRE deployment