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

Paramfile handler object #3384

Merged
merged 1 commit into from Jun 22, 2018

Conversation

Projects
None yet
6 participants
@stealthycoin
Copy link
Contributor

stealthycoin commented Jun 15, 2018

Add configuration option to opt-out of url parameter fetching

By default the CLI will fetch the content from any string parameter that
starts with https:// or http:// and use the content as the parameter
rather than the URL itself. This adds a configuration option to block
this behavior and these two cases the same as any other string argument.

Specifically the newly added config option is the cli_follow_urlparam
option. It can be used to disable this behavior by adding the following
to the config file in .aws/config.

cli_follow_urlparam = false

If the value is set to true or if the config key is not present then the
default behavior is preserved.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 15, 2018

Codecov Report

Merging #3384 into develop will increase coverage by <.01%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #3384      +/-   ##
==========================================
+ Coverage     94.4%   94.4%   +<.01%     
==========================================
  Files          169     169              
  Lines        13223   13237      +14     
==========================================
+ Hits         12483   12497      +14     
  Misses         740     740
Impacted Files Coverage Δ
awscli/argprocess.py 94.21% <ø> (-0.34%) ⬇️
awscli/testutils.py 58.3% <0%> (ø) ⬆️
awscli/handlers.py 100% <100%> (ø) ⬆️
awscli/customizations/cliinputjson.py 100% <100%> (ø) ⬆️
awscli/paramfile.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f4ac42...18b5c08. Read the comment docs.

@stealthycoin stealthycoin force-pushed the stealthycoin:paramfile-handler-object branch from c45f875 to 401ce40 Jun 15, 2018

@kyleknap
Copy link
Member

kyleknap left a comment

Looks good. Just had some comments



class UriArgumentHandler(object):
def __init__(self, cases=None):

This comment has been minimized.

@kyleknap

kyleknap Jun 18, 2018

Member

Maybe it may make sense to rename cases to something more specific such asprefixes to better indicate that this is a dictionary of prefixes to loaders?

