replicatedhq · svl7 · Apr 5, 2020
diff --git a/lib/checks.js b/lib/checks.js
@@ -160,5 +160,35 @@ var commands = module.exports = {
       return ['invalid_format'];
     }
     return [];
+  },
+  // valid: HEALTHCHECK NONE  -> disabled
+  //        HEALTHCHECK [OPTIONS] CMD
+  // options are: --interval=DURATION       --timeout=DURATION
+  //              --start-period=DURATION   --retries=N
+  is_valid_healtcheck: function(args) {
+    args = args.split(' ')
+    hc_opts_regex = RegExp(/--(interval|timeout|start-period|retries)=\d+/);
+    cmd_position = args.indexOf('cmd');
+    cmd_opts = args.slice(cmd_position + 1);
+
+    if (cmd_position !== -1){
+      // CMD given, but no tool specified
+      if (cmd_opts.length === 0) return ['invalid_format'];
+      // HEALTHCHECK options after CMD are invalid
+      if (hc_opts_regex.test(cmd_opts)) return ['healthcheck_trailing_opts'];
+    }
+    // CMD not first arg -> expecting HEALTHCHECK opts
+    if (cmd_position !== -1 && cmd_position !== 0){
+      healthcheck_opts = args.slice(0, cmd_position);
+      for(const opt of healthcheck_opts){
+        // only options specified by Docker DSL are valid
+        if (!hc_opts_regex.test(opt)) return ['invalid_format'];
+      }
+    }
+    if (args[0] === 'none'){
+      if (args.length != 1) return ['invalid_format'];
+    }
+    else if (cmd_position == -1 && args[0] !== 'none' ) return ['invalid_format'];
+    return [];
   }
