Ia 4799 cloned workflows apps fail to upgrade

[cindy-broadinstitute]

Jira ticket: https://broadworkbench.atlassian.net/browse/IA-4799

Summary of changes

What

• create records in APP_CONTROLLED_RESOURCE when apps are cloned
• do a migration to back populate these records

Why

• updating apps fails due to not finding databases in APP_CONTROLLED_RESOURCE when a workspace has been cloned

Testing these changes

What to test

Who tested and where

• This change is covered by automated tests
  • NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
• I validated this change
• Primary reviewer validated this change
• I validated this change in the dev environment

[codecov]

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 71.74%. Comparing base (ed4e5dd) to head (abd37eb).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4155      +/-   ##
===========================================
+ Coverage    71.70%   71.74%   +0.03%     
===========================================
  Files          154      154              
  Lines        14255    14287      +32     
  Branches      1134     1150      +16     
===========================================
+ Hits         10222    10250      +28     
- Misses        4033     4037       +4     

Flag	Coverage Δ
pact	48.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...nstitute/dsde/workbench/leonardo/azureModels.scala	0.00% <ø> (ø)
...h/leonardo/db/AppControlledResourceComponent.scala	100.00% <ø> (ø)
.../dsde/workbench/leonardo/util/AKSInterpreter.scala	85.74% <82.35%> (+0.11%)	⬆️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed4e5dd...abd37eb. Read the comment docs.

[rtitle]

One comment, but the change looks correct to me 👍

Question -- how will we backfill the existing records? I was assuming this PR would have a liquibase migration?

[rtitle]

Sorry, some more comments above about the approach. Happy to talk with faces too.

I'm wondering if this approach would work:

1. In createOrFetch[Identity|Database] (which is called from createApp), make sure we populate APP_CONTROLLED_RESOURCE for the clone and non-clone cases. This should ensure correct records for new apps going forward.
2. To back-populate existing apps, add some logic in updateAndPollApp:

   If it's a shared app and wsmIdentityOpt == None, retrieve the managed identity in WSM and populate the APP_CONTROLLED_RESOURCE table.
   
   If it's an X app type and doesn't have X's required databases, retrieve the databases in WSM and populate the APP_CONTROLLED_RESOURCE table.
   

Then the next upgrade should "fix" the APP_CONTROLLED_RESOURCE table. Note that we also rely on that table for app deletion (to know which WSM resources to delete). So I think we're also at risk of leaking cloud resources today when we delete apps. Luckily those resources do get deleted at workspace deletion time.

[cindy-broadinstitute]

I'm wondering if this approach would work:

1. In createOrFetch[Identity|Database] (which is called from createApp), make sure we populate APP_CONTROLLED_RESOURCE for the clone and non-clone cases. This should ensure correct records for new apps going forward.
2. To back-populate existing apps, add some logic in updateAndPollApp:

   If it's a shared app and wsmIdentityOpt == None, retrieve the managed identity in WSM and populate the APP_CONTROLLED_RESOURCE table.
   
   If it's an X app type and doesn't have X's required databases, retrieve the databases in WSM and populate the APP_CONTROLLED_RESOURCE table.
   

That was what I was attempting to do. My confusion was with the ManagedIdentity needed to process the createOrFetchWsmDatabaseResources() which I thought would be fine to use knowing that in the update flow the tables would already be created so it would just fetch them.

As I mentioned above, I will go back to considering the migration instead of trying to update old records on the fly. It does seem like the cleaner approach. I might like to discuss the optimal way to do that.

[rtitle]

As I mentioned above, I will go back to considering the migration instead of trying to update old records on the fly. It does seem like the cleaner approach. I might like to discuss the optimal way to do that.

Sounds good - thank you! Happy to discuss.

[rtitle]

Names are unique within a workspace, and there can only be 1 shared app of a given type per workspace, so this works. Agree about thinking about future options though.

Also would be good to move away from using WSM enumerateResources in favor of a getByName API or something. That has implications on auth domains I think (cc @aherbst-broad )

[rtitle]

Cool, seems like this approach works ✅