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.

Sign up for GitHub

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

[DCJ-386] Register Azure Managed Resource Group with SAM #1697

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
+410 −11
Draft

[DCJ-386] Register Azure Managed Resource Group with SAM #1697

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a16e7ca
feat: add dummy step with unit tests
fboulnois Jun 3, 2024
1b8e36b
feat: add managed resource group registration to iamService
fboulnois Jun 3, 2024
4f259e0
fix: ensure correct spelling of resource for test
fboulnois Jun 3, 2024
bc7c019
feat: add implementation of registering managed resource group
fboulnois Jun 4, 2024
9a693ca
fix: move mrg registration after profile step
fboulnois Jun 7, 2024
7cd628e
refactor: big refactor
fboulnois Jun 7, 2024
57bef66
feat: add unit tests for delete
fboulnois Jun 7, 2024
06a1409
docs: switch to spend profile
fboulnois Jun 7, 2024
dc28b86
fix: progress
fboulnois Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/main/java/bio/terra/service/auth/iam/IamService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.concurrent.TimeUnit;
import org.apache.commons.collections4.ListUtils;
import org.apache.commons.collections4.map.PassiveExpiringMap;
import org.broadinstitute.dsde.workbench.client.sam.model.ManagedResourceGroupCoordinates;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -442,4 +443,14 @@ public String signUrlForBlob(
AuthenticatedUserRequest userReq, String project, String path, Duration duration) {
return callProvider(() -> iamProvider.signUrlForBlob(userReq, project, path, duration));
}

public void registerManagedResourceGroup(
AuthenticatedUserRequest userReq,
String billingProfileId,
ManagedResourceGroupCoordinates managedResourceGroupCoordinates) {
callProvider(
() ->
iamProvider.azureCreateManagedResourceGroup(
userReq, billingProfileId, managedResourceGroupCoordinates));
}
}
14 changes: 14 additions & 0 deletions src/main/java/bio/terra/service/profile/ProfileService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.List;
import java.util.Set;
import java.util.UUID;
import org.broadinstitute.dsde.workbench.client.sam.model.ManagedResourceGroupCoordinates;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -303,4 +304,17 @@ public void verifyDeployedApplication(
+ "operation");
}
}

