Repository: cloud-custodian/cloud-custodian

Issue:
aws: ecr.actions.set-lifecycle's countUnit says it is required for countType imageCountMoreThan
**Describe the bug**
ecr.actions.set-lifecycle's countUnit says it is required for countType imageCountMoreThan when it should only used when the countType is set to sinceImagePushed.

> The tagPrefixList parameter is only used if tagStatus is tagged. The countUnit parameter is only used if countType is sinceImagePushed. The countNumber parameter is only used if countType is set to imageCountMoreThan. 

Ref: https://docs.aws.amazon.com/AmazonECR/latest/userguide/LifecyclePolicies.html

**To Reproduce**
Steps to reproduce the behavior:

1. Create policy
2. Run policy

**Expected behavior**
Policy should work without having to set countUnit

**Background (please complete the following information):**
 - OS: OSX 10.15
 - Custodian Version: custodian-cask
 - Policy:
```yaml
policies: 
  - name: ecr-no-lifecycle
    resource: aws.ecr
    filters:
      - type: lifecycle-rule
        state: false
    actions:
      - type: set-lifecycle
        rules:
          - rulePriority: 1
            description: "Cleans up older untagged images"
            selection:
              tagStatus: untagged
              countType: imageCountMoreThan
              # c7n requirement. value is ignored because the type is NOT sinceImagePushed
              # countUnit: days
              countNumber: 5
            action:
              type: expire
```
 - Traceback: [if applicable, please exclude sensitive/account information]
```
✗ custodian-cask run -c test-policies/ecr-no-lifecycle.yml --dryrun -s out --verbose --cache-period 0

Custodian Cask 20190904.1 (cloudcustodian/c7n:latest)
Skipped image pull - Last checked 16 minutes ago.

2020-04-14 17:11:07,137: custodian.commands:ERROR invalid policy file: /home/custodian/ecr-no-lifecycle.yml error: Failed to validate policy ecr-no-lifecycle
 Error on policy:ecr-no-lifecycle resource:ecr
'countUnit' is a required property

Failed validating 'required' in schema[12]['properties']['rules']['items']['properties']['selection']:
    {'addtionalProperties': False,
     'properties': {'countNumber': {'type': 'integer'},
                    'countType': {'enum': ['imageCountMoreThan',
                                           'sinceImagePushed']},
                    'countUnit': {'enum': ['hours', 'days']},
                    'tagPrefixList': {'items': {'type': 'string'},
                                      'type': 'array'},
                    'tagStatus': {'enum': ['tagged', 'untagged', 'any']}},
     'required': ['countType', 'countUnit'],
     'type': 'object'}

On instance['rules'][0]['selection']:
    {'countNumber': 5,
     'countType': 'imageCountMoreThan',
     'tagStatus': 'untagged'}
2020-04-14 17:11:07,137: custodian.commands:ERROR Found 1 errors.  Exiting.
```
 - `custodian version --debug` output

**Additional context**
Add any other context about the problem here.


