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

Flag support. (Implementation 2) #30

Merged
merged 17 commits into from
Apr 27, 2017
Merged

Flag support. (Implementation 2) #30

merged 17 commits into from
Apr 27, 2017

Conversation

scriptsrc
Copy link
Contributor

@scriptsrc scriptsrc commented Apr 18, 2017

Just get WEBSITE and POLICY:

from cloudaux.orchestration.aws.s3 import get_bucket, FLAGS as FLAGS
bucket = get_bucket("zsh.sh", flags=FLAGS.WEBSITE | FLAGS.POLICY)

Result:

>>> print json.dumps(bucket, indent=2, sort_keys=True)
{
  "Policy": {
    "Statement": [
      {
        "Action": "s3:GetObject", 
        "Effect": "Allow", 
        "Principal": {
          "AWS": "*"
        }, 
        "Resource": "arn:aws:s3:::zsh.sh/*", 
        "Sid": "AddPerm"
      }
    ], 
    "Version": "2008-10-17"
  }, 
  "Website": {
    "ErrorDocument": {
      "Key": "404.html"
    }, 
    "IndexDocument": {
      "Suffix": "index.html"
    }
  }
}

Get the BASE as well:

bucket = get_bucket("zsh.sh", flags=FLAGS.BASE | FLAGS.WEBSITE | FLAGS.POLICY)                                                                                                                                  
print json.dumps(bucket, indent=2, sort_keys=True)

Result:

{
  "Arn": "arn:aws:s3:::zsh.sh", 
  "Policy": {
    "Statement": [
      {
        "Action": "s3:GetObject", 
        "Effect": "Allow", 
        "Principal": {
          "AWS": "*"
        }, 
        "Resource": "arn:aws:s3:::zsh.sh/*", 
        "Sid": "AddPerm"
      }
    ], 
    "Version": "2008-10-17"
  }, 
  "Region": "us-west-1", 
  "Website": {
    "ErrorDocument": {
      "Key": "404.html"
    }, 
    "IndexDocument": {
      "Suffix": "index.html"
    }
  }, 
  "_version": 5
}
from cloudaux.orchestration.aws.iam.role import get_role, FLAGS as ROLEFLAGS
role = get_role({'Arn': 'arn:aws:iam::xxxxxxxxxxxx:role/SecurityMonkey'}, flags=ROLEFLAGS.INSTANCE_PROFILES | ROLEFLAGS.MANAGED_POLICIES)

@scriptsrc
Copy link
Contributor Author

Alternate implementation to #29.

Copy link
Contributor

@mcpeak mcpeak left a comment

Choose a reason for hiding this comment

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

Really nice enhancement! After staring at both for a while I definitely prefer this to the chain of if's.

if flags & entry['flag']:
result.update({entry['key']: entry['method'](bucket_name, **conn)})

if flags & FLAGS.GRANTS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to get this to use the same approach as rest of flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the comment for the rtv_ix. It would be nice to use the same approach. Maybe you can help figure out how to do it.

if type(key) not in [list, tuple]:
key_list = [key]
for idx in xrange(len(flag_list)):
cls.r.append(dict(flag=flag_list[idx], key=key_list[idx], method=fn, rtv_ix=idx))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment as simple as an example line here just as a shortcut to future reviewers?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's rtv_ix used for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point - I initially wrote the FlagRegistry class to try and handle the get_grants() method, which is unique in that it has multiple return values that should get mapped to different fields in the return dictionary.

+# @FlagRegistry.register(
 +#     flag=(FLAGS.GRANTS, FLAGS.GRANT_REFERENCES, FLAGS.OWNER),
 +#     key=('grants', 'grant_references', 'owner'))
 +def get_grants(bucket_name, include_owner=True, **conn):

The rtv_ix was suppose to indicate the index of the return value. However, I kind of abandoned the idea of having FlagRegistry support these more complex interactions. I don't want to call the method three times.

import logging
import json


logger = logging.getLogger('cloudaux')


def get_grants(bucket_name, include_owner=False, **conn):
FLAGS=Bunch(
Copy link
Contributor Author

@scriptsrc scriptsrc Apr 18, 2017

Choose a reason for hiding this comment

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

Is Bunch okay or should we use something else.
Can IDE's autocomplete on bunch?

@scriptsrc
Copy link
Contributor Author

Things to consider:

  • s3 GRANT flags share a code right now. (I'll push a change to fix this.)
  • Should FLAGS.BASE be a thing? Can you use get_* without BASE?
    • What if you just want a user's access keys, but don't want the data returned by boto's get_user...
  • Should I force flags on the other two GCP technologies?
    • If we make a FLAGS.BASE, then yes.
  • Bunch okay or should we use something else.
    • Can IDE's autocomplete on bunch?

@scriptsrc
Copy link
Contributor Author

  • S3 GRANT flags fixed.
  • FLAGS.BASE is now a thing. Yay.
  • I put FLAGS on all the techs.
  • I got rid of Bunch.

@scriptsrc scriptsrc changed the title [WIP] Initial commit of s3 flag support. (Implementation 2) Flag support. (Implementation 2) Apr 21, 2017
@scriptsrc
Copy link
Contributor Author

@supertom - I think this is about ready.

I added flags to all three GCP technologies.

@scriptsrc
Copy link
Contributor Author

FlagRegistry.build_out calls each method regardless of whether the corresponding flag is set.

Fix that before merge.

Run the security_monkey tests and watchers with this version and ensure things work as expected.

@scriptsrc
Copy link
Contributor Author

include_created default should be False, but the flag default is FLAG.ALL.

Need to preserve this behavior.

@scriptsrc
Copy link
Contributor Author

I fixed the build_out() method and the include_created argument/defaults.

Test on SM before merging.

@scriptsrc
Copy link
Contributor Author

Tested on SM. Works great.

Spend some time making sure the GCP services work as expected and then we're good to go.

@scriptsrc scriptsrc merged commit fe0ef9f into master Apr 27, 2017
@scriptsrc scriptsrc deleted the issue_7_flags_2 branch November 25, 2017 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants