Fn::Select #104

kcculhwch · 2018-01-29T13:52:42Z

Hi, I'm just taking a stab at implementing Fn::Select here. I'm not sure if this is all in exactly the format you would like. Please let me know if this works, and what I can do to get the code up to par, if not.

Tests included should cover all major failure points, as well as a passing scenario.

Thanks,
David P. Smith
LinuxAcademy.com

martysweet

Hi David,

Thanks for your contribution to cfn-lint! I have made a few comments below, let me know if you need additional information or you disagree on anything.

I have used https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-select.html for the quotations.

Thanks,
Marty

martysweet · 2018-01-30T09:07:41Z

src/validator.ts

+  }
+
+  let list = toGet[1]
+  if (!Array.isArray(list)) {


It looks like CommaDelimitedList should be supported.

Type: CommaDelimitedList Default: "10.0.48.0/24, 10.0.112.0/24, 10.0.176.0/24"

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-select.html

I'll be sure to get that in there too.

This should be automatically converted using this lookup https://github.com/martysweet/cfn-lint/blob/master/data/aws_parameter_types.json. So should just be a case of using the array.

There may be some validation issues here https://github.com/martysweet/cfn-lint/blob/master/src/validator.ts#L247-L263 which don't catch invalid CommaDelimitedList provided lists. If this turns into a lot of effort we can split improving that into another PR.

Hmm, how would I go about getting that array? I'm not seeing it in workingInput, but maybe I'm looking in the wrong place. workingInput['Parameters'][parameterName]['Attributes']['Ref'] is just returning the original string. I could just do a .split(',') and .trim() each value in the array from that. But I feel like there is a better way to do that. :)

Here's what I added to assignParametersOutput just after it checks allowedValues

if(parameter['Type'] === "CommaDelimitedList" && typeof parameterValue === 'string') { parameterValue = parameterValue.split(',').map(x => x.trim()); parameterValue.forEach(val => { if (val === ""){ addError('crit', `Parameter ${parameterName} contains a CommaDelimitedList where the number of commas appears to be equal or greater than the list of items.`, ['Parameters', parameterName], "Parameters"); } }) }

on my local build, but I'm sure there is a better way somewhere

martysweet · 2018-01-30T09:14:05Z

testData/invalid/json/5_invalid_intrinsic_select_1.json

@@ -0,0 +1,245 @@
+{


Due to the size of these templates, it not particularly clear where the error specified in the test should be occurring. These would be nicer as smaller templates where the test case to fail is easy to spot, and relatively isolated. I.e. no parameters and only features which are required for the test to fail in the specified way.

Yeah, I think I'll try to move my tests into something more a like a unit test. That will make it much more clear whats happening.

These look much better!

martysweet · 2018-01-30T09:16:16Z

src/validator.ts

+  if (index >= 0 && index < list.length) {
+    return list[index];
+  } else {
+    addError('warn', "First element of Fn::Select exceeds the length of the list, if list is a function, make sure it returns a longer list", placeInTemplate, "Fn::Select");


This should probably result in a error instead as it will fail the deployment.

Fn::Select does not check for null values or if the index is out of bounds of the array. Both conditions will result in a stack error, so you should be certain that the index you choose is valid, and that the list contains non-null values.

I was unsure on that one. Since it was possible that a mocked intrinsic could have supplied a shorter list than would be was expected. Fn::Select I think is recommended for choosing AZs, but it looked like the mocked getAZ was only returning three entries. (I'm not sure about that, I only had a minute to glance over it. - and I'm definitely not an expert that front :) )

Yeah, I was wondering about that too. It would involve lots of effort to improve the getAZ mock (i.e. integration with the AWS SDK), as you can't guarantee that all account have the same number of AZs in each region.

Therefore, your account might have a different number of available Availability Zones in a region than another account.

We could, however, have a flag to define the amount of AZs you wish to validate against, as we can always expect this to start from a, AFAIK.

If you can think of any other edge cases where making this an "error" would cause annoyance then we might need to think of another solution.

martysweet · 2018-01-30T09:17:33Z

src/validator.ts

+      addError('crit', "Fn::Select requires the second element to resolve to a list, it appears to be a string", placeInTemplate, "Fn::Select");
+      return 'INVALID_SELECT';
+    }
+    if (!Array.isArray(list)) {


As we are expecting an Array, can we check for the following property of null entries?

This list must not be null, nor can it have null entries.

martysweet · 2018-01-30T09:18:31Z

src/test/validatorTest.ts

+              expect(result).to.have.deep.property('templateValid', false);
+              expect(result['errors']['crit']).to.have.lengthOf(1);
+              expect(result['errors']['warn']).to.have.lengthOf(0);
+            });


