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

DT-1202: Add "inherit steward" support to snapshot creation #1902

Merged
merged 13 commits into from
Feb 14, 2025
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,15 @@ Map<IamRole, String> createDatasetResource(
*
* @param userReq authenticated user
* @param snapshotId id of the snapshot
* @param parentId id of the parent dataset of the snapshot
* @param policies user emails to add as snapshot policy members
* @return Map of policy group emails for the snapshot policies
*/
Map<IamRole, String> createSnapshotResource(
AuthenticatedUserRequest userReq, UUID snapshotId, SnapshotRequestModelPolicies policies)
AuthenticatedUserRequest userReq,
UUID snapshotId,
UUID parentId,
pshapiro4broad marked this conversation as resolved.
Show resolved Hide resolved
SnapshotRequestModelPolicies policies)
throws InterruptedException;

/**
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/bio/terra/service/auth/iam/IamService.java
Original file line number Diff line number Diff line change
Expand Up @@ -285,12 +285,17 @@ public void deleteSnapshotResource(AuthenticatedUserRequest userReq, UUID snapsh
*
* @param userReq authenticated user
* @param snapshotId id of the snapshot
* @param parentDatasetId id of the snapshot's parent dataset
* @param policies user emails to add as snapshot policy members
* @return Map of policy group emails for the snapshot policies
*/
public Map<IamRole, String> createSnapshotResource(
AuthenticatedUserRequest userReq, UUID snapshotId, SnapshotRequestModelPolicies policies) {
return callProvider(() -> iamProvider.createSnapshotResource(userReq, snapshotId, policies));
AuthenticatedUserRequest userReq,
UUID snapshotId,
UUID parentDatasetId,
SnapshotRequestModelPolicies policies) {
return callProvider(
() -> iamProvider.createSnapshotResource(userReq, snapshotId, parentDatasetId, policies));
}

