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.

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

AWS: Fix KeyError in aws_secret lookup #54792

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@bobdoah
Copy link

commented Apr 3, 2019

The region parameter must be specified as either a keyword arg
or in the lookup plugin options. I opted for the latter, as
run method currently retrieves it from the options.

SUMMARY

I added the region to the options docstring. This stops the lookup plugin from blowing up as it's not specified. It's optional, so does not have to be supplied - but the plugin was raising a KeyError because it was completely undefined. The plugin was already written to read the region value from the options dictionary. The other approach is to supply it as a keyword arg, as in the lib/ansible/plugins/lookup/aws_ssm.py.

Fixes #54790

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/plugins/lookup/aws_secret.py

ADDITIONAL INFORMATION

@Akasurde Akasurde changed the title Fixes #54790 AWS: Fix KeyError in aws_secret lookup Apr 3, 2019

@Akasurde
Copy link
Member

left a comment

Would you be able to add unit test or integration for this ? Thanks.

Show resolved Hide resolved lib/ansible/plugins/lookup/aws_secret.py Outdated
@bobdoah

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Would you be able to add unit test or integration for this ? Thanks.

@Akasurde, I've added a couple of unit tests which should cover this.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@bobdoah This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

The test ansible-test sanity --test pep8 [explain] failed with 15 errors:

test/units/plugins/lookup/test_aws_secret.py:43:1: E101 indentation contains mixed spaces and tabs
test/units/plugins/lookup/test_aws_secret.py:43:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:44:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:45:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:46:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:46:3: E126 continuation line over-indented for hanging indent
test/units/plugins/lookup/test_aws_secret.py:47:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:48:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:49:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:50:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:51:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:52:1: W191 indentation contains tabs
test/units/plugins/lookup/test_aws_secret.py:53:1: E101 indentation contains mixed spaces and tabs
test/units/plugins/lookup/test_aws_secret.py:59:1: E303 too many blank lines (4)
test/units/plugins/lookup/test_aws_secret.py:82:1: E302 expected 2 blank lines, found 0

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@bobdoah this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@ansibot ansibot added the test label Apr 5, 2019

@bobdoah

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

ready_for_review


import pytest
import datetime
import tzlocal

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 8, 2019

Member

Instead of adding a dependency on tzlocal, use UTC for the timezone. Something like this should work:

class UTC(datetime.tzinfo):
    ZERO = datetime.timedelta(0)

    def utcoffset(self, dt):
        return self.ZERO

    def tzname(self, dt):
        return "UTC"

    def dst(self, dt):
        return self.ZERO


utc = UTC()

value = datetime.datetime(2019, 4, 4, 11, 41, 0, 878000, tzinfo=utc)

See the "Example tzinfo classes" portion of the tzinfo documentation for the example on which this is based.

This comment has been minimized.

Copy link
@bobdoah

bobdoah Apr 9, 2019

Author

Is there a reason not to add the dependency? The tzlocal library is just a wrapper around pytz and the python docs say

The standard library has no tzinfo instances, but there exists a third-party library which brings the IANA timezone database (also known as the Olson database) to Python: pytz.

pytz contains up-to-date information and its usage is recommended.

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

After looking at this more closely I realized that boto3 does use local time here instead of UTC. However, boto3 is using dateutil instead of tzlocal. So this will work without any additional requirements:

import dateutil.tz

value = datetime.datetime(2019, 4, 4, 11, 41, 0, 878000, tzinfo=dateutil.tz.tz.tzlocal())

import pytest
import datetime
import tzlocal

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 9, 2019

Member

After looking at this more closely I realized that boto3 does use local time here instead of UTC. However, boto3 is using dateutil instead of tzlocal. So this will work without any additional requirements:

import dateutil.tz

value = datetime.datetime(2019, 4, 4, 11, 41, 0, 878000, tzinfo=dateutil.tz.tz.tzlocal())

@bcoca bcoca removed the needs_triage label Apr 9, 2019

@bobdoah bobdoah force-pushed the bobdoah:devel branch from 08bac2f to 7fc469c Apr 10, 2019

bobdoah and others added some commits Apr 3, 2019

Fixes #54790
The region parameter must be specified as either a keyword arg
or in the lookup plugin options. I opted for the latter, as
`run` method currently retrieves it from the options.
Update lib/ansible/plugins/lookup/aws_secret.py
Co-Authored-By: bobdoah <robertwilliams1985@gmail.com>
Added unit tests as requested by @Akasurde.
The tests consist of a simple success and failure case. I noticed
that the AWS SSM lookup used an include of the aws_region docs to ensure
the config option was set correctly. I've swapped to this, instead of
defining it manually. It seems to work fine with the use case I
specified in #54790.

Whilst I was testing I dumped a pytestdebug.log, which git wanted to
add. I've added it to the .gitignore file to stop anyone else
accidentally committing this.
The tzlocal package isn't installed by default. This is needed to moc…
…k the CreatedDate in the AWS response correctly.

@bobdoah bobdoah force-pushed the bobdoah:devel branch from b75dc8d to d3d9df9 Apr 10, 2019

Dismissing out-of-date review.

@bobdoah

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

@mattclay @Akasurde is there something else I need to do to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.