return_value=None) as uri_param_mock:
mock_paramfile_handler = mock.MagicMock()
mock_paramfile_handler.return_value = None
with mock.patch('awscli.paramfile.UriArgumentHandler',

This comment has been minimized.

@kyleknap

kyleknap Jun 18, 2018

Member

I think we should be able to set this up with awscli.paramfile.UriArgumentHandler.__call__ instead. If not, we should at least be using spec for the mock.

from botocore.hooks import HierarchicalEmitter


class FakeSession(object):

This comment has been minimized.

@kyleknap

kyleknap Jun 18, 2018

Member

Can we refactor this so the same FakeSession class is being used for unit tests? I did a quick scan and it looks like there are already two FakeSession implementations in individual test files. I want to avoid making a third.

self.assertIn('https://', prefixes)

def test_uses_supplied_cases(self):
with mock.patch(

This comment has been minimized.

@kyleknap

kyleknap Jun 18, 2018

Member

It would be great if we can minimize the use of patches. Specifically, it would be nice if we provide our own recording handler (or something like that) that we ensure gets called. This concerns are a little less for the default scenarios, but it may make sense to instead to switch some of these to functional tests (i.e. make sure each of the prefix types work).

@kyleknap
Copy link
Member

kyleknap left a comment

Looks better. Thanks for updating to use functional tests. I just had some more suggestions on how to make the functional tests a bit cleaner.

{
"category": "Argument processing",
"type": "enhancement",
"description": "Added cli_follow_urlparam option in the config file which can be set to false to disable following of http"

This comment has been minimized.

@kyleknap

kyleknap Jun 20, 2018

Member

Maybe we should update this to be "to disable following of parameter values that start with http:// or https://"? Right now it is a little unclear of when it get follows.

self.text = 'function-name'


class BaseTestCLIFollowParamURL(BaseCLIWireResponseTest):

This comment has been minimized.

@kyleknap

kyleknap Jun 20, 2018

Member

There are not too many tests that use BaseCLIWireResponseTest. I would recommend using BaseAWSCommandParamsTest instead as that class gives you a lot more to work with. For example you do not have to set a wire response, and instead you can set a parsed response.

self.files.remove_all()
self.stop_patches()

def start_patches(self):

This comment has been minimized.

@kyleknap

kyleknap Jun 20, 2018

Member

I prefer we avoid the pattern in general of starting the patches in the setUp and tearing them down in the tearDown(). Instead, let's either try to avoid patching at all cost for functional tests or use context managers. When they are in the setUp and tearDown it is pretty easy to forget to tear down a patch causing issues down the road when more tests get added.


def start_patches(self):
self.mock_open = mock_open(read_data='function-name')
self.open_patch = patch(

This comment has been minimized.

@kyleknap

kyleknap Jun 20, 2018

Member

I would also prefer we just use the FileCreator for this instead of patching

self.run_cmd(command)


class TestCLIFollowParamURLDefault(BaseTestCLIFollowParamURL):

This comment has been minimized.

@kyleknap

kyleknap Jun 20, 2018

Member

Hmmm. I think for these functional tests it would be better if we minimized the amount of assertions on mock we are making and be more explicit in each test case in what the inputs and outputs are. So specifically, I think we should try to copy the same style we do for the service-specific functional tests:

class TestCLIFollowParamURLDefault(BaseTestCLIFollowParamURL):
    def test_with_http_prefix(self):
        command = "lambda get-function --function-name http://myremote.url"
        self.parsed_response = {}
        with mock.patch('awscli.paramfile.requests.get', spec=True) as mock_get:
            mock_get = FakeResponse(content='my-function-name')
            self.assert_params_for_cmd(command, {'FunctionName': 'my-function-name'}
            mock_get.assert_called_with('http://myremote.url')

Sure it is a little longer than what you currently have, but I find it a lot easier to read and understand. It also asserts that paramfile value were properly pulled into the client parameters. Would like to know your thoughts.

@kyleknap

This comment has been minimized.

Copy link
Member

kyleknap commented Jun 20, 2018

Also it looks like the Travis build is failing.

@jamesls
Copy link
Member

jamesls left a comment

Kyle captured most of my feedback. Looks reasonable to me.

{
"category": "Argument processing",
"type": "enhancement",
"description": "Added cli_follow_urlparam option in the config file which can be set to false to disable the automatic following of string parameters prefixed with http:// or https://"

This comment has been minimized.

@jamesls

jamesls Jun 20, 2018

Member
  1. Reference existing issues that this will close
  2. We talked about creating a tracking issue where people could discuss this feature. Did you create one? If so, link that here as well.
session.register('load-cli-arg', handler)


class UriArgumentHandler(object):

This comment has been minimized.

@jamesls

jamesls Jun 20, 2018

Member

s/Uri/URI/ pep8

@kyleknap
Copy link
Member

kyleknap left a comment

Nice! Looks much better. 🚢

@stealthycoin stealthycoin force-pushed the stealthycoin:paramfile-handler-object branch from 612cfb7 to 1e05079 Jun 21, 2018

@stealthycoin stealthycoin force-pushed the stealthycoin:paramfile-handler-object branch from 1e05079 to 9e1617c Jun 21, 2018

Add configuration option to opt-out of url parameter fetching
By default the CLI will fetch the content from any string parameter that
starts with https:// or http:// and use the content as the parameter
rather than the URL itself. This adds a configuration option to block
this behavior and these two cases the same as any other string argument.

Specifically the newly added config option is the `cli_follow_urlparam`
option. It can be used to disable this behavior by adding the following
to the config file in `.aws/config`.

```
cli_follow_urlparam = false
```

If the value is set to `true` or if the config key is not present then the
default behavior is preserved.

@stealthycoin stealthycoin force-pushed the stealthycoin:paramfile-handler-object branch from 9e1617c to 18b5c08 Jun 22, 2018

@stealthycoin stealthycoin merged commit 354b138 into aws:develop Jun 22, 2018

3 checks passed

codecov/patch 97.29% of diff hit (target 94.4%)
Details
codecov/project 94.4% (+<.01%) compared to 8f4ac42
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stealthycoin stealthycoin deleted the stealthycoin:paramfile-handler-object branch Jun 22, 2018

@michaelize

This comment has been minimized.

Copy link

michaelize commented Jul 2, 2018

By specifying this config, it resolves the issue for #2507, but then breaks the remote file feature described in https://docs.aws.amazon.com/cli/latest/userguide/cli-using-param.html#cli-using-param-file.

Would like to follow up on this issue.

@mrburrito

This comment has been minimized.

Copy link

mrburrito commented Jul 2, 2018

My two cents, since my major issue with this behavior is the SSM put-param in #2507.

The problem here is that you're treating this behavior as one-size-fits-all for the entire CLI when, in the majority of use cases, local JSON parameters especially, you DO want to read from a remote URL and the user is expecting that.

The SSM, in particular, might be the exception case where this should be inverted, though I'm sure there are others since I haven't exercised every command in the CLI.

@stealthycoin

This comment has been minimized.

Copy link
Contributor

stealthycoin commented Jul 2, 2018

@michaelize @mrburrito This is intended to give people a more reasonable way of opting out of this behavior in general, not in particular to SSM. SSM isn't the exception to the rule, there are quite a few https://github.com/aws/aws-cli/blob/develop/awscli/paramfile.py#L31-L119 and currently if we didn't hardcode it in that list you are out of luck, this gives people a way of unblocking themselves if/when we miss one.

On the flip side this doesn't block you from fetching urls even when disabled, since the CLI can be easily composed with other commands

$ aws ssm put-parameter --name foo --value $(curl http://...)

We can open a tracking feature request for more granular paramfile control configuration options: #3423

@michaelize

This comment has been minimized.

Copy link

michaelize commented Jul 3, 2018

@stealthycoin Thanks for the explanation. It seems the current solution is good for now as it provides a consistent behavior for all the commands.

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