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

Miscellaneous fixes from TRESA testing #2068

Merged
merged 22 commits into from
Aug 2, 2024
Merged

Miscellaneous fixes from TRESA testing #2068

merged 22 commits into from
Aug 2, 2024

Conversation

jemrobinson
Copy link
Member

@jemrobinson jemrobinson commented Jul 31, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

n/a

⤴️ Summary

Miscellaneous fixes from TRESA testing. Mostly documentation, but the following code changes:

  • add an explicit list of Azure locations that will work at the command line
  • replace any separators in the SHM name with - when constructing the Pulumi stack name (which is used for resource naming).
  • relax restrictions on Azure subscription naming which were preventing the use of subscriptions that already exist

🌂 Related issues

Closes #2063
Closes #2066

🔬 Tests

Tested through TRESA deployment

💭 Thoughts

  • Is a hardcoded list of Azure locations a good idea?
    • We can't check it programmatically because we do the validation before signing in to the CLI.
    • However, leaving it without validation (as was done previously) leaves us open to quite opaque errors when e.g. "UK South" is provided instead of "uksouth".
    • Another option might be to maintain a dict/enum/data structure of allowed options which map onto the short names
  • Is there an authoritative list of which characters are allowed in Azure subscriptions? The Microsoft guidance says that e.g. "[" is not allowed at the beginning of a name, which is provably false.

@jemrobinson jemrobinson requested review from a team as code owners July 31, 2024 15:15
@jemrobinson jemrobinson marked this pull request as draft July 31, 2024 15:16
@jemrobinson jemrobinson changed the title Miscellaneous fixes from TRESA testing WIP Miscellaneous fixes from TRESA testing Jul 31, 2024
Copy link

github-actions bot commented Jul 31, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/external/api
  graph_api.py 440
  data_safe_haven/infrastructure/programs
  declarative_sre.py
  data_safe_haven/validators
  validators.py
Project Total  

This report was generated by python-coverage-comment-action

@jemrobinson jemrobinson changed the title WIP Miscellaneous fixes from TRESA testing Miscellaneous fixes from TRESA testing Aug 1, 2024
@jemrobinson jemrobinson requested review from craddm and JimMadge August 1, 2024 10:58
@jemrobinson jemrobinson marked this pull request as ready for review August 1, 2024 10:58
Copy link
Contributor

@craddm craddm left a comment

Choose a reason for hiding this comment

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

LGTM

jemrobinson and others added 2 commits August 2, 2024 13:31
@jemrobinson
Copy link
Member Author

Thoughts on the hard-coded list of allowed locations @JimMadge @craddm ?

@JimMadge
Copy link
Member

JimMadge commented Aug 2, 2024

Thoughts on the hard-coded list of allowed locations @JimMadge @craddm ?

I almost commented on that. I think it is a sensible compromise.

We could use az CLI to get the list, but that would require signing in, be difficult to test in CI, and slow down the code.

@jemrobinson jemrobinson merged commit 78ba7b7 into develop Aug 2, 2024
11 checks passed
@jemrobinson jemrobinson deleted the tresa-testing branch August 2, 2024 14:01
@craddm
Copy link
Contributor

craddm commented Aug 2, 2024

Thoughts on the hard-coded list of allowed locations @JimMadge @craddm ?

I almost commented on that. I think it is a sensible compromise.

We could use az CLI to get the list, but that would require signing in, be difficult to test in CI, and slow down the code.

Same, I had a look if you could get a list using the Python SDK and couldn't see an obvious one, and only one mentioned on stackoverflow that would require logging in

@jemrobinson
Copy link
Member Author

We have a function for this in our AzureSDK here:

def get_locations(self) -> list[str]:
"""Retrieve list of Azure locations
Returns:
List[str]: Names of Azure locations
"""
try:
subscription_client = SubscriptionClient(self.credential())
return [
str(location.name)
for location in cast(
list[Location],
subscription_client.subscriptions.list_locations(
subscription_id=self.subscription_id
),
)
]
except AzureError as exc:
msg = "Azure locations could not be loaded."
raise DataSafeHavenAzureError(msg) from exc

However, this means you'd need to log in to the Azure CLI before being able to validate location and I wasn't sure whether this was a good idea or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants