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.

Sign up for GitHub

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

Jump to bottom

Improve IAM role statements, policies and principals internal handling and resolution #8511

Closed
wants to merge 13 commits into from
Closed

Improve IAM role statements, policies and principals internal handling and resolution #8511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
de0a382
started working on iss9396
rzaldana Oct 20, 2020
cab0679
started writing tests
rzaldana Oct 29, 2020
00c061e
wrote more unit tests
rzaldana Oct 30, 2020
e0b72e4
wrote more tests on mergeIamTemplates.test.js
rzaldana Nov 2, 2020
dc86185
started adding tests for log group statements
rzaldana Nov 5, 2020
91a58ee
ran into an issue
rzaldana Nov 8, 2020
97d0f4a
finished step 3
rzaldana Nov 9, 2020
bf8f0eb
started writing writing tests for resolveIamRoles
rzaldana Nov 14, 2020
b7a871c
refactored resolveIamroles tests
rzaldana Nov 15, 2020
43e321a
fixed some errors in mergeIamTemplates
rzaldana Nov 15, 2020
31be8c1
cleaned up code written so far
rzaldana Nov 16, 2020
34b66dd
addressing comments
rzaldana Nov 30, 2020
27302e4
creating functions[].iamConfig in mergeIamTemplates.js
rzaldana Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 82 additions & 0 deletions lib/plugins/aws/package/compile/functions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,88 @@ class AwsCompileFunctions {
const serviceArtifactFileName = this.provider.naming.getServiceArtifactName();
const functionArtifactFileName = this.provider.naming.getFunctionArtifactName(functionName);

// create function's iamConfig object
/*
functionObject.iamConfig = {
principals: new Set(),
policyStatements: [],
managedPolicies: [],
};
Comment on lines +132 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

This ideally should be initialized in mergeIamTemplates.js (so we have this configuration setup upfront, while this plugin is complied as some next item in order afaik)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @medikoo . I tried this before but it was giving me multiple errors (~63), which is why I opted for including this code in functions/index.js instead. I put the code in mergeIamTemplates.js in my latest commit so you can take a look. Maybe there's something I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @medikoo . I tried this before but it was giving me multiple errors (~63)

What errors specifically? Is this about failing tests, if so in that case, please list all test files - we need to refactor them to runServelress variant.

Technically this upgrade should not require any tests to be written or updated, and just work, but for that we need to have many tests refactored to new approach

Copy link
Contributor Author

@rzaldana rzaldana Dec 8, 2020 •
edited

Choose a reason for hiding this comment

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

The errors only occur when the code to create the functions[].iamConfig objects are included in mergeIamTemplates.js instead of functions/index.js. All the errors are from index.test.js and they all say this:

TypeError: Cannot read property \'policyStatements\' of undefined

so I think it means the iamConfig object hasn't been created yet. That's why I opted for including this code in functions/index.js instead since I think mergeIamTemplates.js hasn't yet run by the time functions/index.js is executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

All the errors are from index.test.js

This means that we need to address #8523. After having that, there won't be such error.

Again, let's not get blocked, or have our implementation influenced by failing (for wrong reason) tests. We have very bad tests in many places, and all that fail should be simply refactored (as proposed in above issue)

*/

// add log IAM policies to function's iamConfig object
const logStatementsTemplate = [
{
Effect: 'Allow',
Action: ['logs:CreateLogStream', 'logs:CreateLogGroup'],
Resource: [],
},
{
Effect: 'Allow',
Action: ['logs:PutLogEvents'],
Resource: [],
},
];

const canonicalFunctionNamePrefix = `${
this.serverless.service.service
}-${this.provider.getStage()}`;
const logGroupsPrefix = this.provider.naming.getLogGroupName(canonicalFunctionNamePrefix);

const resolvedFunctionName = functionObject.name;

// if function has canonical name
if (!resolvedFunctionName || resolvedFunctionName.startsWith(canonicalFunctionNamePrefix)) {
logStatementsTemplate[0].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${logGroupsPrefix}*:*`,
});

logStatementsTemplate[1].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${logGroupsPrefix}*:*:*`,
});
} else {
// if function has custom name resolution
const customFunctionNameLogGroupsPrefix = this.provider.naming.getLogGroupName(
resolvedFunctionName
);

logStatementsTemplate[0].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${customFunctionNameLogGroupsPrefix}:*`,
});

logStatementsTemplate[1].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${customFunctionNameLogGroupsPrefix}:*:*`,
});
}

