Skip to content

Conversation

@brtrvn
Copy link
Contributor

@brtrvn brtrvn commented Jul 26, 2022

  • Changes to the tenant-onboarding tempates to create FSx ONTAP
    resources
  • Adding the private subnet route table to the tenant resources so we
    can pass it to the FSx file system resource
  • Have the installer save the AD password to Secrets Manager so we can
    resolve it in CloudFormation for the storage virtual machine resource
  • Updates to security groups and IAM policies
  • Update the DNS hostname custom resource to work for both FSx Windows
    and FSx ONTAP
  • Update the user data boot script for Windows to properly mount the
    storage virtual machine

Co-authored-by: netapp-vedantsethia Vedant.Sethia@netapp.com
Co-authored-by: netapp-dhruv-tyagi Dhruv.Tyagi@netapp.com


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

- Changes to the tenant-onboarding tempates to create FSx ONTAP
  resources
- Adding the private subnet route table to the tenant resources so we
  can pass it to the FSx file system resource
- Have the installer save the AD password to Secrets Manager so we can
  resolve it in CloudFormation for the storage virtual machine resource
- Updates to security groups and IAM policies
- Update the DNS hostname custom resource to work for both FSx Windows
  and FSx ONTAP
- Update the user data boot script for Windows to properly mount the
  storage virtual machine

Co-authored-by: netapp-vedantsethia <Vedant.Sethia@netapp.com>
Co-authored-by: netapp-dhruv-tyagi <Dhruv.Tyagi@netapp.com>
Copy link
Contributor

@PoeppingT PoeppingT left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a couple small changes required

- fsx:TagResource
- fsx:UntagResource
Resource:
- !Sub arn:aws:fsx:${AWS::Region}:${AWS::AccountId}:file-system/*
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be AWS::Partition to support aws-cn

Condition: ProvisionFSx
Properties:
TemplateURL: !Sub 'https://{{resolve:ssm:/saas-boost/${Environment}/SAAS_BOOST_BUCKET}}.s3.amazonaws.com/tenant-onboarding-fsx.yaml'
TemplateURL: !Sub 'https://{{resolve:ssm:/saas-boost/${Environment}/SAAS_BOOST_BUCKET}}.s3.${AWS::Region}.amazonaws.com/tenant-onboarding-fsx.yaml'
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly this should be AWS::URLSuffix to support aws-cn

Comment on lines 1007 to 1027
StringBuilder cli = new StringBuilder();
cli.append("aws cloudformation create-stack --stack-name ");
cli.append(stackName);
cli.append(" --capabilities CAPABILITY_NAMED_IAM CAPABILITY_AUTO_EXPAND --template-url ");
cli.append(templateUrl);
cli.append(" --notification-arns ");
cli.append(ONBOARDING_APP_STACK_SNS);
cli.append(" --parameters ");
for (Parameter parameter : templateParameters) {
cli.append("ParameterKey=");
cli.append(parameter.parameterKey());
cli.append(",ParameterValue=\"");
cli.append(parameter.parameterValue());
cli.append("\" ");
}
LOGGER.info(cli.toString());
String stackId;
try {
CreateStackResponse cfnResponse = cfn.createStack(CreateStackRequest.builder()
.stackName(stackName)
.disableRollback(true) // For ease in debugging of failed stacks. Maybe not appropriate for "production".
.disableRollback(true) //TODO undo this
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this debugging data

OntapConfiguration:
JunctionPath: /vol1
SecurityStyle: NTFS
SizeInMegabytes: '40' # Quoted to make cfn-lint happy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make this a Parameter with a default of '40'?

Copy link
Contributor

@PoeppingT PoeppingT left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants