Skip to content

Commit

Permalink
Fix Create and Delete bugs (#15)
Browse files Browse the repository at this point in the history
Bugs that got fixed:
* In the initial call from CloudFormation the CallbackContext is
null. That case was not handled
* All operations were lacking permissions for opsworks-cm
* Delete did not get the correct serverName
  • Loading branch information
nopx authored May 15, 2020
1 parent b8b5149 commit a611d90
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 49 deletions.
20 changes: 12 additions & 8 deletions aws-opsworkscm-server/aws-opsworkscm-server.json
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
"/properties/Engine"
],
"primaryIdentifier": [
"/properties/Id"
"/properties/ServerName"
],
"readOnlyProperties": [
"/properties/Id",
Expand All @@ -155,29 +155,33 @@
"handlers": {
"create": {
"permissions": [
"opsworkscm:ListReceiptFilters",
"opsworkscm:CreateReceiptFilter"
"opsworks-cm:CreateServer",
"opsworks-cm:DescribeServers",
"iam:PassRole"
]
},
"delete": {
"permissions": [
"opsworkscm:ListReceiptFilters",
"opsworkscm:DeleteReceiptFilter"
"opsworks-cm:DeleteServer",
"opsworks-cm:DescribeServers"
]
},
"update": {
"permissions": [
"opsworkscm:UpdateReceiptFilters"
"opsworks-cm:UpdateServer",
"opsworks-cm:TagResource",
"opsworks-cm:UntagResource",
"opsworks-cm:DescribeServers"
]
},
"list": {
"permissions": [
"opsworkscm:ListReceiptFilters"
"opsworks-cm:DescribeServers"
]
},
"read": {
"permissions": [
"opsworkscm:ListReceiptFilters"
"opsworks-cm:DescribeServers"
]
}
}
Expand Down
11 changes: 7 additions & 4 deletions aws-opsworkscm-server/resource-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@ Resources:
Statement:
- Effect: Allow
Action:
- "opsworkscm:CreateReceiptFilter"
- "opsworkscm:DeleteReceiptFilter"
- "opsworkscm:ListReceiptFilters"
- "opsworkscm:UpdateReceiptFilters"
- "iam:PassRole"
- "opsworks-cm:CreateServer"
- "opsworks-cm:DeleteServer"
- "opsworks-cm:DescribeServers"
- "opsworks-cm:TagResource"
- "opsworks-cm:UntagResource"
- "opsworks-cm:UpdateServer"
Resource: "*"
Outputs:
ExecutionRoleArn:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected void initialize(final AmazonWebServicesClientProxy proxy,
this.request = request;
this.model = request.getDesiredResourceState();
this.oldModel = request.getPreviousResourceState();
this.callbackContext = callbackContext;
this.callbackContext = callbackContext == null ? new CallbackContext() : callbackContext;
this.log = new LoggerWrapper(logger);

setModelServerName();
Expand All @@ -48,7 +48,9 @@ protected void initialize(final AmazonWebServicesClientProxy proxy,
}

private void setModelServerName() {
if (StringUtils.isNullOrEmpty(model.getServerName())) {
if (oldModel != null && !StringUtils.isNullOrEmpty(oldModel.getServerName())) {
model.setServerName(oldModel.getServerName());
} else if (StringUtils.isNullOrEmpty(model.getServerName())) {
log.log("RequestModel doesn't have the server name. Setting it using request identifier and client token");
model.setServerName(
IdentifierUtils.generateResourceIdentifier(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static software.amazon.opsworkscm.server.ResourceModel.IDENTIFIER_KEY_SERVERNAME;

@AllArgsConstructor
public class ClientWrapper {

Expand All @@ -35,8 +37,8 @@ public class ClientWrapper {
AmazonWebServicesClientProxy proxy;
LoggerWrapper log;

public DescribeServersResponse describeServer() {
return proxy.injectCredentialsAndInvokeV2(buildDescribeServerRequest(), client::describeServers);
public DescribeServersResponse describeServer(String serverName) {
return proxy.injectCredentialsAndInvokeV2(buildDescribeServerRequest(serverName), client::describeServers);
}

public DescribeServersResponse describeAllServers() {
Expand Down Expand Up @@ -71,9 +73,9 @@ public UpdateServerResponse updateServer() {
return proxy.injectCredentialsAndInvokeV2(buildUpdateServerRequest(), client::updateServer);
}

private DescribeServersRequest buildDescribeServerRequest() {
private DescribeServersRequest buildDescribeServerRequest(String serverName) {
return DescribeServersRequest.builder()
.serverName(model.getServerName())
.serverName(serverName)
.build();
}
private DescribeServersRequest buildDescribeAllServersRequest() {
Expand All @@ -82,7 +84,7 @@ private DescribeServersRequest buildDescribeAllServersRequest() {

private DeleteServerRequest buildDeleteServerRequest() {
return DeleteServerRequest.builder()
.serverName(model.getServerName())
.serverName(model.getPrimaryIdentifier().get(IDENTIFIER_KEY_SERVERNAME).toString())
.build();
}

Expand Down Expand Up @@ -172,9 +174,9 @@ private UntagResourceRequest buildUntagResourceRequest() {
}

private String getResourceArn() {
DescribeServersResponse describeServersResponse = describeServer();
DescribeServersResponse describeServersResponse = describeServer(model.getServerName());
if (describeServersResponse != null && describeServersResponse.hasServers()) {
return describeServer().servers().get(0).serverArn();
return describeServer(model.getServerName()).servers().get(0).serverArn();
}
throw ResourceNotFoundException.builder().message(String.format("Server with name %s does not exist.", model.getServerName())).build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
initialize(proxy, request, callbackContext, logger);

try {
if (callbackContext.isStabilizationStarted()) {
if (this.callbackContext.isStabilizationStarted()) {
return handleStabilize();
} else {
return handleExecute();
}
} catch (InvalidStateException e) {
log.error(String.format("Service Side failure during create-server for %s.", model.getServerName()), e);
return ProgressEvent.failed(model, callbackContext, HandlerErrorCode.InternalFailure, "Service Internal Failure");
log.error(String.format("Service Side failure during create-server for %s.", this.model.getServerName()), e);
return ProgressEvent.failed(this.model, this.callbackContext, HandlerErrorCode.InternalFailure, "Service Internal Failure");
} catch (Exception e) {
log.error(String.format("CreateHandler failure during create-server for %s.", model.getServerName()), e);
return ProgressEvent.failed(model, callbackContext, HandlerErrorCode.InternalFailure, "Internal Failure");
log.error(String.format("CreateHandler failure during create-server for %s.", this.model.getServerName()), e);
return ProgressEvent.failed(this.model, this.callbackContext, HandlerErrorCode.InternalFailure, "Internal Failure");
}
}

Expand All @@ -55,7 +55,7 @@ private ProgressEvent<ResourceModel, CallbackContext> handleStabilize() {
callbackContext.incrementRetryTimes();

try {
result = client.describeServer();
result = client.describeServer(model.getServerName());
} catch (final ResourceNotFoundException e) {
return handleServerNotFound(serverName);
}
Expand All @@ -82,6 +82,7 @@ private ProgressEvent<ResourceModel, CallbackContext> handleStabilize() {
case RESTORING:
case UNDER_MAINTENANCE:
case CREATING:
log.info(String.format("Server %s is still creating.", actualServerName));
return ProgressEvent.defaultInProgressHandler(callbackContext, CALLBACK_DELAY_SECONDS, model);
default:
log.info(String.format("Server %s failed to CREATE because of reason: %s", actualServerName, statusReason));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import software.amazon.cloudformation.proxy.ProgressEvent;
import software.amazon.cloudformation.proxy.ResourceHandlerRequest;

import static software.amazon.opsworkscm.server.ResourceModel.IDENTIFIER_KEY_SERVERNAME;

public class DeleteHandler extends BaseOpsWorksCMHandler {

public static final String SERVER_DELETION_FAILED_MESSAGE = "Server %s deletion has failed with reason: %s";
Expand All @@ -28,26 +30,26 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
initialize(proxy, request, callbackContext, logger);

try {
if (callbackContext.isStabilizationStarted()) {
if (this.callbackContext.isStabilizationStarted()) {
return handleStabilize();
} else {
return handleExecute();
}
} catch (InvalidStateException e) {
log.error(String.format("Service Side failure during delete-server for %s.", model.getServerName()), e);
return ProgressEvent.failed(model, callbackContext, HandlerErrorCode.NotStabilized, "Service Internal Failure");
log.error(String.format("Service Side failure during delete-server for %s.", this.model.getServerName()), e);
return ProgressEvent.failed(this.model, this.callbackContext, HandlerErrorCode.NotStabilized, "Service Internal Failure");
} catch (ResourceNotFoundException e) {
return handleServerNotFound(model.getServerName());
return handleServerNotFound(this.model.getServerName());
} catch (ValidationException e) {
log.error(String.format("ValidationException during delete-server of %s.", model.getServerName()), e);
if (e.getMessage().matches(String.format(SERVER_OPERATION_STILL_IN_PROGRESS_MESSAGE, model.getServerName()))) {
log.error(String.format("Server operation still in progress during delete-server of %s.", model.getServerName()));
return ProgressEvent.defaultInProgressHandler(callbackContext, CALLBACK_DELAY_SECONDS, model);
log.error(String.format("ValidationException during delete-server of %s.", this.model.getServerName()), e);
if (e.getMessage().matches(String.format(SERVER_OPERATION_STILL_IN_PROGRESS_MESSAGE, this.model.getServerName()))) {
log.error(String.format("Server operation still in progress during delete-server of %s.", this.model.getServerName()));
return ProgressEvent.defaultInProgressHandler(this.callbackContext, CALLBACK_DELAY_SECONDS, this.model);
}
return ProgressEvent.defaultFailureHandler(e, HandlerErrorCode.InvalidRequest);
} catch (Exception e) {
log.error(String.format("CreateHandler failure during delete-server for %s.", model.getServerName()), e);
return ProgressEvent.failed(model, callbackContext, HandlerErrorCode.InternalFailure, "Internal Failure");
log.error(String.format("CreateHandler failure during delete-server for %s.", this.model.getServerName()), e);
return ProgressEvent.failed(this.model, this.callbackContext, HandlerErrorCode.InternalFailure, "Internal Failure");
}
}

Expand All @@ -59,10 +61,10 @@ private ProgressEvent<ResourceModel, CallbackContext> handleExecute() {

private ProgressEvent<ResourceModel, CallbackContext> handleStabilize() {
final DescribeServersResponse result;
String serverName = model.getServerName();
String serverName = model.getPrimaryIdentifier().get(IDENTIFIER_KEY_SERVERNAME).toString();
callbackContext.incrementRetryTimes();

result = client.describeServer();
result = client.describeServer(serverName);

if (result == null || result.servers() == null) {
log.info("Describe result is Null. Retrying request.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import software.amazon.cloudformation.proxy.ProgressEvent;
import software.amazon.cloudformation.proxy.ResourceHandlerRequest;

import static software.amazon.opsworkscm.server.ResourceModel.IDENTIFIER_KEY_SERVERNAME;

public class ReadHandler extends BaseOpsWorksCMHandler {

@Override
Expand All @@ -20,13 +22,13 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
initialize(proxy, request, callbackContext, logger);

final DescribeServersResponse result;
final String serverName = model.getServerName();
final String serverName = model.getPrimaryIdentifier().get(IDENTIFIER_KEY_SERVERNAME).toString();
callbackContext.incrementRetryTimes();

log.info(String.format("Calling Describe Servers for ServerName %s", serverName));

try {
result = client.describeServer();
result = client.describeServer(serverName);
Server server = result.servers().get(0);
addDescribeServerResponseAttributes(server);
return ProgressEvent.defaultSuccessHandler(model);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,21 @@ public ProgressEvent<ResourceModel, CallbackContext> handleRequest(
initialize(proxy, request, callbackContext, logger);

try {
if (!callbackContext.isUpdateTagComplete()) {
if (!this.callbackContext.isUpdateTagComplete()) {
return updateTags();
}
if (!callbackContext.isUpdateServerComplete()) {
if (!this.callbackContext.isUpdateServerComplete()) {
return updateServer();
}
return ProgressEvent.defaultSuccessHandler(model);
return ProgressEvent.defaultSuccessHandler(this.model);
} catch (ResourceNotFoundException e) {
log.error(String.format("ResourceNotFoundException during update of server %s, with message %s", model.getServerName(), e.getMessage()), e);
log.error(String.format("ResourceNotFoundException during update of server %s, with message %s", this.model.getServerName(), e.getMessage()), e);
return ProgressEvent.defaultFailureHandler(e, HandlerErrorCode.NotFound);
} catch (InvalidStateException e) {
log.error(String.format("InvalidStateException during update of server %s, with message %s", model.getServerName(), e.getMessage()), e);
log.error(String.format("InvalidStateException during update of server %s, with message %s", this.model.getServerName(), e.getMessage()), e);
return ProgressEvent.defaultFailureHandler(e, HandlerErrorCode.NotUpdatable);
} catch (ValidationException e) {
log.error(String.format("ValidationException during update of server %s, with message %s", model.getServerName(), e.getMessage()), e);
log.error(String.format("ValidationException during update of server %s, with message %s", this.model.getServerName(), e.getMessage()), e);
return ProgressEvent.defaultFailureHandler(e, HandlerErrorCode.InvalidRequest);
}
}
Expand Down
4 changes: 2 additions & 2 deletions aws-opsworkscm-server/templates/opsworkscm-server.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ Resources:
Engine: 'ChefAutomate'
EngineVersion: '2'
EngineModel: 'Single'
InstanceProfileArn: "arn:aws:iam::<ACCOUNT_ID>:instance-profile/opsworks-cm-ec2-role"
InstanceProfileArn: "arn:aws:iam::<ACCOUNT_ID>:instance-profile/aws-opsworks-cm-ec2-role"
InstanceType: 'm4.xlarge'
PreferredBackupWindow: '08:00'
PreferredMaintenanceWindow: 'Fri:08:00'
ServiceRoleArn: "arn:aws:iam::<ACCOUNT_ID>:role/opsworks-cm-service-role"
ServiceRoleArn: "arn:aws:iam::<ACCOUNT_ID>:role/service-role/aws-opsworks-cm-service-role"

0 comments on commit a611d90

Please sign in to comment.