-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix: Various TRE bugfixes #642
Changes from 10 commits
2ea263c
b43e3b6
74f8115
0ab258b
6a07a29
49dc873
ce0682d
668cc1f
4c2f27f
3243e1d
5c035f6
969128a
201e2cb
3f0649c
25c0159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -307,9 +307,7 @@ class EnvironmentConfigVarsService extends Service { | |
adminKeyPairName, | ||
}; | ||
|
||
if (enableEgressStore && enableEgressStore.toUpperCase() === 'TRUE') { | ||
result.egressStoreIamPolicyDocument = JSON.stringify(egressStoreIamPolicyDocument); | ||
} | ||
result.egressStoreIamPolicyDocument = JSON.stringify(egressStoreIamPolicyDocument); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our workspace cfn template expect a value for this attribute, and will error out if we don't provide it. If the attribute is |
||
|
||
return result; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,6 @@ | |
"rev", | ||
"inWorkflow", | ||
"status", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With Jeet's change of removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can cidr be completely removed? I wonder why egress store needs it. |
||
"cidr", | ||
"createdAt", | ||
"updatedBy", | ||
"createdBy", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ const updateEnvTypeConfigSchema = createEnvTypeConfigSchema; | |
const settingKeys = { | ||
envTypeConfigsBucketName: 'envTypeConfigsBucketName', | ||
envTypeConfigsPrefix: 'envTypeConfigsPrefix', | ||
isAppStreamEnabled: 'isAppStreamEnabled', | ||
}; | ||
|
||
/** | ||
|
@@ -136,14 +137,14 @@ class EnvTypeConfigService extends Service { | |
|
||
// Make sure the specified configuration has params mapping specified for | ||
// all non-default CFN input params for the given env type | ||
await this.assertNoMissingParams(requestContext, envType, config); | ||
const updatedConfig = await this.checkAndUpdateParams(envType, config); | ||
|
||
// Make sure the config with the same id for the same env type does not exist already | ||
const existingConfigs = await this.getConfigsFromS3(envTypeId); | ||
const existingConfig = _.find(existingConfigs, { id: config.id }); | ||
const existingConfig = _.find(existingConfigs, { id: updatedConfig.id }); | ||
if (existingConfig) { | ||
throw this.boom.badRequest( | ||
`The environment type configuration with id "${config.id}" for environment type "${envTypeId}" already exists`, | ||
`The environment type configuration with id "${updatedConfig.id}" for environment type "${envTypeId}" already exists`, | ||
true, | ||
); | ||
} | ||
|
@@ -152,7 +153,7 @@ class EnvTypeConfigService extends Service { | |
const by = _.get(requestContext, 'principalIdentifier.uid'); | ||
const { bucket, key } = await this.getS3Coordinates(envType.id); | ||
const now = new Date().toISOString(); | ||
const configToSave = this.fromRawToS3Object(config, { | ||
const configToSave = this.fromRawToS3Object(updatedConfig, { | ||
createdBy: by, | ||
updatedBy: by, | ||
createdAt: now, | ||
|
@@ -208,11 +209,9 @@ class EnvTypeConfigService extends Service { | |
} | ||
|
||
// Merge given config with the existing config before updating | ||
const configToUpdate = { ...existingConfig, ...config }; | ||
let configToUpdate = { ...existingConfig, ...config }; | ||
|
||
// Make sure the specified configuration has params mapping specified for | ||
// all non-default CFN input params for the given env type | ||
await this.assertNoMissingParams(requestContext, envType, configToUpdate); | ||
configToUpdate = await this.checkAndUpdateParams(envType, configToUpdate); | ||
|
||
// Everything is good so far, time to save the given configuration to S3 now | ||
const by = _.get(requestContext, 'principalIdentifier.uid'); | ||
|
@@ -241,6 +240,33 @@ class EnvTypeConfigService extends Service { | |
return savedConfig; | ||
} | ||
|
||
async checkAndUpdateParams(envType, config) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting default values for the env params, depending on if AppStream is turned on or not. |
||
const isAppStreamEnabled = this.settings.getBoolean(settingKeys.isAppStreamEnabled); | ||
const updatedConfig = { ...config }; | ||
updatedConfig.params.push({ | ||
key: 'IsAppStreamEnabled', | ||
value: isAppStreamEnabled.toString(), | ||
}); | ||
let params = [...updatedConfig.params]; | ||
if (!isAppStreamEnabled) { | ||
params = [ | ||
...params, | ||
{ key: 'EgressStoreIamPolicyDocument', value: '{}' }, | ||
{ key: 'SolutionNamespace', value: '' }, | ||
]; | ||
} else { | ||
params.push({ | ||
key: 'AccessFromCIDRBlock', | ||
value: '', | ||
}); | ||
} | ||
updatedConfig.params = params; | ||
// Make sure the specified configuration has params mapping specified for | ||
// all non-default CFN input params for the given env type | ||
await this.assertNoMissingParams(envType, updatedConfig); | ||
return updatedConfig; | ||
} | ||
|
||
async delete(requestContext, envTypeId, configId) { | ||
// ensure that the caller has permissions to delete env type config | ||
// Perform default condition checks to make sure the user is active and is admin | ||
|
@@ -338,7 +364,7 @@ class EnvTypeConfigService extends Service { | |
} | ||
} | ||
|
||
async assertNoMissingParams(requestContext, envType, config) { | ||
async assertNoMissingParams(envType, config) { | ||
// Make sure the specified configuration is complete and provides mapping | ||
// for all non-default CFN input parameters | ||
const cfnInputParams = envType.params || []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,8 +34,22 @@ import { createForm } from '@aws-ee/base-ui/dist/helpers/form'; | |
*/ | ||
function getCfnParamsForm(cfnParams, existingParamValues) { | ||
const isAppStreamEnabled = process.env.REACT_APP_IS_APP_STREAM_ENABLED === 'true'; | ||
const filteredCfnParams = cfnParams.filter(cfnParam => { | ||
const { ParameterKey } = cfnParam; | ||
// We don't need to provide `IsAppStreamEnabled` as a config option to user because we can pull this value from settings on the backend | ||
const keysToFilterOut = ['IsAppStreamEnabled']; | ||
if (isAppStreamEnabled) { | ||
keysToFilterOut.push('AccessFromCIDRBlock'); | ||
} else { | ||
keysToFilterOut.push('EgressStoreIamPolicyDocument'); | ||
keysToFilterOut.push('SolutionNamespace'); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't quite get why
Also update unit tests for this method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Let's not make I'll also update the unit test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this can be defined outside the filter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
// Include keys that are not in keysToFilterOut array | ||
return !keysToFilterOut.includes(ParameterKey); | ||
}); | ||
|
||
const fields = {}; | ||
_.forEach(cfnParams, ({ ParameterKey, Description, DefaultValue }) => { | ||
filteredCfnParams.forEach(({ ParameterKey, Description, DefaultValue }) => { | ||
const existingValue = _.get(_.find(existingParamValues, { key: ParameterKey }), 'value') || DefaultValue; | ||
if (!isAppStreamEnabled || (isAppStreamEnabled && !(ParameterKey === 'AccessFromCIDRBlock'))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
fields[ParameterKey] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ Note: Please set up your [AWS Profile](https://docs.aws.amazon.com/cli/latest/us | |
cd ~\Documents | ||
|
||
# Pull the Image Builder script from Github | ||
Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/mainline/scripts/app-stream/buildImage.ps1 -OutFile buildImage.ps1 | ||
Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/feat-secure-workspace-egress/scripts/app-stream/buildImage.ps1 -OutFile buildImage.ps1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sure we add a task to change it back to mainline when releasing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let me add todos for these |
||
|
||
# Execute Image builder script | ||
.\buildImage.ps1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various text enhancements suggested by Jolie