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.

Sign up for GitHub

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

Jump to bottom

iamcrossaccount - condition validation correction #1868

Merged
merged 7 commits into from Dec 5, 2017
Merged

iamcrossaccount - condition validation correction #1868

merged 7 commits into from Dec 5, 2017

Conversation

joshuaroot
Copy link
Contributor

  • setting set_condition iterator to 'c' from 's' as 's' is defined as a parameter of the function. this was causing some issues validating number of keys in the condition.
2017-12-04 16:26:10,468: custodian.output:ERROR Error while executing policy
Traceback (most recent call last):
  File "/home/ubuntu/cloud-custodian/c7n/policy.py", line 305, in run
    resources = self.policy.resource_manager.resources()
  File "/home/ubuntu/cloud-custodian/c7n/query.py", line 390, in resources
    return self.filter_resources(resources)
  File "/home/ubuntu/cloud-custodian/c7n/manager.py", line 88, in filter_resources
    resources = f.process(resources, event)
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 271, in process
    return super(CrossAccountAccessFilter, self).process(resources, event)
  File "/home/ubuntu/cloud-custodian/c7n/filters/core.py", line 171, in process
    return list(filter(self, resources))
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 304, in __call__
    violations = self.checker.check(p)
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 100, in check
    if self.handle_statement(s):
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 107, in handle_statement
    self.handle_action(s))) and not self.handle_condition(s)):
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 156, in handle_condition
    op, key, value = self.normalize_condition(s)
  File "/home/ubuntu/cloud-custodian/c7n/filters/iamaccess.py", line 193, in normalize_condition
    assert len(s['Condition'][s_cond_op]) == 1, "Multiple keys on condition"
TypeError: string indices must be integers

@joshuaroot
condition validation correction
6ddd2e1 
- setting set_condition iterator to 'c' from 's' as 's' is defined as a parameter of the function. this was causing some issues validating number of keys in the condition.
@joshuaroot joshuaroot changed the title condition validation correction iamcrossaccount - condition validation correction Dec 4, 2017
@kapilt
Copy link
Collaborator

kapilt commented Dec 4, 2017

can you add a test for this as well?

@joshuaroot
adding unittest
a39b227 
- adding unittest for 'ForAllValues:StringEquals'
@joshuaroot
updating unittest
3e01c88 
- adding secondary fail test ForAllValues
jhnlsn
jhnlsn reviewed Dec 4, 2017
View reviewed changes
c7n/filters/iamaccess.py Outdated
for s in set_conditions:
if not s_cond_op.startswith(s_cond_op):
for c in set_conditions:
if not s_cond_op.startswith(c):
Copy link
Contributor

Choose a reason for hiding this comment

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

There is more wrong with this code block, still trying to figure out the correct behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok the code needs to change to say if s_cond_op does not start with at least one of the values in set_conditions, then return. Otherwise if at least one of them matches, keep evaluating.

Copy link
Contributor Author

@joshuaroot joshuaroot Dec 4, 2017 •
edited

Choose a reason for hiding this comment

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

Bah, I also wrote the ForAllValues wrong. Should be ForAllValues:StringEquals, not just ForAllValues...

Edit: Oh, I see where you said that already.

joshuaroot and others added 3 commits December 4, 2017 15:44
@joshuaroot
update iamaccess
097689f 
- changing condition to continue evaluation if any of the set_conditions are at the start of s_cond_op
- correcting string in condition 'ForAllValues:StringEquals'
- adding unittests for 'ForAnyValues:StringEquals'
@joshuaroot
Merge branch 'master' into custodian-iamaccess-conditions
154ff55
@joshuaroot
Merge branch 'master' into custodian-iamaccess-conditions
ab08803
jhnlsn
jhnlsn approved these changes Dec 5, 2017
View reviewed changes
Copy link
Contributor

@jhnlsn jhnlsn left a comment

Choose a reason for hiding this comment

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

lgtm

@jhnlsn jhnlsn merged commit 9667ff4 into cloud-custodian:master Dec 5, 2017
@joshuaroot joshuaroot deleted the custodian-iamaccess-conditions branch December 6, 2017 12:36
lamyanba pushed a commit to lamyanba/cloud-custodian that referenced this pull request Apr 23, 2019
@joshuaroot
iamcrossaccount - condition validation correction (cloud-custodian#1868)
e6350ee 
* condition validation correction

- setting set_condition iterator to 'c' from 's' as 's' is defined as a parameter of the function. this was causing some issues validating number of keys in the condition.

* adding unittest

- adding unittest for 'ForAllValues:StringEquals'

* updating unittest

- adding secondary fail test ForAllValues

* update iamaccess

- changing condition to continue evaluation if any of the set_conditions are at the start of s_cond_op
- correcting string in condition 'ForAllValues:StringEquals'
- adding unittests for 'ForAnyValues:StringEquals'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhnlsn jhnlsn jhnlsn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@joshuaroot @kapilt @jhnlsn