It would also be good to ensure the YAML shorthand is working as expected.

AvailabilityZone: !Select - 0 - !GetAZs Ref: 'AWS::Region' AvailabilityZone: !Select - 0 - Fn::GetAZs: !Ref 'AWS::Region'

martysweet · 2018-01-30T09:23:28Z

src/validator.ts

+  if (!Array.isArray(list)) {
+    //we may need to resolve it
+    if (typeof list !== 'string') {
+      list = resolveIntrinsicFunction(list, Object.keys(list)[0]);


There should be some validation around resolving an intrinsic function, this is to support nested functions, see https://github.com/martysweet/cfn-lint/blob/master/src/validator.ts#L894-L897 for an example. This file will need updating with the list of the supported functions from AWS. https://github.com/martysweet/cfn-lint/blob/master/data/aws_intrinsic_functions.json#L13

Supported Functions For the Fn::Select index value, you can use the Ref and Fn::FindInMap functions. For the Fn::Select list of objects, you can use the following functions: Fn::FindInMap Fn::GetAtt Fn::GetAZs Fn::If Fn::Split Ref

Thanks for explaining that bit.

kcculhwch · 2018-01-30T14:25:42Z

Thanks for the feedback! It may be a few days before I can address everything.

Thank you for making such a nice product!

…s, with simplified templates... more to come

Update LA Fork

martysweet

Looking good, is that all the functionality that is required? Just a couple of typos and formatting issues below.

martysweet · 2018-01-31T19:46:10Z

src/validator.ts

@@ -526,7 +526,9 @@ function resolveIntrinsicFunction(ref: any, key: string) : string | boolean | st
        case 'Fn::Not':
            return doIntrinsicNot(ref, key);
        case 'Fn::ImportValue':
-            return doIntrinsicImportValue(ref, key);


Please, can you clean up the spacing issue on here?

martysweet · 2018-01-31T19:48:29Z

src/validator.ts

+    } else if (typeof toGet[0] === 'number'){
+          index = toGet[0];
+    } else {
+      addError('crit', `Fn:Select's first arguement must be a number or resolve to a number, it appears to be a ${typeof(toGet[0])}`, placeInTemplate, "Fn::Select");


arguement typo

martysweet · 2018-01-31T19:49:51Z

src/validator.ts

+            return 'INVALID_SELECT';
+        }
+    } else if (list.indexOf(null) > -1) {
+        addError('crit', "FnSelect requires that the list be free of null values", placeInTemplate, "Fn::Select");


FnSelect typo

martysweet · 2018-01-31T19:51:26Z

src/validator.ts

+    if (index >= 0 && index < list.length) {
+        return list[index];
+    } else {
+        addError('crit', "First element of Fn::Select exceeds the length of the list, if list is a function, make sure it returns a longer list", placeInTemplate, "Fn::Select");


I'm not too keen on the wording of this.

martysweet · 2018-01-31T19:52:01Z

testData/invalid/json/5_invalid_intrinsic_select_1.json

@@ -0,0 +1,245 @@
+{


These look much better!

…e to invalid json, to make sure it is invalid yaml too, as well as checking for null values in the yamlcleanup function

kcculhwch · 2018-02-05T17:25:58Z

src/parser.ts

@@ -63,7 +63,7 @@ function cleanupYaml(ref: any){
            let key = Object.keys(ref)[i];

            // Resolve the function
-            if(ref[key].hasOwnProperty('class') && ref[key].hasOwnProperty('data')){
+            if( ref[key] !== null && ref[key].hasOwnProperty('class') && ref[key].hasOwnProperty('data')){


I found that I needed to make a couple of changes to the parser to block the script from trying to parse out null entries in yaml arrays.

kcculhwch · 2018-02-05T17:26:30Z

src/parser.ts

@@ -96,12 +96,12 @@ function cleanupYaml(ref: any){
                ref[key] = {};
                ref[key][outputKeyName] = outputData;

-            }else if(key != 'Attributes' && typeof ref[key] == "object"){
+            }else if(key != 'Attributes' && typeof ref[key] == "object" && ref[key] !== null){


And here too.

The result of this was that I had to modify the invalid json doc, so that it would also be invalid yaml

kcculhwch · 2018-02-05T17:26:59Z

testData/invalid/json/invalid_json.json

I added the arrow assigment operator and 0 to make it both invalid json and invalid yaml

kcculhwch

This should be most of it, but I want to do one more thorough combing through of the tests, to make sure I have them all generating the right error for the right reason.

…sages

kcculhwch · 2018-02-07T19:45:35Z

Ok, did another once over on the tests, and made them check for specific responses.

…x breaks

martysweet · 2018-02-09T17:20:41Z

Great stuff @kcculhwch, thanks! Are you happy for this to be merged?

kcculhwch · 2018-02-12T13:55:21Z

Yep, I think this is ready. I didn't update the README.md, yet, to indicate support for Fn::Select, as I figured that might need to wait until the release itself gets created.

kcculhwch added 4 commits January 26, 2018 20:25

Fn::Select functionality

886de44

parse the number

0a0abd7

udating with lots of json tests

5b05558

small fix for console.log call

0bafc75

martysweet suggested changes Jan 30, 2018

View reviewed changes

kcculhwch added 3 commits January 30, 2018 15:56

Checking for more possiblities of valid and invalid usages, more test…

93b0798

…s, with simplified templates... more to come

Merge pull request #1 from martysweet/master

0ace222

Update LA Fork

handling commaDelimiedList default data, turning into array

a754adc

martysweet suggested changes Jan 31, 2018

View reviewed changes

kcculhwch added 2 commits February 5, 2018 10:03

updates for yaml tests, minor corrections to crit messages, and chang…

d992705

…e to invalid json, to make sure it is invalid yaml too, as well as checking for null values in the yamlcleanup function

fixing typo and spacing

67b7d98

kcculhwch commented Feb 5, 2018

View reviewed changes

updated test with better comments, and explicit checking of error mes…

a1d8af7

…sages

package.json needs to specify approx 2.6.2, otherwise tyepscript 2.7.…

4c3f420

…x breaks

martysweet approved these changes Feb 9, 2018

View reviewed changes

martysweet added the enhancement label Feb 15, 2018

martysweet merged commit 581807d into martysweet:master Feb 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fn::Select #104

Fn::Select #104

kcculhwch commented Jan 29, 2018

martysweet left a comment

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

kcculhwch Jan 30, 2018 •

edited

Loading

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

martysweet Jan 31, 2018

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

martysweet Jan 30, 2018

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

martysweet Jan 30, 2018

kcculhwch Jan 30, 2018

kcculhwch commented Jan 30, 2018

martysweet left a comment

martysweet Jan 31, 2018

martysweet Jan 31, 2018

martysweet Jan 31, 2018

martysweet Jan 31, 2018

martysweet Jan 31, 2018

kcculhwch Feb 5, 2018

kcculhwch Feb 5, 2018

kcculhwch Feb 5, 2018 •

edited

Loading

kcculhwch left a comment

kcculhwch commented Feb 7, 2018

martysweet commented Feb 9, 2018

kcculhwch commented Feb 12, 2018

Fn::Select #104

Fn::Select #104

Conversation

kcculhwch commented Jan 29, 2018

martysweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcculhwch Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcculhwch commented Jan 30, 2018

martysweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcculhwch Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

kcculhwch left a comment

Choose a reason for hiding this comment

kcculhwch commented Feb 7, 2018

martysweet commented Feb 9, 2018

kcculhwch commented Feb 12, 2018

kcculhwch Jan 30, 2018 •

edited

Loading

kcculhwch Feb 5, 2018 •

edited

Loading