-}
+}
diff --git a/lib/index.js b/lib/index.js
@@ -12,18 +12,27 @@ var messages = require('./messages');
 module.exports.run = function(configPath, content) {
   // Parse the file into an array of instructions
   var instructions = {};
+  var emptyLineContinuations = {};
   var lines = content.split(/\r?\n/) || [];
 
   // Supporting multi-line commands, read each line into an array of "instructions", noting the
   // original line number the instruction started on
   var partialLine = '';
   var partialLineNum = 0;
+
   lines.forEach(function(line, lineNum) {
     // Trim whitespace from the line
     line = line.trim();
 
-    // Ignore empty lines and comments
-    if (line === '' || line.startsWith('#')) {
+    // Ignore comments
+    if (line.startsWith('#')) {
+      return;
+    }
+    // Remember empty continuation lines
+    if (line === '') {
+      if (partialLine !== ''){
+        emptyLineContinuations[partialLineNum] = true;
+      }
       return;
     }
 
@@ -59,7 +68,7 @@ module.exports.run = function(configPath, content) {
   }
 
   for (var idx in instructions) {
-    var result = runLine(state, instructions, idx);
+    var result = runLine(state, instructions, idx, emptyLineContinuations);
     state.items = state.items.concat(result.items);
 
     // We care about only having 1 cmd instruction
@@ -103,7 +112,8 @@ function tryLoadFile(filename) {
   }
 }
 
-function runLine(state, instructions, idx) {
+var keyword_occurences = { stopsignal: 0, healthcheck: 0 }
+function runLine(state, instructions, idx, emptyLineContinuations) {
   // return items in an object with the line number as key, value is array of items for this line
   var items = [];
   var line = parseInt(idx) + 1;
@@ -159,6 +169,11 @@ function runLine(state, instructions, idx) {
         var aptgetSubcommands = [];
         var commands = command_parser.split_commands(args);
 
+        // empty line continuation
+        if (emptyLineContinuations[idx]) {
+          items.push(messages.build(state.rules, 'empty_continuation_line', line))
+        }
+
         // parse apt commands
         apt.aptget_commands(commands).forEach(function(aptget_command, index) {
           var subcommand = apt.aptget_subcommand(aptget_command);
@@ -215,6 +230,7 @@ function runLine(state, instructions, idx) {
         });
         break;
       case 'cmd':
+        // TODO empty contination line in CMD as well?
         break;
       case 'label':
         checks.label_format(args).forEach(function(item) {
@@ -264,12 +280,26 @@ function runLine(state, instructions, idx) {
       case 'onbuild':
         break;
       case 'stopsignal':
+        keyword_occurences.stopsignal ++;
+        // don't report again if more than two instances are found
+        if (keyword_occurences.stopsignal == 2) {
+          items.push(messages.build(state.rules, 'multiple_stopsignals', line))
+        }
         break;
       case 'shell':
         checks.is_valid_shell(args).forEach(function(item) {
           items.push(messages.build(state.rules, item, line));
         });
         break;
+      case 'healthcheck':
+        keyword_occurences.healthcheck ++;
+        if (keyword_occurences.healthcheck == 2) {
+          items.push(messages.build(state.rules, 'multiple_healthchecks', line))
+        }
+        checks.is_valid_healtcheck(args).forEach(function(item) {
+          items.push(messages.build(state.rules, item, line));
+        });
+        break;
       default:
          items.push(messages.build(state.rules, 'invalid_command', line));
          break;

diff --git a/lib/reference.js b/lib/reference.js
@@ -168,5 +168,25 @@ container create/start process manage the mapping to host ports using either of
       'title': 'apt-get update with matching cache rm',
       'description': 'Use of apt-get update should be paired with rm -rf /var/lib/apt/lists/* in the same layer.',
       'category': 'Optimization'
+  },
+  'empty_continuation_line': {
+      'title': 'Empty continuation line',
+      'description': 'Empty continuation lines will become errors in a future release.',
+      'category': 'Deprecation'
+  },
+  'healthcheck_trailing_opts': {
+      'title': 'HEALTHCHECK trailing options',
+      'description': 'Options for HEALTHCHECK must come before command.',
+      'category': 'Possible Bug'
+  },
+  'multiple_healthchecks':{
+    'title': 'Multiple HEALTHCHECK declarations',
+    'description': 'Multiple HEALTHCHECK definitions are pointless, only last one is used.',
+    'category': 'Clarity'
+  },
+  'multiple_stopsignals':{
+        'title': 'Multiple STOPSIGNAL declarations',
+        'description': 'Multiple STOPSIGNAL definitions are pointless, only last one is used.',
+        'category': 'Clarity'
   }
 }
diff --git a/test/cli_reporter.js b/test/cli_reporter.js
@@ -99,7 +99,7 @@ describe('cli_reporter', () => {
           title: 'Expose Only Container Port',
           description: 'Using `EXPOSE` to specify a host port is not allowed.',
           category: 'Deprecation',
-          line: 25
+          line: 26
         }
       ];
       let report = new CliReporter()
@@ -124,7 +124,7 @@ describe('cli_reporter', () => {
         '    ' + chalk.cyan('3') + '  ' + chalk.cyan.inverse('Clarity') + '       ' + chalk.cyan('Base Image Latest') + '     ' + chalk.gray('Base images should not use the latest tag.'),
         '                     ' + chalk.cyan('Tag'),
         '',
-        'Line 25: ' + chalk.magenta('EXPOSE 80:80'),
+        'Line 26: ' + chalk.magenta('EXPOSE 80:80'),
         'Issue  Category      Title                 Description',
         '    ' + chalk.red('4') + '  ' + chalk.red.inverse('Deprecation') + '   ' + chalk.red('Expose Only') + '           ' + chalk.gray('Using `EXPOSE` to specify a host port is not allowed.'),
         '                     ' + chalk.red('Container Port'),

diff --git a/test/examples/Dockerfile.healthcheck b/test/examples/Dockerfile.healthcheck
@@ -0,0 +1,12 @@
+FROM ruby:2.7
+# correct
+HEALTHCHECK none
+HEALTHCHECK --interval=1 CMD curl localhost:8080
+# invalid
+HEALTHCHECK CMD --interval=1 curl localhost:8080
+HEALTHCHECK CMD --interval=1
+HEALTHCHECK CMD curl localhost --interval=1
+HEALTHCHECK CMD
+HEALTHCHECK --interval=1
+HEALTHCHECK none CMD curl
+HEALTHCHECK --interval=1 none
diff --git a/test/examples/Dockerfile.misc b/test/examples/Dockerfile.misc
@@ -17,6 +17,7 @@ RUN apt-get update -y
 RUN apt-get update \
 	&& DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
 		curl \
+
 	&& rm -rf /var/lib/apt/lists/*
 RUN apk add python-pip
 RUN apk add --no-cache python-dev build-base wget && apk del python-dev build-base wget
@@ -33,6 +34,7 @@ WORKDIR in valid
 ARG test
 ONBUILD RUN echo test
 STOPSIGNAL SIGKILL
+STOPSIGNAL SIGKILL
 ADD /test* /test2
 CMD ["bash"]
 SHELL ["/bin/sh", "-c"]

diff --git a/test/index.js b/test/index.js
@@ -143,6 +143,44 @@ describe("index", function(){
     });
   });
 
+  describe("#healthcheck", function () {
+    it("validates the HEALTHCHECK syntax issues are detected", function () {
+      var expected = [
+        { title: 'Multiple HEALTHCHECK declarations',
+          line: 4,
+          rule: 'multiple_healthchecks'},
+        { title: 'HEALTHCHECK trailing options',
+          line: 6,
+          rule: 'healthcheck_trailing_opts'},
+        { title: 'HEALTHCHECK trailing options',
+          line: 7,
+          rule: 'healthcheck_trailing_opts'},
+        { title: 'HEALTHCHECK trailing options',
+          line: 8,
+          rule: 'healthcheck_trailing_opts'},
+        { title: 'Invalid Argument Format',
+          line: 9,
+          rule: 'invalid_format'},
+        { title: 'Invalid Argument Format',
+          line: 10,
+          rule: 'invalid_format'},
+        { title: 'Invalid Argument Format',
+          line: 11,
+          rule: 'invalid_format'},
+        { title: 'Invalid Argument Format',
+          line: 12,
+          rule: 'invalid_format'},
+      ];
+      const lintResult = dockerfilelint.run('./test/examples', fs.readFileSync('./test/examples/Dockerfile.healthcheck', 'UTF-8'));
+      _.forEach(lintResult, function(r) {
+        delete r['description'];
+        delete r['category'];
+      });
+      expect(lintResult).to.have.length(expected.length);
+      expect(lintResult).to.deep.equal(expected);
+    });
+  });
+
   describe("#misc", function(){
     it("validates the misc Dockerfile have the exact right issues reported", function(){
       var expected = [
@@ -197,36 +235,42 @@ describe("index", function(){
           { title: 'apt-get update without matching apt-get install',
             rule: 'apt-get-update_require_install',
             line: 16 },
+          { title: 'Empty continuation line',
+            rule: 'empty_continuation_line',
+            line: 17 },
           { title: 'Consider `--no-cache or --update with rm -rf /var/cache/apk/*`',
             rule: 'apkadd-missing_nocache_or_updaterm',
-            line: 21 },
+            line: 22 },
           { title: 'Consider `--virtual` when using apk add and del together in a command.',
             rule: 'apkadd-missing-virtual',
-            line: 22 },
+            line: 23 },
           { title: 'Invalid Port Exposed',
             rule: 'invalid_port',
-            line: 24 },
+            line: 25 },
           { title: 'Expose Only Container Port',
             rule: 'expose_host_port',
-            line: 25 },
+            line: 26 },
           { title: 'Invalid Argument Format',
             rule: 'invalid_format',
-            line: 27 },
+            line: 28 },
           { title: 'Label Is Invalid',
             rule: 'label_invalid',
-            line: 29 },
+            line: 30 },
           { title: 'Extra Arguments',
             rule: 'extra_args',
-            line: 31 },
+            line: 32 },
           { title: 'Invalid WORKDIR',
             rule: 'invalid_workdir',
-            line: 32 },
+            line: 33 },
+          { title: 'Multiple STOPSIGNAL declarations',
+            rule: 'multiple_stopsignals',
+            line: 37 },
           { title: 'Invalid ADD Source',
             rule: 'add_src_invalid',
-            line: 36 },
+            line: 38 },
           { title: 'Invalid ADD Destination',
             rule: 'add_dest_invalid',
-            line: 36 }
+            line: 38 }
         ];
 
       var result = dockerfilelint.run('./test/examples', fs.readFileSync('./test/examples/Dockerfile.misc', 'UTF-8'));

diff --git a/test/json_reporter.js b/test/json_reporter.js
@@ -81,7 +81,7 @@ describe('json_reporter', () => {
           description: 'Base images should not use the latest tag.'
         },
         {
-          line: '25',
+          line: '26',
           content: 'EXPOSE 80:80',
           category: 'Deprecation',
           title: 'Expose Only Container Port',

diff --git a/test/reporter.js b/test/reporter.js
@@ -76,7 +76,7 @@ describe('reporter', () => {
       expect(Object.keys(reporter.fileReports)).to.have.length(1);
       let fileReport = reporter.fileReports[file];
       expect(fileReport.uniqueIssues).to.equal(3);
-      expect(fileReport.contentArray).to.have.length(40);
+      expect(fileReport.contentArray).to.have.length(42);
       expect(fileReport.itemsByLine).to.deep.equal({
         '5': [ items[0] ],
         '6': items.splice(1)
@@ -104,7 +104,7 @@ describe('reporter', () => {
       expect(Object.keys(reporter.fileReports)).to.have.length(1);
       let fileReport = reporter.fileReports[file];
       expect(fileReport.uniqueIssues).to.equal(1);
-      expect(fileReport.contentArray).to.have.length(40);
+      expect(fileReport.contentArray).to.have.length(42);
       expect(fileReport.itemsByLine).to.deep.equal({
         '6': [ items[0] ]
       });
@@ -149,7 +149,7 @@ describe('reporter', () => {
       });
       let file2Report = reporter.fileReports[file2];
       expect(file2Report.uniqueIssues).to.equal(2);
-      expect(file2Report.contentArray).to.have.length(40);
+      expect(file2Report.contentArray).to.have.length(42);
       expect(file2Report.itemsByLine).to.deep.equal({
         '5': [ file2Items[0] ],
         '6': [ file2Items[1] ]