Discussion:
JohnHillegass: Thanks for the add! Something to be aware of there is that it will look for the countUnit if countType is sinceImagePushed. 
https://docs.aws.amazon.com/AmazonECR/latest/userguide/LifecyclePolicies.html
But fortunately there is already a validation in place on line 203. 
myoung34: Right, and that'll error at the PUT request from cloud-custodian which is fine IMO
kapilt: removing validation isn't the same as applying the conditional validation, we want up front validation.
myoung34: @kapilt that validation does exist [here](https://github.com/cloud-custodian/cloud-custodian/blob/master/c7n/resources/ecr.py#L203)
kapilt: > @kapilt that validation does exist [here](https://github.com/cloud-custodian/cloud-custodian/blob/master/c7n/resources/ecr.py#L204)

cool, thanks.. I was going down the rabbit hole of doing this in jsonschema

```diff
+            'anyOf': [
+                {"properties": {
+                    "countType": { "const": "sinceImagePushed" },
+                    "required": ["countUnit"]}},
+                {"properties": {
+                    "countType": { "const": "imageCountMoreThan" },
+                    "required": ["countNumber"]}},
+            ],
```

myoung34: I updated my comment, its called at validation time already so it shouldnt be necessary to add complex schema validation
kapilt: > I updated my comment, its called at validation time already so it shouldnt be necessary to add complex schema validation

the issue is that python validation isn't fully correct to handle all cases (per the note I wrote inline to that function), ie. this is missing countNumber and passes validation, but blows up at runtime.


```
policies: 
  - name: ecr-no-lifecycle
    resource: aws.ecr
    actions:
      - type: set-lifecycle
        rules:
          - rulePriority: 1
            description: "Cleans up older untagged images"
            selection:
              tagStatus: untagged
              countType: imageCountMoreThan
            action:
              type: expire
```


the additional jsonschema lines in my previous comment tried to cover this, or we can add more python code and a unit test to the validate method.


kapilt: going through https://docs.aws.amazon.com/AmazonECR/latest/userguide/LifecyclePolicies.html#lp_tag_status

it looks like tagStatus, countType, and countNumber should be required unconditionally in the schema.

kapilt: i went ahead and added a commit and test to cover off on fixing validation (three required attributes on each rule in jsonschema), and the case of countUnit specified on imageCountMoreThan in the rule validate function.

Patch:
diff --git a/c7n/resources/ecr.py b/c7n/resources/ecr.py
index b2c8d8ae7..057232940 100644
--- a/c7n/resources/ecr.py
+++ b/c7n/resources/ecr.py
@@ -174,7 +174,7 @@ LIFECYCLE_RULE_SCHEMA = {
         'selection': {
             'type': 'object',
             'addtionalProperties': False,
-            'required': ['countType', 'countUnit'],
+            'required': ['countType', 'countNumber', 'tagStatus'],
             'properties': {
                 'tagStatus': {'enum': ['tagged', 'untagged', 'any']},
                 'tagPrefixList': {'type': 'array', 'items': {'type': 'string'}},
@@ -197,7 +197,7 @@ def lifecycle_rule_validate(policy, rule):
     if (rule['selection']['tagStatus'] == 'tagged' and
             'tagPrefixList' not in rule['selection']):
         raise PolicyValidationError(
-            ("{} has invalid lifecycle rule {} tagprefixlist "
+            ("{} has invalid lifecycle rule {} tagPrefixList "
              "required for tagStatus: tagged").format(
                  policy.name, rule))
     if (rule['selection']['countType'] == 'sinceImagePushed' and
@@ -206,6 +206,12 @@ def lifecycle_rule_validate(policy, rule):
             ("{} has invalid lifecycle rule {} countUnit "
              "required for countType: sinceImagePushed").format(
                  policy.name, rule))
+    if (rule['selection']['countType'] == 'imageCountMoreThan' and
+            'countUnit' in rule['selection']):
+        raise PolicyValidationError(
+            ("{} has invalid lifecycle rule {} countUnit "
+             "invalid for countType: imageCountMoreThan").format(
+                 policy.name, rule))
 
 
 @ECR.filter_registry.register('lifecycle-rule')
diff --git a/requirements-dev.txt b/requirements-dev.txt
index 1ac6d0612..569548c6b 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -24,5 +24,5 @@
 # we don't export dev requirements of subpackages (due to editable/distribution above)
 # so explicitly list them here.
 vcrpy-unittest==0.1.7
-parameterized==0.7.1
-fakeredis==1.2.1
+parameterized==0.7.4
+fakeredis==1.4.0


Test patch:
diff --git a/tests/test_ecr.py b/tests/test_ecr.py
index a450e3380..7c535cbf8 100644
--- a/tests/test_ecr.py
+++ b/tests/test_ecr.py
@@ -13,12 +13,29 @@
 # limitations under the License.
 import json
 
-from .common import BaseTest, functional
+from .common import BaseTest, functional, Bag
 from botocore.exceptions import ClientError
 
+from c7n.exceptions import PolicyValidationError
+from c7n.resources.ecr import lifecycle_rule_validate
+
 
 class TestECR(BaseTest):
 
+    def test_rule_validation(self):
+        policy = Bag(name='xyz')
+        with self.assertRaises(PolicyValidationError) as ecm:
+            lifecycle_rule_validate(
+                policy, {'selection': {'tagStatus': 'tagged'}})
+        self.assertIn('tagPrefixList required', str(ecm.exception))
+        with self.assertRaises(PolicyValidationError) as ecm:
+            lifecycle_rule_validate(
+                policy, {'selection': {
+                    'tagStatus': 'untagged',
+                    'countNumber': 10, 'countUnit': 'days',
+                    'countType': 'imageCountMoreThan'}})
+        self.assertIn('countUnit invalid', str(ecm.exception))
+
     def create_repository(self, client, name):
         """ Create the named repository. Delete existing one first if applicable. """
         existing_repos = {