public void registerManagedResourceGroup(
BillingProfileRequestModel request, AuthenticatedUserRequest user) {
String billingProfileId = request.getBillingAccountId();
Copy link
Contributor

Choose a reason for hiding this comment

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

A BillingProfileRequestModel only has a billingAccountId supplied when a GCP billing profile is being created -- it's the ID of the GCP billing account to wrap. For Azure billing profile creation, it is unspecified. I suspect this is why your AzureIntegrationTests are failing with this error:

https://scans.gradle.com/s/da5olvhqbaima/tests/overview?outcome=FAILED

java.lang.AssertionError: Error validating azure billing profile create.  Got response: bio.terra.integration.DataRepoResponse@193cb0e9[response=bio.terra.integration.ObjectOrErrorResponse@5de6abf3[statusCode=500 INTERNAL_SERVER_ERROR,locationHeader=Optional.empty,errorObject=Optional[class ErrorModel { |  
-- | --
  | message: Message: Missing the required parameter 'billingProfileId' when calling createManagedResourceGroup(Async)

What this endpoint is expecting is the ID of the Sam spend-profile resource created in this flight. I believe that it needs the Sam resource to already exist. Do you know where that Sam resource creation takes place in the flight?

Copy link
Contributor Author

@fboulnois fboulnois Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

CreateProfileAuthzIamStep seems to ultimately call samResourceApi.createResourceV2(IamResourceType.SPEND_PROFILE.toString(), req);. req uses request.getId().toString() as the profileId. However, .getId() is deprecated, so this is a bit confusing to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's a bit confusing!

Here's the past PR where we deprecated this ID in the request, with more context on the history: #1370

The first step in the billing profile creation flight will generate a UUID for the sam-profile and stores it in the request object if one doesn't already exist:

jade-data-repo/src/main/java/bio/terra/service/profile/flight/create/GetOrCreateProfileIdStep.java

Lines 18 to 21 in d8d53ce

UUID profileId = request.getId();
if (profileId == null) {
request.setId(UUID.randomUUID());
}

But the ID is only supplied to Sam to create the resource in Sam later in the flight, which looks to happen in CreateProfileAuthzIamStep.

ManagedResourceGroupCoordinates managedResourceGroupCoordinates =
new ManagedResourceGroupCoordinates()
.tenantId(request.getTenantId().toString())
.subscriptionId(request.getSubscriptionId().toString())
.managedResourceGroupName(request.getResourceGroupName());
Copy link
Contributor

Choose a reason for hiding this comment

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

I had left this remark in the ticket:

The format of the MRG coordinates that TDR billing profile creation expects from the caller may not match exactly the format expected by the Sam endpoint.

I was specifically wondering if TDR's BillingProfileRequestModel.resourceGroupName is the expected value for managedResourceGroupName. In TDR Azure architecture, the user creates a Resource Group in which to deploy their TDR managed app. The TDR managed app manages a new Managed Resource Group. I think TDR billing profile creation takes in the user-created Resource Group name, but the Sam endpoint takes in Managed Resource Group name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not really sure how to address this issue. The request doesn't seem to contain the "real" Managed Resource Group name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Azure service has the following:

var properties = objectMapper.valueToTree(applicationDeployment.properties());
var managedResourceGroupId = properties.get(MANAGED_RESOURCE_GROUP_ID_KEY)

I'm not sure if this is safe to access from the running steps, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are on the right track. The request body may not contain the MRG name, but this is something TDR can obtain through interactions with the Azure SDK… and the snippet you found shows that there is precedence elsewhere in the code for doing just that.

(BTW -- where possible to link to snippets from the existing codebase, that would be much appreciated to make sure that we're looking at the same thing, and leave a trail of breadcrumbs for those perusing this PR in the future.)

The snippet you shared looks to come from AzureApplicationDeploymentService.newApplicationDeployment which is called only in AzureApplicationDeploymentService.getOrRegisterApplicationDeployment.

jade-data-repo/src/main/java/bio/terra/service/resourcemanagement/azure/AzureApplicationDeploymentService.java

Line 88 in 3198570

return newApplicationDeployment(billingProfile);

getOrRegisterApplicationDeployment is presently only called when creating datasets and snapshots, as part of getting or creating their Azure storage accounts.

I may be missing historical reasoning, but it makes more sense to me to register an application deployment as part of creating an Azure billing profile, as a step within the flight that can also delete the application deployment registration in its undoStep definition. Then the remainder of the flight can rely on having the MRG name available.

Any future attempts to create billing profiles from the same MRG would fail fast at billing profile creation rather than later on at dataset creation, which seems to be the desired effect:

jade-data-repo/src/main/java/bio/terra/service/resourcemanagement/azure/AzureApplicationDeploymentService.java

Lines 75 to 81 in 3198570

throw new MismatchedBillingProfilesException(
"Cannot reuse existing application deployment "
+ applicationResource.getAzureApplicationDeploymentName()
+ " from profile "
+ applicationResource.getProfileId()
+ " with a different profile "
+ billingProfile.getId());


iamService.registerManagedResourceGroup(
user, billingProfileId, managedResourceGroupCoordinates);
}
}
38 changes: 38 additions & 0 deletions src/main/java/bio/terra/service/profile/flight/create/CreateProfileManagedResourceGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package bio.terra.service.profile.flight.create;

import bio.terra.common.iam.AuthenticatedUserRequest;
import bio.terra.model.BillingProfileRequestModel;
import bio.terra.service.profile.ProfileService;
import bio.terra.stairway.FlightContext;
import bio.terra.stairway.Step;
import bio.terra.stairway.StepResult;
import bio.terra.stairway.exception.RetryException;

public class CreateProfileManagedResourceGroup implements Step {

private final ProfileService profileService;
private final BillingProfileRequestModel request;
private final AuthenticatedUserRequest user;

public CreateProfileManagedResourceGroup(
ProfileService profileService,
BillingProfileRequestModel request,
AuthenticatedUserRequest user) {
this.profileService = profileService;
this.request = request;
this.user = user;
}

@Override
public StepResult doStep(FlightContext flightContext)
throws InterruptedException, RetryException {
profileService.registerManagedResourceGroup(request, user);
Copy link
Contributor

Choose a reason for hiding this comment

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

I had left this question in the ticket:

What is the Sam endpoint’s behavior when a billing profile already exists for the MRG coordinates?

Looking at the documented responses, it will return a 409: https://sam.dsde-prod.broadinstitute.org/#/Azure/createManagedResourceGroup

Flagging this as I'm not sure if this will end up proving problematic in the current setup for our AzureIntegrationTests. Let's revisit that after the next round of changes.

return StepResult.getStepResultSuccess();
}

@Override
public StepResult undoStep(FlightContext flightContext) throws InterruptedException {
// Registering the Managed Resource Group has no side effects to clean up
return StepResult.getStepResultSuccess();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There actually is a side effect to clean up: you can call Sam to delete the MRG registration. Here's that endpoint:

https://sam.dsde-prod.broadinstitute.org/#/Azure/deleteManagedResourceGroup

But you could ask in #dsp-identiteam if deleting the Sam profile resource would also delete any MRG registration associated with that billing profile. If that's the case, then a no-op undoStep here would be acceptable, because undoing the step which deletes the Sam profile resource would take care of this clean-up for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a larger problem with making thedeleteManagedResourceGroup call, which is that it must be called before deleting the billing profile in Sam (according to Doug). This means that the cleanup to CreateProfileAuthzIamStep will cause conflicts with Managed Resource Group undo step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a problem, but we do need to change where this step runs.

The createManagedResourceGroup endpoint must be called after creating the billing profile, and deleteManagedResourceGroup must be called before deleting the billing profile. So, this step must run after CreateProfileAuthzIamStep.

Then when a flight is moving forward:

  • CreateProfileAuthzIamStep.doStep creates the profile
  • CreateProfileManagedResourceGroupStep.doStep creates the association between profile and MRG coords

If a flight is rolled back, it walks backwards through the steps to undo:

  • CreateProfileManagedResourceGroupStep.undoStep deletes the association between profile and MRG coords
  • CreateProfileAuthzIamStep.undoStep deletes the profile

}
1 change: 1 addition & 0 deletions src/main/java/bio/terra/service/profile/flight/create/ProfileCreateFlight.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public ProfileCreateFlight(FlightMap inputParameters, Object applicationContext)
addStep(new CreateProfileVerifyAccountStep(profileService, request, user));
}
if (platform.isAzure()) {
addStep(new CreateProfileManagedResourceGroup(profileService, request, user));
addStep(new CreateProfileVerifyDeployedApplicationStep(profileService, request, user));
}
addStep(new CreateProfileAuthzIamStep(profileService, request, user));
Expand Down
1 change: 1 addition & 0 deletions src/test/java/bio/terra/flight/ProfileCreateFlightTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ void testConstructFlightAzure() {
contains(
"GetOrCreateProfileIdStep",
"CreateProfileMetadataStep",
"CreateProfileManagedResourceGroup",
"CreateProfileVerifyDeployedApplicationStep",
"CreateProfileAuthzIamStep",
"CreateProfileJournalEntryStep"));
Expand Down
18 changes: 18 additions & 0 deletions src/test/java/bio/terra/service/auth/iam/IamServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.List;
import java.util.Set;
import java.util.UUID;
import org.broadinstitute.dsde.workbench.client.sam.model.ManagedResourceGroupCoordinates;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -202,4 +203,21 @@ void testDeriveSnapshotPolicies() {
.readers(expectedReaders)
.discoverers(policies.getDiscoverers())));
}

@Test
void testRegisterManagedResourceGroup() throws InterruptedException {
String billingProfileId = "billingProfileId";
ManagedResourceGroupCoordinates managedResourceGroupCoordinates =
new ManagedResourceGroupCoordinates()
.tenantId("tenantId")
.subscriptionId("subscriptionId")
.managedResourceGroupName("managedResourceGroupName");

iamService.registerManagedResourceGroup(
TEST_USER, billingProfileId, managedResourceGroupCoordinates);

verify(iamProvider)
.azureCreateManagedResourceGroup(
TEST_USER, billingProfileId, managedResourceGroupCoordinates);
}
}
2 changes: 1 addition & 1 deletion src/test/java/bio/terra/service/auth/iam/sam/SamIamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ void testSignUrl() throws InterruptedException, ApiException {
}

@Test
void testAzureBillingProfileCreateManagedResourcieGroup()
void testAzureBillingProfileCreateManagedResourceGroup()
throws ApiException, InterruptedException {
mockSamAzureApi();
String billingProfileName = "billingProfileName";
Expand Down
27 changes: 27 additions & 0 deletions src/test/java/bio/terra/service/profile/ProfileServiceUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import bio.terra.service.profile.google.GoogleBillingService;
import bio.terra.service.resourcemanagement.exception.InaccessibleBillingAccountException;
import java.util.UUID;
import org.broadinstitute.dsde.workbench.client.sam.model.ManagedResourceGroupCoordinates;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -167,4 +168,30 @@ void testVerifyAccountNoAccess() {
InaccessibleBillingAccountException.class,
() -> profileService.verifyGoogleBillingAccount(id, user));
}

@Test
void testRegisterManagedResourceGroup() {
String billingProfileId = "billingProfileId";
UUID tenantId = UUID.fromString("bbbbbbbb-cccc-dddd-eeee-aaaaaaaaaaaa");
UUID subscriptionId = UUID.fromString("cccccccc-dddd-eeee-aaaa-bbbbbbbbbbbb");
String resourceGroupName = "resourceGroupName";

var billingProfileRequestModel = new BillingProfileRequestModel();
billingProfileRequestModel.setBillingAccountId(billingProfileId);
billingProfileRequestModel.setId(PROFILE_ID);
billingProfileRequestModel.setTenantId(tenantId);
billingProfileRequestModel.setSubscriptionId(subscriptionId);
billingProfileRequestModel.setResourceGroupName(resourceGroupName);

ManagedResourceGroupCoordinates managedResourceGroupCoordinates =
new ManagedResourceGroupCoordinates()
.tenantId(tenantId.toString())
.subscriptionId(subscriptionId.toString())
.managedResourceGroupName(resourceGroupName);

profileService.registerManagedResourceGroup(billingProfileRequestModel, user);

verify(iamService, times(1))
.registerManagedResourceGroup(user, billingProfileId, managedResourceGroupCoordinates);
}
}
52 changes: 52 additions & 0 deletions src/test/java/bio/terra/service/profile/azure/CreateProfileManagedResourceGroupStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package bio.terra.service.profile.azure;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.Mockito.verify;

