fix IS custom op on intrinsic functions (#2673)

Co-authored-by: Kevin DeJong <[email protected]>

Author: kftsehk

src/cfnlint/rules/custom/Operators.py

@@ -127,62 +127,116 @@ def _split_inset_properties(self, property_chain):
 
             return None, []
 
-        def _check_value(self, value, path, property_chain, cfn):
+        def _check_value_defined(self, value, path, property_chain, cfn, **_):
             matches = []
             child_property, new_property_chain = self._split_inset_properties(
                 property_chain
             )
-            if self.is_defined and (
-                value is None or value.get(child_property, None) is None
+            # Specific for !Ref AWS::NoValue, no callback even for pass_if_null=True
+            if len(property_chain) == 1 and value == {
+                child_property: {"Ref": "AWS::NoValue"}
+            }:
+                matches.append(
+                    cfnlint.rules.RuleMatch(
+                        path, error_message or f"{path} must be defined"
+                    )
+                )
+                return matches
+            if value is None or (
+                isinstance(value, dict) and value.get("Ref", None) == "AWS::NoValue"
             ):
                 matches.append(
                     cfnlint.rules.RuleMatch(
                         path, error_message or f"{path} must be defined"
                     )
                 )
+                return matches
+
             if child_property is not None:
                 matches.extend(
                     cfn.check_value(
                         value,
                         child_property,
                         path,
-                        check_value=self._check_value,
+                        check_value=self._check_value_defined,
+                        check_ref=self._check_value_defined,
+                        check_get_att=self._check_value_defined,
+                        check_find_in_map=self._check_value_defined,
+                        check_split=self._check_value_defined,
+                        check_join=self._check_value_defined,
+                        check_import_value=self._check_value_defined,
+                        check_sub=self._check_value_defined,
+                        pass_if_null=True,
                         property_chain=new_property_chain,
                         cfn=cfn,
                     )
                 )
+            return matches
+
+        def _check_value_not_defined(self, value, path, property_chain, cfn, **_):
+            matches = []
+            if value is None:
                 return matches
-            if not self.is_defined and value is not None:
+            if len(property_chain) == 0 and value is not None:
                 matches.append(
                     cfnlint.rules.RuleMatch(
                         path, error_message or f"{path} must not be defined"
                     )
                 )
+                return matches
+
+            child_property, new_property_chain = self._split_inset_properties(
+                property_chain
+            )
+            matches.extend(
+                cfn.check_value(
+                    value,
+                    child_property,
+                    path,
+                    check_value=self._check_value_not_defined,
+                    check_ref=self._check_value_not_defined,
+                    check_get_att=self._check_value_not_defined,
+                    check_find_in_map=self._check_value_not_defined,
+                    check_split=self._check_value_not_defined,
+                    check_join=self._check_value_not_defined,
+                    check_import_value=self._check_value_not_defined,
+                    check_sub=self._check_value_not_defined,
+                    property_chain=new_property_chain,
+                    cfn=cfn,
+                    pass_if_null=True,
+                )
+            )
             return matches
 
         def match_resource_properties(self, properties, _, path, cfn):
             child_property, new_property_chain = self._split_inset_properties(
                 self.property_chain
             )
+            check_fn = (
+                self._check_value_defined
+                if self.is_defined
+                else self._check_value_not_defined
+            )
             matches = []
             # here does nothing when the value is not defined, this is checked separately below
             matches.extend(
                 cfn.check_value(
                     properties,
                     child_property,
                     path,
-                    check_value=self._check_value,
+                    check_value=check_fn,
+                    check_ref=check_fn,
+                    check_get_att=check_fn,
+                    check_find_in_map=check_fn,
+                    check_split=check_fn,
+                    check_join=check_fn,
+                    check_import_value=check_fn,
+                    check_sub=check_fn,
                     property_chain=new_property_chain,
                     cfn=cfn,
+                    pass_if_null=True,
                 )
             )
-            # check child exists separately when checking is_defined
-            if self.is_defined and properties.get(child_property, None) is None:
-                matches.append(
-                    cfnlint.rules.RuleMatch(
-                        path, error_message or f"{path} must be defined"
-                    )
-                )
             return matches
 
     return CustomIsDefinedRule(

test/fixtures/templates/good/custom/is-defined.yaml

@@ -1,29 +1,84 @@
----
 AWSTemplateFormatVersion: "2010-09-09"
 Description: >
   Testing for IS DEFINED rule
+Parameter:
+  NodeEnv:
+    Type: String
 Resources:
   LambdaExecutionRole:
-    Type: AWS::IAM::Role
     Properties:
       AssumeRolePolicyDocument:
-        Version: "2012-10-17"
         Statement:
-          - Effect: Allow
+          - Action:
+              - sts:AssumeRole
+            Effect: Allow
             Principal:
               Service:
                 - lambda.amazonaws.com
                 - ec2.amazonaws.com
-            Action:
-              - sts:AssumeRole
+        Version: "2012-10-17"
       Path: "/"
-  LambdaFunctionTestDefined:
-    Type: AWS::Lambda::Function
+    Type: AWS::IAM::Role
+  LambdaFunctionTestDefinedArray:
     Properties:
+      Code: ./
+      Environment:
+        Variables:
+          NODE_ENV:
+            - 1
+            - 2
       Handler: index.handler
-      Role: !GetAtt LambdaExecutionRole.Arn
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestDefinedEmpty:
+    Properties:
+      Code: ./
       Environment:
         Variables:
           NODE_ENV: ""
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestDefinedGetAttr:
+    Properties:
       Code: ./
+      Environment:
+        Variables:
+          NODE_ENV: !GetAtt "LambdaExecutionRole.Arn"
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
       Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestDefinedObject:
+    Properties:
+      Code: ./
+      Environment:
+        Variables:
+          NODE_ENV:
+            A: 1
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestDefinedRef:
+    Properties:
+      Code: ./
+      Environment:
+        Variables:
+          NODE_ENV: !Ref 'NodeEnv'
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestDefinedValue:
+    Properties:
+      Code: ./
+      Environment:
+        Variables:
+          NODE_ENV: value
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function

test/fixtures/templates/good/custom/is-not-defined.yaml

@@ -1,36 +1,54 @@
----
 AWSTemplateFormatVersion: "2010-09-09"
 Description: >
   Testing for IS NOT_DEFINED rule
 Resources:
   LambdaExecutionRole:
-    Type: AWS::IAM::Role
     Properties:
       AssumeRolePolicyDocument:
-        Version: "2012-10-17"
         Statement:
-          - Effect: Allow
+          - Action:
+              - sts:AssumeRole
+            Effect: Allow
             Principal:
               Service:
                 - lambda.amazonaws.com
                 - ec2.amazonaws.com
-            Action:
-              - sts:AssumeRole
+        Version: "2012-10-17"
       Path: "/"
-  LambdaFunctionTestNotDefined1:
-    Type: AWS::Lambda::Function
+    Type: AWS::IAM::Role
+  LambdaFunctionTestNotDefinedFromParent:
     Properties:
+      Code: ./
+      Environment:
+        Variables: !GetAtt "LambdaExecutionRole.Arn"
       Handler: index.handler
-      Role: !GetAtt LambdaExecutionRole.Arn
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestNotDefinedFromProperties:
+    Properties:
       Code: ./
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
       Runtime: nodejs18.x
-  LambdaFunctionTestNotDefined2:
     Type: AWS::Lambda::Function
+  LambdaFunctionTestNotDefinedRefAWSNoValue:
     Properties:
+      Code: ./
+      Environment:
+        Variables:
+          NODE_ENV: !Ref "AWS::NoValue"
       Handler: index.handler
-      Role: !GetAtt LambdaExecutionRole.Arn
+      Role: !GetAtt "LambdaExecutionRole.Arn"
+      Runtime: nodejs18.x
+    Type: AWS::Lambda::Function
+  LambdaFunctionTestNotDefinedWithSiblings:
+    Properties:
+      Code: ./
       Environment:
         Variables:
           OTHER_VARS: ""
-      Code: ./
+      Handler: index.handler
+      Role: !GetAtt "LambdaExecutionRole.Arn"
       Runtime: nodejs18.x
+    Type: AWS::Lambda::Function

test/unit/rules/custom/test_is_defined.py

@@ -32,5 +32,5 @@ def test_file_positive(self):
     def test_file_negative(self):
         """Test failure"""
         self.helper_file_negative(
-            "test/fixtures/templates/good/custom/is-not-defined.yaml", 2
+            "test/fixtures/templates/good/custom/is-not-defined.yaml", 4
         )

test/unit/rules/custom/test_is_not_defined.py

@@ -32,5 +32,5 @@ def test_file_positive(self):
     def test_file_negative(self):
         """Test failure"""
         self.helper_file_negative(
-            "test/fixtures/templates/good/custom/is-defined.yaml", 1
+            "test/fixtures/templates/good/custom/is-defined.yaml", 6
         )