functionObject.iamConfig.policyStatements.push(logStatementsTemplate[0]);
functionObject.iamConfig.policyStatements.push(logStatementsTemplate[1]);

// add managed policies to allow ENI management to iamConfig object if vpc config present

if (functionObject.vpc) {
const vpcManagedPolicy = {
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole',
],
],
};

functionObject.iamConfig.managedPolicies.push(vpcManagedPolicy);
}

let artifactFilePath =
functionObject.package.artifact || this.serverless.service.package.artifact;

Expand Down
112 changes: 112 additions & 0 deletions lib/plugins/aws/package/compile/functions/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2525,6 +2525,118 @@ describe('AwsCompileFunctions #2', () => {
});
});

describe('IamConfig object tests', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Concerning tests, as indicated in main issue description, let's not put any effort in updating them in current shape.

Technically we're just refactoring internals, and result CloudFormation template should remain same (at least it should produce very same effect)

To avoid tests breaking on internal changes, we need to refactor affected tests to runServerless variant.

I've outlined on how to do that for two tests files that you're trying to update here Check: #8523 and #8524

Until those are addressed, we need assume that this PR cannot be merged (but it doesn't stop us from finalizing this refactor - it just cannot be properly validated until tests are refactored).

After we refactor all affected tests, the only test changes that may be wanted in scope of this PR is moving some of the tests to other files, or improving the coverage, that's it, but I would decide that once we have all tests refactored, and functionality in this PR ready

let serverless;
let awsNaming;

before(async () => {
({ serverless, awsNaming } = await runServerless({
fixture: 'function',
configExt: {
functions: {
custom: { name: 'custom-name', handler: 'index.handler' },
canonical: {
handler: 'index.handler',
vpc: { securityGroupIds: ['securityGroupId1'], subnetIds: ['subnetId1'] },
},
},
},
cliArgs: ['package'],
}));
});

it('Should create iamConfig object', () => {
serverless.service.getAllFunctions().forEach(functionName => {
const functionObject = serverless.service.getFunction(functionName);
expect(functionObject.iamConfig).to.exist;
});
});

it('should add log policies to iamConfig object', () => {
const logStatementsTemplate = [
{
Effect: 'Allow',
Action: ['logs:CreateLogStream', 'logs:CreateLogGroup'],
Resource: [],
},
{
Effect: 'Allow',
Action: ['logs:PutLogEvents'],
Resource: [],
},
];

const canonicalFunctionNamePrefix = `${serverless.service.service}-${serverless
.getProvider('aws')
.getStage()}`;
const logGroupsPrefix = awsNaming.getLogGroupName(canonicalFunctionNamePrefix);

serverless.service.getAllFunctions().forEach(functionName => {
const functionObject = serverless.service.getFunction(functionName);
const resolvedFunctionName = functionObject.name;

// if function has canonical name
if (!resolvedFunctionName || resolvedFunctionName.startsWith(canonicalFunctionNamePrefix)) {
logStatementsTemplate[0].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${logGroupsPrefix}*:*`,
});

logStatementsTemplate[1].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${logGroupsPrefix}*:*:*`,
});
} else {
// if function has custom name resolution
const customFunctionNameLogGroupsPrefix = awsNaming.getLogGroupName(resolvedFunctionName);

logStatementsTemplate[0].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${customFunctionNameLogGroupsPrefix}:*`,
});

logStatementsTemplate[1].Resource.push({
'Fn::Sub':
'arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}' +
`:log-group:${customFunctionNameLogGroupsPrefix}:*:*`,
});
}

const statements = functionObject.iamConfig.policyStatements;

expect(statements).to.deep.include(logStatementsTemplate[0]);
expect(statements).to.deep.include(logStatementsTemplate[1]);

logStatementsTemplate[0].Resource = [];
logStatementsTemplate[1].Resource = [];
});
});

it('should add vpc managed policies to iamConfig object when vpc config is present', () => {
const vpcManagedPolicy = {
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole',
],
],
};

serverless.service.getAllFunctions().forEach(functionName => {
const functionObject = serverless.service.getFunction(functionName);

if (!_.isEmpty(functionObject.vpc)) {
expect(functionObject.iamConfig.managedPolicies).to.deep.include(vpcManagedPolicy);
}
});
});
});

describe('when using fileSystemConfig', () => {
const arn =
'arn:aws:elasticfilesystem:us-east-1:111111111111:access-point/fsap-a1a1a1a1a1a1a1a1a';
Expand Down
11 changes: 7 additions & 4 deletions lib/plugins/aws/package/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const generateCoreTemplate = require('./lib/generateCoreTemplate');
const saveServiceState = require('./lib/saveServiceState');
const saveCompiledTemplate = require('./lib/saveCompiledTemplate');
const mergeIamTemplates = require('./lib/mergeIamTemplates');
const resolveIamRoles = require('./lib/resolveIamRoles');

class AwsPackage {
constructor(serverless, options) {
Expand All @@ -27,7 +28,8 @@ class AwsPackage {
generateArtifactDirectoryName,
mergeCustomProviderResources,
saveServiceState,
saveCompiledTemplate
saveCompiledTemplate,
resolveIamRoles
);

// Define inner lifecycles
Expand Down Expand Up @@ -66,9 +68,10 @@ class AwsPackage {
BbPromise.bind(this).then(this.generateArtifactDirectoryName),

'package:finalize': () =>
BbPromise.bind(this).then(() =>
this.serverless.pluginManager.spawn('aws:package:finalize')
),
BbPromise.bind(this).then(() => {
// this.resolveIamRoles();
this.serverless.pluginManager.spawn('aws:package:finalize');
}),

/**
* Inner lifecycle hooks
Expand Down
84 changes: 72 additions & 12 deletions lib/plugins/aws/package/lib/mergeIamTemplates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,37 @@ module.exports = {
}

iamRoleLambdaExecutionTemplate.Properties.Policies[0].PolicyName = this.provider.naming.getPolicyName();
// NOTE. blocks enclosed by:
// **** start
// **** end
// will be taken out

// adding iamConfig object to provider
this.serverless.service.provider.iamConfig = {
principals: new Set(),
policyStatements: [],
managedPolicies: [],
};

// adding iamConfig object to functions
this.serverless.service.getAllFunctions().forEach(functionName => {
const functionObject = this.serverless.service.getFunction(functionName);

functionObject.iamConfig = {
principals: new Set(),
policyStatements: [],
managedPolicies: [],
};
});

const iamConfig = this.serverless.service.provider.iamConfig;

_.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources, {
[this.provider.naming.getRoleLogicalId()]: iamRoleLambdaExecutionTemplate,
});

// **** start

const canonicalFunctionNamePrefix = `${
this.provider.serverless.service.service
}-${this.provider.getStage()}`;
Expand Down Expand Up @@ -128,23 +154,39 @@ module.exports = {
});
}

// **** end

if (this.serverless.service.provider.iamRoleStatements) {
// add custom iam role statements
// **** start
this.serverless.service.provider.compiledCloudFormationTemplate.Resources[
this.provider.naming.getRoleLogicalId()
].Properties.Policies[0].PolicyDocument.Statement = policyDocumentStatements.concat(
this.serverless.service.provider.iamRoleStatements
);
// **** end

this.serverless.service.provider.iamRoleStatements.forEach(statement => {
iamConfig.policyStatements.push(statement);
});
}

if (this.serverless.service.provider.iamManagedPolicies) {
// add iam managed policies
const iamManagedPolicies = this.serverless.service.provider.iamManagedPolicies;
if (iamManagedPolicies.length > 0) {
// **** start
this.mergeManagedPolicies(iamManagedPolicies);
// **** end

iamManagedPolicies.forEach(policy => {
iamConfig.managedPolicies.push(policy);
});
}
}

// **** start

// check if one of the functions contains vpc configuration
const vpcConfigProvided = [];
this.serverless.service.getAllFunctions().forEach(functionName => {
Expand All @@ -154,25 +196,41 @@ module.exports = {
}
});

if (vpcConfigProvided.includes(true) || this.serverless.service.provider.vpc) {
// **** end

if (
// **** start
vpcConfigProvided.includes(true) ||
// **** end
this.serverless.service.provider.vpc
) {
// add managed iam policy to allow ENI management
this.mergeManagedPolicies([
{
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole',
],

const vpcManagedPolicy = {
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole',
],
},
]);
],
};

// **** start

this.mergeManagedPolicies(vpcManagedPolicy);

// **** end

iamConfig.managedPolicies.push(vpcManagedPolicy);
}

return BbPromise.resolve();
},

// **** start

mergeManagedPolicies(managedPolicies) {
const resource = this.serverless.service.provider.compiledCloudFormationTemplate.Resources[
this.provider.naming.getRoleLogicalId()
Expand All @@ -182,4 +240,6 @@ module.exports = {
}
resource.ManagedPolicyArns = resource.ManagedPolicyArns.concat(managedPolicies);
},

// **** end
};