import bio.terra.common.category.Unit;
import bio.terra.common.fixtures.AuthenticationFixtures;
import bio.terra.common.iam.AuthenticatedUserRequest;
import bio.terra.model.BillingProfileRequestModel;
import bio.terra.service.profile.ProfileService;
import bio.terra.service.profile.flight.create.CreateProfileManagedResourceGroup;
import bio.terra.stairway.FlightContext;
import bio.terra.stairway.StepResult;
import bio.terra.stairway.StepStatus;
import java.util.UUID;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
@Tag(Unit.TAG)
public class CreateProfileManagedResourceGroupStepTest {
@Mock private ProfileService profileService;
@Mock private FlightContext flightContext;

private static final AuthenticatedUserRequest TEST_USER =
AuthenticationFixtures.randomUserRequest();

private static final UUID BILLING_PROFILE_ID = UUID.randomUUID();

private BillingProfileRequestModel request;
private CreateProfileManagedResourceGroup step;

@BeforeEach
void setup() {
request = new BillingProfileRequestModel().id(BILLING_PROFILE_ID);
step = new CreateProfileManagedResourceGroup(profileService, request, TEST_USER);
}

@Test
void testDoAndUndoStep() throws InterruptedException {
StepResult doResult = step.doStep(flightContext);
assertThat(doResult.getStepStatus(), equalTo(StepStatus.STEP_RESULT_SUCCESS));
verify(profileService).registerManagedResourceGroup(request, TEST_USER);
StepResult undoResult = step.undoStep(flightContext);
assertThat(undoResult.getStepStatus(), equalTo(StepStatus.STEP_RESULT_SUCCESS));
}
}
Loading