/**
Expand Down
31 changes: 25 additions & 6 deletions src/main/java/bio/terra/service/auth/iam/sam/SamIam.java
Original file line number Diff line number Diff line change
Expand Up @@ -263,25 +263,36 @@ private Map<IamRole, String> syncDatasetResourcePoliciesInner(

@Override
public Map<IamRole, String> createSnapshotResource(
AuthenticatedUserRequest userReq, UUID snapshotId, SnapshotRequestModelPolicies policies)
AuthenticatedUserRequest userReq,
UUID snapshotId,
UUID parentId,
SnapshotRequestModelPolicies policies)
throws InterruptedException {
SamRetry.retry(
configurationService, () -> createSnapshotResourceInnerV2(userReq, snapshotId, policies));
configurationService,
() -> createSnapshotResourceInnerV2(userReq, snapshotId, parentId, policies));
return SamRetry.retry(
configurationService, () -> syncSnapshotResourcePoliciesInner(userReq, snapshotId));
}

private void createSnapshotResourceInnerV2(
AuthenticatedUserRequest userReq, UUID snapshotId, SnapshotRequestModelPolicies policies)
AuthenticatedUserRequest userReq,
UUID snapshotId,
UUID parentId,
SnapshotRequestModelPolicies policies)
throws ApiException {
ResourcesApi samResourceApi = samApiService.resourcesApi(userReq.getToken());
CreateResourceRequestV2 req = createSnapshotResourceRequest(userReq, snapshotId, policies);
CreateResourceRequestV2 req =
createSnapshotResourceRequest(userReq, snapshotId, parentId, policies);
samResourceApi.createResourceV2(IamResourceType.DATASNAPSHOT.toString(), req);
}

@VisibleForTesting
CreateResourceRequestV2 createSnapshotResourceRequest(
AuthenticatedUserRequest userReq, UUID snapshotId, SnapshotRequestModelPolicies policies) {
AuthenticatedUserRequest userReq,
UUID snapshotId,
UUID parentId,
SnapshotRequestModelPolicies policies) {
policies = Optional.ofNullable(policies).orElse(new SnapshotRequestModelPolicies());
UserStatusInfo userStatusInfo = getUserInfoAndVerify(userReq);
CreateResourceRequestV2 req = new CreateResourceRequestV2().resourceId(snapshotId.toString());
Expand All @@ -306,7 +317,15 @@ CreateResourceRequestV2 createSnapshotResourceRequest(
createAccessPolicy(IamRole.AGGREGATE_DATA_READER, policies.getAggregateDataReaders()));

req.authDomain(List.of());
logger.debug("SAM request: " + req);

if (parentId != null) {
req.setParent(
new FullyQualifiedResourceId()
.resourceTypeName(IamResourceType.DATASET.toString())
.resourceId(parentId.toString()));
}

logger.debug("SAM request: {}", req);
return req;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ private SnapshotWorkingMapKeys() {}
public static final String SNAPSHOT_FIRECLOUD_GROUP_EMAIL = "snapshotFirecloudGroupEmail";
public static final String SNAPSHOT_DATA_ACCESS_CONTROL_GROUPS =
"snapshotDataAccessControlGroups";
public static final String SNAPSHOT_INHERIT_STEWARD_ENABLED = "snapshotInheritStewardEnabled";
}
Original file line number Diff line number Diff line change
@@ -1,28 +1,43 @@
package bio.terra.service.snapshot.flight.create;

import bio.terra.common.iam.AuthenticatedUserRequest;
import bio.terra.service.auth.iam.IamResourceType;
import bio.terra.service.auth.iam.IamRole;
import bio.terra.service.auth.iam.IamService;
import bio.terra.service.dataset.Dataset;
import bio.terra.service.resourcemanagement.ResourceService;
import bio.terra.service.snapshot.Snapshot;
import bio.terra.service.snapshot.SnapshotService;
import bio.terra.service.snapshot.flight.SnapshotWorkingMapKeys;
import bio.terra.stairway.FlightContext;
import bio.terra.stairway.FlightMap;
import bio.terra.stairway.Step;
import bio.terra.stairway.StepResult;
import com.fasterxml.jackson.core.type.TypeReference;
import java.util.Collections;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

public class SnapshotAuthzBqJobUserStep implements Step {
private final SnapshotService snapshotService;
private final ResourceService resourceService;
private final IamService sam;
private final AuthenticatedUserRequest request;
private final String snapshotName;
private final Dataset sourceDataset;

public SnapshotAuthzBqJobUserStep(
SnapshotService snapshotService, ResourceService resourceService, String snapshotName) {
SnapshotService snapshotService,
ResourceService resourceService,
IamService sam,
AuthenticatedUserRequest request,
String snapshotName,
Dataset sourceDataset) {
this.snapshotService = snapshotService;
this.resourceService = resourceService;
this.sam = sam;
this.request = request;
this.snapshotName = snapshotName;
this.sourceDataset = sourceDataset;
}

@Override
Expand All @@ -31,22 +46,31 @@ public StepResult doStep(FlightContext context) throws InterruptedException {
Map<IamRole, String> policyMap =
workingMap.get(SnapshotWorkingMapKeys.POLICY_MAP, new TypeReference<>() {});

Snapshot snapshot = snapshotService.retrieveByName(snapshotName);
String googleProjectId =
snapshotService.retrieveByName(snapshotName).getProjectResource().getGoogleProjectId();

// Allow the steward and reader to make queries in this project.
List<String> policyEmails =
new ArrayList<>(List.of(policyMap.get(IamRole.STEWARD), policyMap.get(IamRole.READER)));

Boolean inheritEnabled =
context
.getInputParameters()
.get(SnapshotWorkingMapKeys.SNAPSHOT_INHERIT_STEWARD_ENABLED, Boolean.class);
if (inheritEnabled != null && inheritEnabled) {
var datasetPolicyMap =
sam.retrievePolicyEmails(request, IamResourceType.DATASET, sourceDataset.getId());
// Allow the custodian to make queries in this project.
policyEmails.add(datasetPolicyMap.get(IamRole.CUSTODIAN));
}
// The underlying service provides retries so we do not need to retry this operation
resourceService.grantPoliciesBqJobUser(
snapshot.getProjectResource().getGoogleProjectId(),
Collections.singletonList(policyMap.get(IamRole.STEWARD)));
resourceService.grantPoliciesBqJobUser(
snapshot.getProjectResource().getGoogleProjectId(),
Collections.singletonList(policyMap.get(IamRole.READER)));
resourceService.grantPoliciesBqJobUser(googleProjectId, policyEmails);

return StepResult.getStepResultSuccess();
}

@Override
public StepResult undoStep(FlightContext context) throws InterruptedException {
public StepResult undoStep(FlightContext context) {
return StepResult.getStepResultSuccess();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import bio.terra.model.SnapshotRequestModelPolicies;
import bio.terra.service.auth.iam.IamRole;
import bio.terra.service.auth.iam.IamService;
import bio.terra.service.snapshot.SnapshotService;
import bio.terra.service.dataset.Dataset;
import bio.terra.service.snapshot.flight.SnapshotWorkingMapKeys;
import bio.terra.service.snapshot.flight.duos.SnapshotDuosFlightUtils;
import bio.terra.stairway.FlightContext;
Expand All @@ -23,23 +23,23 @@

public class SnapshotAuthzIamStep implements Step {
private final IamService sam;
private final SnapshotService snapshotService;
private final SnapshotRequestModel snapshotRequestModel;
private final AuthenticatedUserRequest userReq;
private final UUID snapshotId;
private final Dataset sourceDataset;
private static final Logger logger = LoggerFactory.getLogger(SnapshotAuthzIamStep.class);

public SnapshotAuthzIamStep(
IamService sam,
SnapshotService snapshotService,
SnapshotRequestModel snapshotRequestModel,
AuthenticatedUserRequest userReq,
UUID snapshotId) {
UUID snapshotId,
Dataset sourceDataset) {
this.sam = sam;
this.snapshotService = snapshotService;
this.snapshotRequestModel = snapshotRequestModel;
this.userReq = userReq;
this.snapshotId = snapshotId;
this.sourceDataset = sourceDataset;
}

@Override
Expand All @@ -57,8 +57,18 @@ public StepResult doStep(FlightContext context) throws InterruptedException {
workingMap.get(SnapshotWorkingMapKeys.SNAPSHOT_FIRECLOUD_GROUP_EMAIL, String.class);
derivedPolicies.addReadersItem(snapshotFirecloudGroupEmail);
}
final UUID parentDatasetId;
Boolean inheritEnabled =
context
.getInputParameters()
.get(SnapshotWorkingMapKeys.SNAPSHOT_INHERIT_STEWARD_ENABLED, Boolean.class);
if (inheritEnabled != null && inheritEnabled) {
parentDatasetId = sourceDataset.getId();
} else {
parentDatasetId = null;
}
Map<IamRole, String> policies =
sam.createSnapshotResource(userReq, snapshotId, derivedPolicies);
sam.createSnapshotResource(userReq, snapshotId, parentDatasetId, derivedPolicies);
workingMap.put(SnapshotWorkingMapKeys.POLICY_MAP, policies);
return StepResult.getStepResultSuccess();
}
Expand All @@ -71,7 +81,7 @@ public StepResult undoStep(FlightContext context) {
// when SAM deletes the ACL. How 'bout that!
} catch (UnauthorizedException ex) {
// suppress exception
logger.error("NEEDS CLEANUP: delete sam resource for snapshot " + snapshotId.toString());
logger.error("NEEDS CLEANUP: delete sam resource for snapshot {}", snapshotId);
logger.warn(ex.getMessage());
} catch (NotFoundException ex) {
// suppress exception
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@

import bio.terra.common.FlightUtils;
import bio.terra.common.exception.PdaoException;
import bio.terra.common.iam.AuthenticatedUserRequest;
import bio.terra.service.auth.iam.IamResourceType;
import bio.terra.service.auth.iam.IamRole;
import bio.terra.service.auth.iam.IamService;
import bio.terra.service.configuration.ConfigurationService;
import bio.terra.service.dataset.Dataset;
import bio.terra.service.snapshot.Snapshot;
import bio.terra.service.snapshot.SnapshotService;
import bio.terra.service.snapshot.flight.SnapshotWorkingMapKeys;
Expand All @@ -28,17 +32,26 @@ public class SnapshotAuthzTabularAclStep implements Step {
private final BigQuerySnapshotPdao bigQuerySnapshotPdao;
private final SnapshotService snapshotService;
private final ConfigurationService configService;
private final IamService iamService;
private final UUID snapshotId;
private final AuthenticatedUserRequest userReq;
private final Dataset sourceDataset;

public SnapshotAuthzTabularAclStep(
BigQuerySnapshotPdao bigQuerySnapshotPdao,
SnapshotService snapshotService,
ConfigurationService configService,
UUID snapshotId) {
IamService iamService,
UUID snapshotId,
AuthenticatedUserRequest userReq,
Dataset sourceDataset) {
this.bigQuerySnapshotPdao = bigQuerySnapshotPdao;
this.snapshotService = snapshotService;
this.configService = configService;
this.snapshotId = snapshotId;
this.userReq = userReq;
this.iamService = iamService;
this.sourceDataset = sourceDataset;
}

@Override
Expand All @@ -53,6 +66,16 @@ public StepResult doStep(FlightContext context) throws InterruptedException {
emails.add(policies.get(IamRole.STEWARD));
emails.add(policies.get(IamRole.READER));

Boolean inheritEnabled =
context
.getInputParameters()
.get(SnapshotWorkingMapKeys.SNAPSHOT_INHERIT_STEWARD_ENABLED, Boolean.class);
if (inheritEnabled != null && inheritEnabled) {
var datasetPolicyMap =
iamService.retrievePolicyEmails(userReq, IamResourceType.DATASET, sourceDataset.getId());
emails.add(datasetPolicyMap.get(IamRole.CUSTODIAN));
}

Comment on lines +69 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pull this into a common method somewhere?

Copy link
Member Author

@pshapiro4broad pshapiro4broad Feb 11, 2025

Choose a reason for hiding this comment

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

This code is temporary until https://broadworkbench.atlassian.net/browse/DT-1197 is done. That will replace this code, which reads from the flight input map, to read from a field in sourceDataset. I'm expecting it would look something like

    if (sourceDataset.inheritSteward()) {
      var datasetPolicyMap =
          iamService.retrievePolicyEmails(userReq, IamResourceType.DATASET, sourceDataset.getId());
      emails.add(datasetPolicyMap.get(IamRole.CUSTODIAN));
    }

So I think a lot of it will go away. If there's still a need to refactor common code it can be done then.

try {
if (configService.testInsertFault(SNAPSHOT_GRANT_ACCESS_FAULT)) {
throw new BigQueryException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ public SnapshotCreateFlight(FlightMap inputParameters, Object applicationContext

// Create the IAM resource and readers for the snapshot
// The IAM code contains retries, so we don't make a retry rule here.
addStep(new SnapshotAuthzIamStep(iamClient, snapshotService, snapshotReq, userReq, snapshotId));
addStep(new SnapshotAuthzIamStep(iamClient, snapshotReq, userReq, snapshotId, sourceDataset));

// Now that the snapshot exists in Sam, we can add data access control groups to the snapshot
addStep(new CreateSnapshotSetDataAccessGroupsStep(snapshotReq.getDataAccessControlGroups()));
Expand Down Expand Up @@ -350,7 +350,13 @@ public SnapshotCreateFlight(FlightMap inputParameters, Object applicationContext
// Apply the IAM readers to the BQ dataset
addStep(
new SnapshotAuthzTabularAclStep(
bigQuerySnapshotPdao, snapshotService, configService, snapshotId),
bigQuerySnapshotPdao,
snapshotService,
configService,
iamService,
snapshotId,
userReq,
sourceDataset),
pdaoAclRetryRule);

// Apply the IAM readers to the GCS files
Expand All @@ -361,7 +367,9 @@ public SnapshotCreateFlight(FlightMap inputParameters, Object applicationContext
pdaoAclRetryRule);
}

addStep(new SnapshotAuthzBqJobUserStep(snapshotService, resourceService, snapshotName));
addStep(
new SnapshotAuthzBqJobUserStep(
snapshotService, resourceService, iamService, userReq, snapshotName, sourceDataset));
addStep(
new SnapshotAuthzServiceAccountConsumerStep(
snapshotService, resourceService, snapshotName, tdrServiceAccountEmail));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
import bio.terra.stairway.FlightContext;
import bio.terra.stairway.Step;
import bio.terra.stairway.StepResult;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -43,14 +44,27 @@ public StepResult doStep(FlightContext context) throws InterruptedException {
Snapshot snapshot = snapshotService.retrieve(snapshotId);

// These policy emails should not change since the snapshot is locked by the flight
Map<IamRole, String> policyEmails =
sam.retrievePolicyEmails(userReq, IamResourceType.DATASNAPSHOT, snapshotId);
List<String> policyEmails =
sam
.retrievePolicyEmails(userReq, IamResourceType.DATASNAPSHOT, snapshotId)
.entrySet()
.stream()
.filter(entry -> entry.getKey() == IamRole.STEWARD || entry.getKey() == IamRole.READER)
.map(Map.Entry::getValue)
.collect(Collectors.toList());

// Remove the custodian's access to make queries in this project.
// If the dataset custodian inherited permissions, remove them now.
var datasetPolicyEmails =
sam.retrievePolicyEmails(
userReq, IamResourceType.DATASET, snapshot.getSourceDataset().getId());
if (datasetPolicyEmails.containsKey(IamRole.CUSTODIAN)) {
policyEmails.add(datasetPolicyEmails.get(IamRole.CUSTODIAN));
}

// Remove access added by SnapshotAuthzBqJobUserStep.
// The underlying service provides retries so we do not need to retry this operation
resourceService.revokePoliciesBqJobUser(
snapshot.getProjectResource().getGoogleProjectId(),
Arrays.asList(policyEmails.get(IamRole.STEWARD), policyEmails.get(IamRole.READER)));
snapshot.getProjectResource().getGoogleProjectId(), policyEmails);

return StepResult.getStepResultSuccess();
}
Expand Down
Loading
Loading