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

Adding module to manage AWS Guardduty detectors #40837

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@slapula
Contributor

slapula commented May 29, 2018

SUMMARY

Here's another simple module that adds support for managing AWS Guardduty detectors. My plan for this is to get the detector_id for usage with a module that manages trusted IP sets and threat IP sets (which I also have in the pipeline).

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aws_guardduty_detector

ANSIBLE VERSION
ansible 2.5.4
  config file = None
  configured module search path = [u'/home/ajsmith/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/ajsmith/.local/lib/python2.7/site-packages/ansible
  executable location = /home/ajsmith/.local/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]
@ansibot

This comment has been minimized.

Contributor

ansibot commented May 29, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/guardduty_detector.py:0:0: E307 version_added should be 2.7. Currently 2.6

click here for bot help

@mattclay

This comment has been minimized.

Member

mattclay commented Jun 8, 2018

Permissions issue in CI:

Traceback (most recent call last):
  File "/tmp/ansible_U5D3Iu/ansible_module_guardduty_detector.py", line 89, in create_detector
    Enable=module.params.get('enabled')
  File "/usr/local/lib/python2.7/dist-packages/botocore/client.py", line 314, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/lib/python2.7/dist-packages/botocore/client.py", line 612, in _make_api_call
    raise error_class(parsed_response, operation_name)
ClientError: An error occurred (AccessDeniedException) when calling the CreateDetector operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible=ansible=67941.64 is not authorized to perform: guardduty:CreateDetector on resource: arn:aws:guardduty:us-east-1:966509639900:detector/*

@ansibot ansibot added the stale_ci label Jun 8, 2018

@s-hertel s-hertel self-requested a review Jun 12, 2018

lib/ansible/modules/cloud/amazon/guardduty_detector.py Outdated
if detector_status:
delete_detector(client, module, result)
module.exit_json(changed=result['changed'], results=result)

This comment has been minimized.

@s-hertel

s-hertel Jul 2, 2018

Contributor

My comments about passing around result in #40851 apply to this too. It looks like the detector_id is really the important information, so you can pass that around and return it rather than a result dict containing it. Another note that also applies to #40851 (missed it when reviewing that) is that changed is provided twice in the output here, once with changed, and once in results.changed.

@slapula slapula force-pushed the slapula:aws_guardduty_detector_module branch to ddd09e4 Jul 3, 2018

@slapula

This comment has been minimized.

Contributor

slapula commented Jul 3, 2018

@s-hertel I have updated this PR based on your feedback. I also went ahead and renamed the module to fit the new naming standard.

@ansibot ansibot removed the stale_ci label Jul 3, 2018

@ansibot ansibot added the stale_ci label Jul 11, 2018

@ryansb

I would like some additional output in the return from the present state, but otherwise this looks good and it seems like we'd be able to add support for named/individual detectors pretty easily.

if detector_status['exists']:
result = delete_detector(client, module, detector_status)
module.exit_json(changed=result['changed'], detector_id=result['detector_id'])

This comment has been minimized.

@ryansb

ryansb Jul 18, 2018

Contributor

It would be nice to add for present state to the returns the status from get_detector for the service role ARN and the status that AWS has for the detector.

@s-hertel

Looks good, a couple little things.

type: string
'''
import os

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

This doesn't get used

try:
response = client.list_detectors()
return {'exists': True, 'detector_id': response['DetectorIds'][0]}
except (ClientError, IndexError):

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

When I call list_detectors and there are no detectors it returns an empty list {..., 'DetectorIds': []}, so you can keep the except IndexError: and return False there, but ClientError and BotoCoreError exceptions should be handled with fail_json_aws, rather than masking or not handling those.

aws_connection_info: &aws_connection_info
aws_access_key: "{{ aws_access_key }}"
aws_secret_key: "{{ aws_secret_key }}"
#security_token: "{{ security_token }}"

This comment has been minimized.

@s-hertel

s-hertel Jul 18, 2018

Contributor

This needs to be uncommented to run in CI. You can make it compatible with non-STS credentials by using "{{ security_token | omit() }}" or "{{ security_token | default() }}"

@s-hertel

This comment has been minimized.

Contributor

s-hertel commented Nov 15, 2018

@slapula I've noticed this module is not idempotent. get_detector as Ryan suggested above will be necessary just so the current state can be compared with the desired state. Right now state: present always returns changed is True.

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