diff --git a/lib/checks.js b/lib/checks.js index afaf3ce..07e3683 100644 --- 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 []; } -} +} \ No newline at end of file diff --git a/lib/index.js b/lib/index.js index fb7a3f2..6d56657 100644 --- 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 index 56ad3cb..06a0bba 100644 --- 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 index 3833d6c..3a714cd 100644 --- 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 new file mode 100755 index 0000000..1ace0cf --- /dev/null +++ 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 index bcc7476..278fa8f 100644 --- 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 index 436f325..69b26be 100644 --- 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 index 49f2ee9..882c2f9 100644 --- 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 index d7a5f19..b960e65 100644 --- 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] ]