From 82e2fcb11851bd5bcb120b70828c053c30fca25b Mon Sep 17 00:00:00 2001 From: JonMiguel Jara Date: Wed, 19 Aug 2020 10:22:25 -0700 Subject: [PATCH] Addressing feedback and getting all the contract tests to pass. --- .../aws-codeartifact-domain.json | 15 ++-- aws-codeartifact-domain/overrides.json | 17 ++++ .../codeartifact/domain/BaseHandlerStd.java | 80 +++++++++++-------- .../amazon/codeartifact/domain/Constants.java | 12 +-- .../codeartifact/domain/CreateHandler.java | 14 ++++ .../codeartifact/domain/DeleteHandler.java | 75 +++++++++-------- .../codeartifact/domain/ListHandler.java | 3 +- .../codeartifact/domain/Translator.java | 33 ++++---- .../codeartifact/domain/UpdateHandler.java | 37 +++++++-- .../codeartifact/domain/AbstractTestBase.java | 9 ++- .../domain/CreateHandlerTest.java | 14 ++-- .../domain/DeleteHandlerTest.java | 67 +++++++++++++--- .../domain/UpdateHandlerTest.java | 53 ++++++++++-- 13 files changed, 296 insertions(+), 133 deletions(-) create mode 100644 aws-codeartifact-domain/overrides.json diff --git a/aws-codeartifact-domain/aws-codeartifact-domain.json b/aws-codeartifact-domain/aws-codeartifact-domain.json index 837645a..3d35c9e 100644 --- a/aws-codeartifact-domain/aws-codeartifact-domain.json +++ b/aws-codeartifact-domain/aws-codeartifact-domain.json @@ -6,7 +6,7 @@ "DomainName": { "description": "The name of the domain.", "type": "string", - "pattern": "[a-z][a-z0-9\\-]{0,48}[a-z0-9]", + "pattern": "^([a-z][a-z0-9\\-]{0,48}[a-z0-9])$", "minLength": 2, "maxLength": 50 }, @@ -39,12 +39,9 @@ ], "type": "string" }, - "PolicyDocument": { + "PermissionsPolicyDocument": { "description": "The access control resource policy on the provided domain.", - "type": [ - "object", - "string" - ], + "type": "object", "minLength": 2, "maxLength": 5120 }, @@ -56,6 +53,9 @@ } }, "additionalProperties": false, + "required": [ + "DomainName" + ], "createOnlyProperties": [ "/properties/DomainName", "/properties/EncryptionKey" @@ -64,7 +64,8 @@ "/properties/Arn", "/properties/CreatedTime", "/properties/RepositoryCount", - "/properties/AssetSizeBytes" + "/properties/AssetSizeBytes", + "/properties/DomainOwner" ], "primaryIdentifier": [ "/properties/Arn" diff --git a/aws-codeartifact-domain/overrides.json b/aws-codeartifact-domain/overrides.json new file mode 100644 index 0000000..0bd1472 --- /dev/null +++ b/aws-codeartifact-domain/overrides.json @@ -0,0 +1,17 @@ +{ + "CREATE": { + "/PermissionsPolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": "*", + "Action": "codeartifact:*", + "Resource": "*" + } + ] + }, + "/EncryptionKey": null, + "/DomainOwner": null + } +} diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java index e2b7467..3486896 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/BaseHandlerStd.java @@ -1,9 +1,10 @@ package software.amazon.codeartifact.domain; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; + import software.amazon.awssdk.awscore.exception.AwsServiceException; import software.amazon.awssdk.services.codeartifact.CodeartifactClient; -import software.amazon.awssdk.services.codeartifact.model.DescribeDomainResponse; -import software.amazon.awssdk.services.codeartifact.model.GetDomainPermissionsPolicyRequest; import software.amazon.awssdk.services.codeartifact.model.PutDomainPermissionsPolicyResponse; import software.amazon.awssdk.services.codeartifact.model.ResourceNotFoundException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; @@ -11,12 +12,9 @@ import software.amazon.cloudformation.proxy.ProgressEvent; import software.amazon.cloudformation.proxy.ProxyClient; import software.amazon.cloudformation.proxy.ResourceHandlerRequest; -import software.amazon.cloudformation.proxy.delay.Constant; -import org.apache.http.HttpStatus; -import java.time.Duration; public abstract class BaseHandlerStd extends BaseHandler { - + public static final ObjectMapper MAPPER = new ObjectMapper(); @Override public final ProgressEvent handleRequest( final AmazonWebServicesClientProxy proxy, @@ -47,40 +45,54 @@ protected ProgressEvent putDomainPermissionsPoli final ProxyClient proxyClient, final Logger logger ) { - final ResourceModel desiredModel = progress.getResourceModel(); - if (desiredModel.getPolicyDocument() == null) { - return ProgressEvent.progress(desiredModel, callbackContext); - } - return proxy.initiate("AWS-CodeArtifact-Domain::Update::PutDomainPermissionsPolicy", proxyClient, - progress.getResourceModel(), progress.getCallbackContext()) - .translateToServiceRequest(Translator::translatePutDomainPolicyRequest) - .makeServiceCall((awsRequest, client) -> { - PutDomainPermissionsPolicyResponse awsResponse = null; - try { - awsResponse = client.injectCredentialsAndInvokeV2(awsRequest, client.client()::putDomainPermissionsPolicy); - logger.log("Domain permission policy successfully added."); - } catch (final AwsServiceException e) { - String domainName = desiredModel.getDomainName(); - Translator.throwCfnException(e, Constants.PUT_DOMAIN_POLICY, domainName); - } - return awsResponse; - }) - .stabilize((awsRequest, awsResponse, client, model, context) -> domainPolicyIsStabilized(model, client, request)) - .progress(); + + final ResourceModel desiredModel = progress.getResourceModel(); + final ResourceModel previousModel = request.getPreviousResourceState(); + + if (desiredModel.getPermissionsPolicyDocument() == null || policyIsUnchanged(desiredModel, previousModel)) { + return ProgressEvent.progress(desiredModel, callbackContext); + } + return proxy.initiate("AWS-CodeArtifact-Domain::Update::PutDomainPermissionsPolicy", proxyClient, progress.getResourceModel(), progress.getCallbackContext()) + .translateToServiceRequest(Translator::translatePutDomainPolicyRequest) + .makeServiceCall((awsRequest, client) -> { + PutDomainPermissionsPolicyResponse awsResponse = null; + try { + awsResponse = client.injectCredentialsAndInvokeV2(awsRequest, client.client()::putDomainPermissionsPolicy); + logger.log("Domain permission policy successfully added."); + } catch (final AwsServiceException e) { + String domainName = desiredModel.getDomainName(); + Translator.throwCfnException(e, Constants.PUT_DOMAIN_POLICY, domainName); + } + return awsResponse; + }) + .progress(); } - private boolean domainPolicyIsStabilized( + boolean policyIsUnchanged(final ResourceModel desiredModel, final ResourceModel previousModel) { + if (previousModel == null) { + return false; + } + + if (desiredModel.getPermissionsPolicyDocument() == null || previousModel.getPermissionsPolicyDocument() == null) { + return false; + } + JsonNode desiredPolicy = MAPPER.valueToTree(desiredModel.getPermissionsPolicyDocument()); + JsonNode currentPolicy = MAPPER.valueToTree(previousModel.getPermissionsPolicyDocument()); + return desiredPolicy.equals(currentPolicy); + } + + protected boolean doesDomainExist( final ResourceModel model, final ProxyClient proxyClient, ResourceHandlerRequest request ) { - try { - proxyClient.injectCredentialsAndInvokeV2( - Translator.translateToGetDomainPermissionPolicy(model, request), proxyClient.client()::getDomainPermissionsPolicy); - return true; - } catch (ResourceNotFoundException e) { - return false; - } + try { + proxyClient.injectCredentialsAndInvokeV2( + Translator.translateToReadRequest(model, request), proxyClient.client()::describeDomain); + return true; + } catch (ResourceNotFoundException e) { + return false; + } } } diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Constants.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Constants.java index a8ef9bd..d096a92 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Constants.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Constants.java @@ -2,12 +2,12 @@ public class Constants { - public static final String CREATE_DOMAIN = "codeartifact::CreateDomain"; - public static final String LIST_DOMAINS = "codeartifact::ListDomains"; - public static final String PUT_DOMAIN_POLICY = "codeartifact::PutDomainPermissionsPolicy"; - public static final String DELETE_DOMAIN_PERMISSION_POLICY = "codeartifact::DeleteDomainPermissionsPolicy"; - public static final String DELETE_DOMAIN = "codeartifact::DeleteDomain"; - public static final String DESCRIBE_DOMAIN = "codeartifact::DescribeDomain"; + public static final String CREATE_DOMAIN = "codeartifact:CreateDomain"; + public static final String LIST_DOMAINS = "codeartifact:ListDomains"; + public static final String PUT_DOMAIN_POLICY = "codeartifact:PutDomainPermissionsPolicy"; + public static final String DELETE_DOMAIN_PERMISSION_POLICY = "codeartifact:DeleteDomainPermissionsPolicy"; + public static final String DELETE_DOMAIN = "codeartifact:DeleteDomain"; + public static final String DESCRIBE_DOMAIN = "codeartifact:DescribeDomain"; public static final String ACTIVE_STATUS = "Active"; public static final int MAX_ITEMS = 1000; } diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/CreateHandler.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/CreateHandler.java index d51343c..caa4033 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/CreateHandler.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/CreateHandler.java @@ -5,6 +5,8 @@ import software.amazon.awssdk.services.codeartifact.model.CreateDomainRequest; import software.amazon.awssdk.services.codeartifact.model.CreateDomainResponse; import software.amazon.awssdk.services.codeartifact.model.DescribeDomainResponse; +import software.amazon.cloudformation.exceptions.CfnInvalidRequestException; +import software.amazon.cloudformation.exceptions.CfnNotUpdatableException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; import software.amazon.cloudformation.proxy.Logger; import software.amazon.cloudformation.proxy.ProgressEvent; @@ -22,6 +24,13 @@ protected ProgressEvent handleRequest( final ProxyClient proxyClient, final Logger logger) { + ResourceModel model = request.getDesiredResourceState(); + + // Make sure the user isn't trying to assign values to readOnly properties + if (hasReadOnlyProperties(model)) { + throw new CfnInvalidRequestException("Attempting to set a ReadOnly Property."); + } + this.logger = logger; return ProgressEvent.progress(request.getDesiredResourceState(), callbackContext) .then(progress -> createDomain(proxy, progress, request, proxyClient)) @@ -59,6 +68,11 @@ private CreateDomainResponse createDomainSdkCall( return awsResponse; } + private boolean hasReadOnlyProperties(final ResourceModel model) { + return model.getAssetSizeBytes() != null || model.getRepositoryCount() != null + || model.getCreatedTime() != null || model.getDomainOwner() != null; + } + private boolean isStabilized( final ResourceModel model, final ProxyClient proxyClient, diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java index 434fc82..7078f3c 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/DeleteHandler.java @@ -1,13 +1,19 @@ package software.amazon.codeartifact.domain; +import groovy.grape.GrapeIvy._closure1; import software.amazon.awssdk.awscore.AwsResponse; import software.amazon.awssdk.awscore.exception.AwsServiceException; import software.amazon.awssdk.core.SdkClient; import software.amazon.awssdk.services.codeartifact.CodeartifactClient; +import software.amazon.awssdk.services.codeartifact.model.ConflictException; import software.amazon.awssdk.services.codeartifact.model.DescribeDomainResponse; import software.amazon.awssdk.services.codeartifact.model.ResourceNotFoundException; import software.amazon.cloudformation.exceptions.CfnGeneralServiceException; +import software.amazon.cloudformation.exceptions.CfnNotFoundException; +import software.amazon.cloudformation.exceptions.CfnResourceConflictException; +import software.amazon.cloudformation.exceptions.TerminalException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; +import software.amazon.cloudformation.proxy.HandlerErrorCode; import software.amazon.cloudformation.proxy.Logger; import software.amazon.cloudformation.proxy.ProgressEvent; import software.amazon.cloudformation.proxy.ProxyClient; @@ -24,44 +30,43 @@ protected ProgressEvent handleRequest( final Logger logger) { this.logger = logger; + ResourceModel model = request.getDesiredResourceState(); + // STEP 1.0 [initialize a proxy context] + ProgressEvent test = proxy + .initiate("AWS-CodeArtifact-Domain::Delete", proxyClient, model, callbackContext) + // STEP 1.1 [construct a body of a request] + .translateToServiceRequest(Translator::translateToDeleteRequest) + // STEP 1.2 [make an api call] + .makeServiceCall((awsRequest, client) -> { - return ProgressEvent.progress(request.getDesiredResourceState(), callbackContext) + // if domain does not exist, deleteDomain does not throw an exception, so we must do this + // to be under the ResourceHandler Contract + if (!doesDomainExist(model, proxyClient, request)) { + throw new CfnNotFoundException(model.getDomainName(), ResourceModel.TYPE_NAME); + } - // STEP 1.0 [delete/stabilize progress chain - required for resource deletion] - .then(progress -> - // STEP 1.0 [initialize a proxy context] - proxy.initiate("AWS-CodeArtifact-Domain::Delete", proxyClient, progress.getResourceModel(), - progress.getCallbackContext()) - // STEP 1.1 [construct a body of a request] - .translateToServiceRequest(Translator::translateToDeleteRequest) - // STEP 1.2 [make an api call] - .makeServiceCall((awsRequest, client) -> { - AwsResponse awsResponse = null; - try { - awsResponse = client.injectCredentialsAndInvokeV2(awsRequest, proxyClient.client()::deleteDomain); - } catch (final AwsServiceException e) { - String domainName = progress.getResourceModel().getDomainName(); - Translator.throwCfnException(e, Constants.DELETE_DOMAIN, domainName); - } + AwsResponse awsResponse = null; + try { + awsResponse = client.injectCredentialsAndInvokeV2(awsRequest, proxyClient.client()::deleteDomain); + } catch (final ConflictException e) { + // this happens if the domain has repositories in the account + throw new CfnResourceConflictException(ResourceModel.TYPE_NAME, model.getDomainName(), e.getMessage(), e); + } catch (final AwsServiceException e) { + String domainName = model.getDomainName(); + Translator.throwCfnException(e, Constants.DELETE_DOMAIN, domainName); + } - logger.log(String.format("%s successfully deleted.", ResourceModel.TYPE_NAME)); - return awsResponse; - }) - // STEP 2.3 [Stabilize to check if the resource got deleted] - .stabilize((deleteDomainRequest, deleteDomainResponse, proxyInvocation, model, context) -> isDeleted(model, proxyInvocation, request)) - .success()); - } + logger.log(String.format("%s successfully deleted.", ResourceModel.TYPE_NAME)); + return awsResponse; + }) + // STEP 2.3 [Stabilize to check if the resource got deleted] + .stabilize((deleteDomainRequest, deleteDomainResponse, proxyInvocation, resourceModel, context) -> !doesDomainExist(model, proxyClient, request)) + .success(); - protected boolean isDeleted(final ResourceModel model, - final ProxyClient proxyClient, - ResourceHandlerRequest request - ) { - try { - proxyClient.injectCredentialsAndInvokeV2( - Translator.translateToReadRequest(model, request), proxyClient.client()::describeDomain); - return false; - } catch (ResourceNotFoundException e) { - return true; - } + + // according to the ResourceHandler contract we must not return the model in the response + test.setResourceModel(null); + return test; } + } diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/ListHandler.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/ListHandler.java index 3e44693..8e281f1 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/ListHandler.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/ListHandler.java @@ -46,9 +46,10 @@ public ProgressEvent handleRequest( String nextToken = response.nextToken(); return ProgressEvent.builder() - .resourceModels(Translator.translateFromListRequest(response)) + .resourceModels(Translator.translateFromListRequest(response, request)) .nextToken(nextToken) .status(OperationStatus.SUCCESS) .build(); } + } diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java index aea386c..5202514 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/Translator.java @@ -66,16 +66,15 @@ static CreateDomainRequest translateToCreateRequest(final ResourceModel model) { /** * Request to read a resource * @param model resource model - * @param model resource handler request + * @param request resource handler request * @return awsRequest the aws service request to describe a resource */ static DescribeDomainRequest translateToReadRequest( final ResourceModel model, final ResourceHandlerRequest request ) { - String accountId = request.getAwsAccountId(); return DescribeDomainRequest.builder() .domain(model.getDomainName()) - .domainOwner(accountId) + .domainOwner(model.getDomainOwner()) .build(); } @@ -88,10 +87,9 @@ static DescribeDomainRequest translateToReadRequest( static GetDomainPermissionsPolicyRequest translateToGetDomainPermissionPolicy( final ResourceModel model, final ResourceHandlerRequest request ) { - String accountId = request.getAwsAccountId(); return GetDomainPermissionsPolicyRequest.builder() .domain(model.getDomainName()) - .domainOwner(accountId) + .domainOwner(model.getDomainOwner()) .build(); } @@ -133,7 +131,7 @@ static DeleteDomainRequest translateToDeleteRequest(final ResourceModel model) { static PutDomainPermissionsPolicyRequest translatePutDomainPolicyRequest(final ResourceModel model) { try { return PutDomainPermissionsPolicyRequest.builder() - .policyDocument(translatePolicyInput(model.getPolicyDocument())) + .policyDocument(MAPPER.writeValueAsString(model.getPermissionsPolicyDocument())) .domainOwner(model.getDomainOwner()) .domain(model.getDomainName()) .build(); @@ -154,13 +152,6 @@ static DeleteDomainPermissionsPolicyRequest translateDeleteDomainPolicyRequest(f .build(); } - static String translatePolicyInput(final Object policy) throws JsonProcessingException { - if (policy instanceof Map) { - return MAPPER.writeValueAsString(policy); - } - return (String) policy; - } - /** * Request to list resources * @param nextToken token passed to the aws service list resources request @@ -178,15 +169,27 @@ static ListDomainsRequest translateToListRequest(final String nextToken) { * @param awsResponse the aws service describe resource response * @return list of resource models */ - static List translateFromListRequest(final ListDomainsResponse awsResponse) { + static List translateFromListRequest( + final ListDomainsResponse awsResponse, final ResourceHandlerRequest request + ) { return streamOfOrEmpty(awsResponse.domains()) .map(domain -> ResourceModel.builder() + // TODO remove this hardcode before submitting + .arn(buildArn(request, domain.owner(), domain.name())) // TODO change domainName to arn when CodeArtifactClient populates arn in the ListDomainsResponse - .domainName(domain.name()) .build()) .collect(Collectors.toList()); } + // TODO change domainName to arn when CodeArtifactClient populates arn in the ListDomainsResponse + private static String buildArn( + ResourceHandlerRequest request, String domainOwner, String domainName + ) { + final String partition = request.getAwsPartition(); + final String region = request.getRegion(); + return String.format("arn:%s:codeartifact:%s:%s:domain/%s", partition, region, domainOwner, domainName); + } + private static Stream streamOfOrEmpty(final Collection collection) { return Optional.ofNullable(collection) .map(Collection::stream) diff --git a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/UpdateHandler.java b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/UpdateHandler.java index 626df4b..53570c7 100644 --- a/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/UpdateHandler.java +++ b/aws-codeartifact-domain/src/main/java/software/amazon/codeartifact/domain/UpdateHandler.java @@ -3,11 +3,13 @@ import software.amazon.awssdk.awscore.exception.AwsServiceException; import software.amazon.awssdk.services.codeartifact.CodeartifactClient; import software.amazon.awssdk.services.codeartifact.model.ResourceNotFoundException; +import software.amazon.cloudformation.exceptions.CfnNotUpdatableException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; import software.amazon.cloudformation.proxy.Logger; import software.amazon.cloudformation.proxy.ProgressEvent; import software.amazon.cloudformation.proxy.ProxyClient; import software.amazon.cloudformation.proxy.ResourceHandlerRequest; +import java.util.Objects; public class UpdateHandler extends BaseHandlerStd { private Logger logger; @@ -21,14 +23,34 @@ protected ProgressEvent handleRequest( ) { this.logger = logger; + + String desiredDomainName = request.getDesiredResourceState().getDomainName(); + String previousDomainName = request.getPreviousResourceState().getDomainName(); + if (!Objects.equals(previousDomainName, desiredDomainName)) { + // cannot update domainName because it's CreateOnly + throw new CfnNotUpdatableException(ResourceModel.TYPE_NAME, desiredDomainName); + } + return ProgressEvent.progress(request.getDesiredResourceState(), callbackContext) - // If PolicyDocument is in the model putDomainPermissions policy - .then(progress -> putDomainPermissionsPolicy(proxy, progress, callbackContext, request, proxyClient, logger)) - // Else then deleteDomainPermission policy - .then(progress -> deleteDomainPermissionsPolicy(proxy, progress, callbackContext, request, proxyClient)) + .then(progress -> updateDomainPermissionsPolicy(proxy, progress, callbackContext, request, proxyClient, logger)) .then(progress -> new ReadHandler().handleRequest(proxy, request, callbackContext, proxyClient, logger)); } + protected ProgressEvent updateDomainPermissionsPolicy( + final AmazonWebServicesClientProxy proxy, + final ProgressEvent progress, + final CallbackContext callbackContext, + final ResourceHandlerRequest request, + final ProxyClient proxyClient, + final Logger logger + ) { + final ResourceModel desiredModel = request.getDesiredResourceState(); + if (desiredModel.getPermissionsPolicyDocument() != null) { + return putDomainPermissionsPolicy(proxy, progress, callbackContext, request, proxyClient, logger); + } + return deleteDomainPermissionsPolicy(proxy, progress, callbackContext, request, proxyClient); + } + private ProgressEvent deleteDomainPermissionsPolicy( AmazonWebServicesClientProxy proxy, ProgressEvent progress, @@ -36,9 +58,10 @@ private ProgressEvent deleteDomainPermissionsPol ResourceHandlerRequest request, ProxyClient proxyClient ) { - final ResourceModel desiredModel = progress.getResourceModel(); + final ResourceModel desiredModel = request.getDesiredResourceState(); + final ResourceModel previousModel = request.getPreviousResourceState(); - if (desiredModel.getPolicyDocument() != null) { + if (desiredModel.getPermissionsPolicyDocument() != null || previousModel.getPermissionsPolicyDocument() == null) { return ProgressEvent.progress(desiredModel, callbackContext); } @@ -73,6 +96,4 @@ private boolean domainPolicyIsDeleted( } } - - } diff --git a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/AbstractTestBase.java b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/AbstractTestBase.java index 4526650..7bef819 100644 --- a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/AbstractTestBase.java +++ b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/AbstractTestBase.java @@ -1,6 +1,8 @@ package software.amazon.codeartifact.domain; import java.time.Instant; +import java.util.Collections; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.function.Function; import software.amazon.awssdk.awscore.AwsRequest; @@ -14,6 +16,8 @@ import software.amazon.cloudformation.proxy.LoggerProxy; import software.amazon.cloudformation.proxy.ProxyClient; +import com.fasterxml.jackson.databind.ObjectMapper; + public class AbstractTestBase { protected static final Credentials MOCK_CREDENTIALS; protected static final LoggerProxy logger; @@ -22,13 +26,14 @@ public class AbstractTestBase { protected static final String DOMAIN_ARN = "testDomainArn"; protected static final String ENCRYPTION_KEY_ARN = "testKeyArn"; protected static final String DOMAIN_OWNER = "123456789"; - protected static final String TEST_POLICY_DOC = "{\"Version\":\"2012-10-17\"," - + "\"Statement\":[{\"Sid\":\"ContributorPolicy\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::946525001030:root\"},\"Action\":[\"codeartifact:CreateRepository\",\"codeartifact:DeleteDomain\",\"codeartifact:DeleteDomainPermissionsPolicy\",\"codeartifact:DescribeDomain\",\"codeartifact:GetAuthorizationToken\",\"codeartifact:GetDomainPermissionsPolicy\",\"codeartifact:ListRepositoriesInDomain\",\"codeartifact:PutDomainPermissionsPolicy\",\"sts:GetServiceBearerToken\"],\"Resource\":\"*\"}]}"; + protected static final Map TEST_POLICY_DOC = Collections.singletonMap("key0", "value0"); protected final Instant NOW = Instant.now(); protected final int REPO_COUNT = 2; protected final String STATUS = "Active"; protected final int ASSET_SIZE = 1234; + public static final ObjectMapper MAPPER = new ObjectMapper(); + static { MOCK_CREDENTIALS = new Credentials("accessKey", "secretKey", "token"); logger = new LoggerProxy(); diff --git a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/CreateHandlerTest.java b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/CreateHandlerTest.java index da7d298..adbae0c 100644 --- a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/CreateHandlerTest.java +++ b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/CreateHandlerTest.java @@ -38,7 +38,6 @@ import software.amazon.cloudformation.exceptions.CfnAlreadyExistsException; import software.amazon.cloudformation.exceptions.CfnGeneralServiceException; import software.amazon.cloudformation.exceptions.CfnInvalidRequestException; -import software.amazon.cloudformation.exceptions.CfnResourceConflictException; import software.amazon.cloudformation.exceptions.CfnServiceLimitExceededException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; import software.amazon.cloudformation.proxy.OperationStatus; @@ -46,6 +45,8 @@ import software.amazon.cloudformation.proxy.ProxyClient; import software.amazon.cloudformation.proxy.ResourceHandlerRequest; +import com.fasterxml.jackson.core.JsonProcessingException; + @ExtendWith(MockitoExtension.class) public class CreateHandlerTest extends AbstractTestBase { @@ -98,7 +99,6 @@ public void handleRequest_simpleSuccess() { final ResourceModel model = ResourceModel.builder() .domainName(DOMAIN_NAME) - .domainOwner(DOMAIN_OWNER) .build(); CreateDomainResponse createDomainResponse = CreateDomainResponse.builder() @@ -134,13 +134,12 @@ public void handleRequest_simpleSuccess() { } @Test - public void handleRequest_withDomainPolicy() { + public void handleRequest_withDomainPolicy() throws JsonProcessingException { final CreateHandler handler = new CreateHandler(); final ResourceModel model = ResourceModel.builder() .domainName(DOMAIN_NAME) - .domainOwner(DOMAIN_OWNER) - .policyDocument(TEST_POLICY_DOC) + .permissionsPolicyDocument(TEST_POLICY_DOC) .build(); CreateDomainResponse createDomainResponse = CreateDomainResponse.builder() @@ -182,10 +181,7 @@ public void handleRequest_withDomainPolicy() { PutDomainPermissionsPolicyRequest capturedRequest = putDomainPermissionsPolicyRequestArgumentCaptor.getValue(); assertThat(capturedRequest.domain()).isEqualTo(DOMAIN_NAME); - assertThat(capturedRequest.domainOwner()).isEqualTo(DOMAIN_OWNER); - assertThat(capturedRequest.policyDocument()).isEqualTo(TEST_POLICY_DOC); - - verify(codeartifactClient).getDomainPermissionsPolicy(any(GetDomainPermissionsPolicyRequest.class)); + assertThat(capturedRequest.policyDocument()).isEqualTo(MAPPER.writeValueAsString(TEST_POLICY_DOC)); } diff --git a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/DeleteHandlerTest.java b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/DeleteHandlerTest.java index 5d74f89..698b197 100644 --- a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/DeleteHandlerTest.java +++ b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/DeleteHandlerTest.java @@ -14,6 +14,7 @@ import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -24,12 +25,15 @@ import software.amazon.awssdk.services.codeartifact.model.DeleteDomainRequest; import software.amazon.awssdk.services.codeartifact.model.DeleteDomainResponse; import software.amazon.awssdk.services.codeartifact.model.DescribeDomainRequest; +import software.amazon.awssdk.services.codeartifact.model.DescribeDomainResponse; +import software.amazon.awssdk.services.codeartifact.model.DomainDescription; import software.amazon.awssdk.services.codeartifact.model.InternalServerException; import software.amazon.awssdk.services.codeartifact.model.ResourceNotFoundException; import software.amazon.awssdk.services.codeartifact.model.ServiceQuotaExceededException; import software.amazon.cloudformation.exceptions.CfnAccessDeniedException; import software.amazon.cloudformation.exceptions.CfnAlreadyExistsException; import software.amazon.cloudformation.exceptions.CfnGeneralServiceException; +import software.amazon.cloudformation.exceptions.CfnNotFoundException; import software.amazon.cloudformation.exceptions.CfnResourceConflictException; import software.amazon.cloudformation.exceptions.CfnServiceLimitExceededException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; @@ -50,6 +54,17 @@ public class DeleteHandlerTest extends AbstractTestBase { @Mock CodeartifactClient codeartifactClient; + private final DomainDescription domainDescription = DomainDescription.builder() + .name(DOMAIN_NAME) + .owner(DOMAIN_OWNER) + .arn(DOMAIN_ARN) + .repositoryCount(REPO_COUNT) + .assetSizeBytes((long) ASSET_SIZE) + .status(STATUS) + .createdTime(NOW) + .encryptionKey(ENCRYPTION_KEY_ARN) + .build(); + @BeforeEach public void setup() { proxy = new AmazonWebServicesClientProxy(logger, MOCK_CREDENTIALS, () -> Duration.ofSeconds(600).toMillis()); @@ -77,7 +92,15 @@ public void handleRequest_simpleSuccess() { when(proxyClient.client().deleteDomain(any(DeleteDomainRequest.class))).thenReturn(deleteDomainResponse); - when(proxyClient.client().describeDomain(any(DescribeDomainRequest.class))).thenThrow(ResourceNotFoundException.class); + DescribeDomainResponse describeDomainResponse = DescribeDomainResponse.builder() + .domain(domainDescription) + .build(); + + // first, when checking if domain exists to be deleted + // second, to check if domain has been deleted + when(proxyClient.client().describeDomain(any(DescribeDomainRequest.class))) + .thenReturn(describeDomainResponse) + .thenThrow(ResourceNotFoundException.class); final ResourceHandlerRequest request = ResourceHandlerRequest.builder() .desiredResourceState(model) @@ -89,13 +112,13 @@ public void handleRequest_simpleSuccess() { assertThat(response).isNotNull(); assertThat(response.getStatus()).isEqualTo(OperationStatus.SUCCESS); assertThat(response.getCallbackDelaySeconds()).isEqualTo(0); - assertThat(response.getResourceModel()).isEqualTo(request.getDesiredResourceState()); + assertThat(response.getResourceModel()).isNull(); assertThat(response.getResourceModels()).isNull(); assertThat(response.getMessage()).isNull(); assertThat(response.getErrorCode()).isNull(); verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(2)).describeDomain(any(DescribeDomainRequest.class)); } @@ -118,13 +141,13 @@ public void handleRequest_conflictException() { try { final ProgressEvent response = handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger); fail("Expected Exception"); - } catch (CfnAlreadyExistsException e) { + } catch (CfnResourceConflictException e) { //Expected } verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient, never()).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(1)).describeDomain(any(DescribeDomainRequest.class)); } @@ -152,7 +175,7 @@ public void handleRequest_accessDeniedException() { } verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient, never()).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(1)).describeDomain(any(DescribeDomainRequest.class)); } @Test @@ -179,7 +202,7 @@ public void handleRequest_serviceQuotaExceededException() { } verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient, never()).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(1)).describeDomain(any(DescribeDomainRequest.class)); } @Test @@ -206,7 +229,7 @@ public void handleRequest_validationException() { } verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient, never()).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(1)).describeDomain(any(DescribeDomainRequest.class)); } @Test @@ -233,6 +256,32 @@ public void handleRequest_internalServerException() { } verify(codeartifactClient).deleteDomain(any(DeleteDomainRequest.class)); - verify(codeartifactClient, never()).describeDomain(any(DescribeDomainRequest.class)); + verify(codeartifactClient, times(1)).describeDomain(any(DescribeDomainRequest.class)); + } + + @Test + public void handleRequest_doesNotExist() { + final DeleteHandler handler = new DeleteHandler(); + + final ResourceModel model = ResourceModel.builder() + .domainName(DOMAIN_NAME) + .encryptionKey(ENCRYPTION_KEY_ARN) + .build(); + + when(proxyClient.client().describeDomain(any(DescribeDomainRequest.class))).thenThrow(ResourceNotFoundException.class); + final ResourceHandlerRequest request = ResourceHandlerRequest.builder() + .desiredResourceState(model) + .logicalResourceIdentifier(DOMAIN_ARN) + .build(); + + try { + final ProgressEvent response = handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger); + fail("Expected Exception"); + } catch (CfnNotFoundException e) { + //Expected + + } + + verify(codeartifactClient).describeDomain(any(DescribeDomainRequest.class)); } } diff --git a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/UpdateHandlerTest.java b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/UpdateHandlerTest.java index f8ba9a8..2e7ea6c 100644 --- a/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/UpdateHandlerTest.java +++ b/aws-codeartifact-domain/src/test/java/software/amazon/codeartifact/domain/UpdateHandlerTest.java @@ -13,6 +13,7 @@ import software.amazon.awssdk.services.codeartifact.model.PutDomainPermissionsPolicyResponse; import software.amazon.awssdk.services.codeartifact.model.ResourceNotFoundException; import software.amazon.awssdk.services.codeartifact.model.ResourcePolicy; +import software.amazon.cloudformation.exceptions.CfnNotUpdatableException; import software.amazon.cloudformation.proxy.AmazonWebServicesClientProxy; import software.amazon.cloudformation.proxy.OperationStatus; import software.amazon.cloudformation.proxy.ProgressEvent; @@ -25,7 +26,10 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import com.fasterxml.jackson.core.JsonProcessingException; + import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.atLeastOnce; @@ -77,24 +81,28 @@ public void setup() { @AfterEach public void tear_down() { - verify(codeartifactClient, atLeastOnce()).serviceName(); verifyNoMoreInteractions(codeartifactClient); } @Test - public void handleRequest_simpleSuccess() { + public void handleRequest_simpleSuccess() throws JsonProcessingException { final UpdateHandler handler = new UpdateHandler(); final ResourceModel model = ResourceModel.builder() .domainName(DOMAIN_NAME) .domainOwner(DOMAIN_OWNER) - .policyDocument(TEST_POLICY_DOC) + .permissionsPolicyDocument(TEST_POLICY_DOC) + .build(); + + final ResourceModel previousModel = ResourceModel.builder() + .domainName(DOMAIN_NAME) + .domainOwner(DOMAIN_OWNER) .build(); PutDomainPermissionsPolicyResponse putDomainPermissionsPolicyResponse = PutDomainPermissionsPolicyResponse.builder() .policy( ResourcePolicy.builder() - .document(TEST_POLICY_DOC) + .document(MAPPER.writeValueAsString(TEST_POLICY_DOC)) .build() ) .build(); @@ -109,6 +117,7 @@ public void handleRequest_simpleSuccess() { final ResourceHandlerRequest request = ResourceHandlerRequest.builder() .desiredResourceState(model) + .previousResourceState(previousModel) .build(); final ProgressEvent response = handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger); @@ -122,14 +131,14 @@ public void handleRequest_simpleSuccess() { assertThat(response.getErrorCode()).isNull(); verify(codeartifactClient).describeDomain(any(DescribeDomainRequest.class)); - verify(codeartifactClient).getDomainPermissionsPolicy(any(GetDomainPermissionsPolicyRequest.class)); verify(codeartifactClient).putDomainPermissionsPolicy(any(PutDomainPermissionsPolicyRequest.class)); + verify(codeartifactClient, atLeastOnce()).serviceName(); } @Test - public void handleRequest_deleteDomainPermissionPolicy() { + public void handleRequest_deleteDomainPermissionPolicy() throws JsonProcessingException { final UpdateHandler handler = new UpdateHandler(); final ResourceModel model = ResourceModel.builder() @@ -137,10 +146,16 @@ public void handleRequest_deleteDomainPermissionPolicy() { .domainOwner(DOMAIN_OWNER) .build(); + final ResourceModel previousModel = ResourceModel.builder() + .domainName(DOMAIN_NAME) + .domainOwner(DOMAIN_OWNER) + .permissionsPolicyDocument(TEST_POLICY_DOC) + .build(); + DeleteDomainPermissionsPolicyResponse deleteDomainPermissionsPolicyResponse = DeleteDomainPermissionsPolicyResponse.builder() .policy( ResourcePolicy.builder() - .document(TEST_POLICY_DOC) + .document(MAPPER.writeValueAsString(TEST_POLICY_DOC)) .build() ) .build(); @@ -158,6 +173,7 @@ public void handleRequest_deleteDomainPermissionPolicy() { final ResourceHandlerRequest request = ResourceHandlerRequest.builder() .desiredResourceState(model) + .previousResourceState(previousModel) .build(); final ProgressEvent response = handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger); @@ -173,5 +189,28 @@ public void handleRequest_deleteDomainPermissionPolicy() { verify(codeartifactClient).describeDomain(any(DescribeDomainRequest.class)); verify(codeartifactClient).deleteDomainPermissionsPolicy(any(DeleteDomainPermissionsPolicyRequest.class)); + verify(codeartifactClient, atLeastOnce()).serviceName(); + } + + @Test + public void handleRequest_throwsCfnNotUpdatableException() { + final UpdateHandler handler = new UpdateHandler(); + + final ResourceModel model = ResourceModel.builder() + .domainName(DOMAIN_NAME) + .build(); + + final ResourceModel previousModel = ResourceModel.builder() + .domainName("different-domain-name") + .domainOwner(DOMAIN_OWNER) + .permissionsPolicyDocument(TEST_POLICY_DOC) + .build(); + + final ResourceHandlerRequest request = ResourceHandlerRequest.builder() + .desiredResourceState(model) + .previousResourceState(previousModel) + .build(); + + assertThrows(CfnNotUpdatableException.class,() -> handler.handleRequest(proxy, request, new CallbackContext(), proxyClient, logger)); } }