Skip to content

Commit

Permalink
Fix IAM role creation issue
Browse files Browse the repository at this point in the history
Fixes bug #565.

So there ended up being a couple issue that I fixed here, first being bug #565
which was fixed by making a change to the awsclient.py file.

The second issue that I discovered while working on this bug was the following.
If you do:

```
$ chalice new-project muck
$ cd muck
$ chalice deploy --no-autogen-policy
```

The same bug happens because the policy-env.json file does not exist. This
results in a policy document of:

```
"Version": "2012-10-17",
"Statement": []
```

being uploaded which throws a MalformedPolicyDocument error. In order to fix
this issue I made changes to the deployer.py file. I removed the default policy
and now raise a RuntimeError and alert the user to the fact that they do not
have a policy-env.json file.

The last change that I made was to handle malformed json by adding nicer
logging modeled after the config file loader.
  • Loading branch information
nplutt authored and jamesls committed Dec 14, 2017
1 parent e080003 commit 5f640d5
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 16 deletions.
8 changes: 6 additions & 2 deletions chalice/awsclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,12 @@ def create_role(self, name, trust_policy, policy):
AssumeRolePolicyDocument=json.dumps(trust_policy)
)
role_arn = response['Role']['Arn']
self.put_role_policy(role_name=name, policy_name=name,
policy_document=policy)
try:
self.put_role_policy(role_name=name, policy_name=name,
policy_document=policy)
except client.exceptions.MalformedPolicyDocumentException as e:
self.delete_role(name=name)
raise e
return role_arn

def delete_role(self, name):
Expand Down
15 changes: 11 additions & 4 deletions chalice/deploy/deployer.py
Original file line number Diff line number Diff line change
Expand Up @@ -922,11 +922,18 @@ def load_last_policy(self, config):
# type: (Config) -> Dict[str, Any]
"""Load the last recorded policy file for the app."""
filename = self._app_policy_file(config)
if not self._osutils.file_exists(filename):
if config.autogen_policy and not self._osutils.file_exists(filename):
return self._EMPTY_POLICY
return json.loads(
self._osutils.get_file_contents(filename, binary=False)
)
elif not self._osutils.file_exists(filename):
raise RuntimeError("Unable to load the policy file. Are you sure "
"it exists?")
try:
return json.loads(
self._osutils.get_file_contents(filename, binary=False)
)
except ValueError as err:
raise RuntimeError("Unable to load the project policy file: %s"
% err)

def record_policy(self, config, policy_document):
# type: (Config, Dict[str, Any]) -> None
Expand Down
26 changes: 26 additions & 0 deletions tests/functional/test_awsclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,32 @@ def test_create_role(self, stubbed_session):
assert actual == arn
stubbed_session.verify_stubs()

def test_create_role_raises_error_on_failure(self, stubbed_session):
arn = 'good_arn' * 3
role_id = 'abcd' * 4
today = datetime.datetime.today()
stubbed_session.stub('iam').create_role(
RoleName='role_name',
AssumeRolePolicyDocument=json.dumps({'trust': 'policy'})
).returns({'Role': {
'RoleName': 'No', 'Arn': arn, 'Path': '/',
'RoleId': role_id, 'CreateDate': today}}
)
stubbed_session.stub('iam').put_role_policy(
RoleName='role_name',
PolicyName='role_name',
PolicyDocument={'policy': 'document'}
).raises_error(
error_code='MalformedPolicyDocumentException',
message='MalformedPolicyDocument'
)
stubbed_session.activate_stubs()
awsclient = TypedAWSClient(stubbed_session)
with pytest.raises(botocore.exceptions.ClientError):
awsclient.create_role(
'role_name', {'trust': 'policy'}, {'policy': 'document'})
stubbed_session.verify_stubs()


class TestCreateLambdaFunction(object):
def test_create_function_succeeds_first_try(self, stubbed_session):
Expand Down
8 changes: 6 additions & 2 deletions tests/functional/test_package.py
Original file line number Diff line number Diff line change
Expand Up @@ -732,8 +732,10 @@ def test_can_create_app_packager_with_no_autogen(tmpdir):
appdir = _create_app_structure(tmpdir)

outdir = tmpdir.mkdir('outdir')
default_params = {'autogen_policy': True}
config = Config.create(project_dir=str(appdir),
chalice_app=sample_app())
chalice_app=sample_app(),
**default_params)
p = package.create_app_packager(config)
p.package_app(config, str(outdir))
# We're not concerned with the contents of the files
Expand All @@ -747,8 +749,10 @@ def test_can_create_app_packager_with_no_autogen(tmpdir):
def test_will_create_outdir_if_needed(tmpdir):
appdir = _create_app_structure(tmpdir)
outdir = str(appdir.join('outdir'))
default_params = {'autogen_policy': True}
config = Config.create(project_dir=str(appdir),
chalice_app=sample_app())
chalice_app=sample_app(),
**default_params)
p = package.create_app_packager(config)
p.package_app(config, str(outdir))
contents = os.listdir(str(outdir))
Expand Down
33 changes: 25 additions & 8 deletions tests/unit/deploy/test_deployer.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,12 +263,23 @@ def test_no_policy_generated_when_disabled_in_config(app_policy,
assert generated == json.loads(previous_policy)


def test_load_last_policy_when_file_does_not_exist(app_policy):
loaded = app_policy.load_last_policy(Config.create(project_dir='.'))
assert loaded == {
"Statement": [],
"Version": "2012-10-17",
}
def test_load_last_policy_returns_policy_autogen_true_no_file(app_policy):
expected_policy = {'Version': '2012-10-17', 'Statement': []}
config = Config.create(project_dir='.', autogen_policy=True)
loaded = app_policy.load_last_policy(config)
assert expected_policy == loaded


def test_load_last_policy_raises_error_when_file_does_not_exist(app_policy):
with pytest.raises(RuntimeError):
app_policy.load_last_policy(Config.create(project_dir='.'))


def test_load_policy_raises_error_invalid_json(app_policy, in_memory_osutils):
filename = os.path.join('.', '.chalice', 'policy-dev.json')
in_memory_osutils.filemap[filename] = '{invalid json}'
with pytest.raises(RuntimeError):
app_policy.load_last_policy(Config.create(project_dir='.'))


def test_load_policy_from_disk_when_file_exists(app_policy,
Expand Down Expand Up @@ -1697,15 +1708,21 @@ def test_lambda_deployer_with_tags(self, sample_app):
)

def test_update_lambda_updates_role_once(self, sample_app):
app_policy = mock.Mock(spec=ApplicationPolicyHandler)
app_policy.generate_policy_from_app_source.return_value = {
'Version': '2012-10-17', 'Statement': []
}
app_policy.load_last_policy.return_value = {
'Version': '2012-10-17', 'Statement': []
}
cfg = Config.create(
chalice_stage='dev', app_name='myapp', chalice_app=sample_app,
manage_iam_role=True, iam_role_arn='role-arn',
project_dir='.', tags={'mykey': 'myvalue'}
)
deployer = LambdaDeployer(
self.aws_client, self.packager, self.ui, self.osutils,
self.app_policy)

app_policy)
self.aws_client.get_role_arn_for_name.return_value = 'role-arn'
deployer.deploy(cfg, self.deployed_resources, 'dev')
self.aws_client.update_function.assert_called_with(
Expand Down

0 comments on commit 5f640d5

Please sign in to comment.