microsoft / AzureTRE

An accelerator to help organizations build Trusted Research Environments on Azure.
https://microsoft.github.io/AzureTRE
MIT License
184 stars 143 forks source link

Cannot update workspace with custom address space #3691

Closed jlabhard-sg closed 11 months ago

jlabhard-sg commented 1 year ago

Describe the bug Creating a workspace with a custom address space and then updating it will fail with The custom 'address_space' you requested does not fit in the current network. This is because the method workspaces.validate_address_space() will check the allocated network on update, find that the network is already allocated (to the current workspace), and return the error.

The same error arises when creating a workspace and specifying a custom address space which overlaps with a workspace that has been deleted.

Steps to reproduce

  1. Create a workspace with custom address space
  2. Update it without any changes

Suggested changes

marrobi commented 1 year ago

@jlabhard-sg thanks for reporing. Not many people use custom address spaces as far as I am aware.

Updating should not be possible as you can't change the address space once it has things on it. Do you agree?

Can you try changing:

https://github.com/marrobi/AzureTRE/blob/main/templates/workspaces/base/template_schema.json#L52-L53

To false and see if that sorts things out?

jlabhard-sg commented 1 year ago

Hi @marrobi , Yes I agree, my issue arises when I update the workspace without changing the address space. I will try your suggested change to see if it fixes it and update you here.

jlabhard-sg commented 1 year ago

@marrobi Setting the address_spaces properties to non-updateable did not solve the issue.

marrobi commented 1 year ago

Ok, it will need further debugging/time to look for an appropriate solution.

If you can come up with a working solution, welcome a contribution, it's unlikely we will have chance to dig into it for a while. I tend to think not validating the address space if it has not changed/is and update , makes most sense.

marrobi commented 1 year ago

@jlabhard-sg just taking a look at this, I'm trying to work out the code path that results in validate_address_space being called on an update.

It is called in get_address_space_based_on_size which is called only on create, here -https://github.com/marrobi/AzureTRE/blob/0c5d945f84086daf479a906b2e362d2ae544702b/api_app/db/repositories/workspaces.py#L75

II will try to reproduce myself, but confused as to why this would occur.

jlabhard-sg commented 1 year ago

@marrobi It is possible that I mixed the error message from two different issues when looking for them in the app insights. "Does not fit in the current network" probably happened when trying to create a workspace with an address space that was used by a deleted workspace. I believe this is still undesirable.

However, updating a workspace with custom address space also doesn't work. But the only error message I can find is "Unevaluated properties is not allowed". Currently, I am able to bypass it by updating the resource in CosmosDB and using the swagger UI to update the workspace.

marrobi commented 1 year ago

Ok, other than the excluding deleted the issue is as address_space_size is not updatable, its not passed with the patch payload. This means that validation fails as it's not expecting the actual address space size in the payload, as its only accepted if address space size is in the payload as custom.

address_space should not be updatable, but read only, this is configured here:

https://github.com/microsoft/AzureTRE/blob/63aa23d9515925c52647930f2bf342c530fae344/api_app/services/schema_service.py#L51-L54

We are only iterating through the top level properties and making fields read only, not the allOf ones, which address space is.

marrobi commented 1 year ago

@jlabhard-sg any change you can build the image in https://github.com/microsoft/AzureTRE/pull/3714 and give it a go and see if the issue is resolved.

I've given it a go and is good for me. Thanks.

jlabhard-sg commented 1 year ago

@marrobi Was able to test and validate the fix in #3714. Thank you!

marrobi commented 1 year ago

Great, might take a little while to get it merged in, but good to know it works.