diff --git a/plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy b/plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy index 636e28c0ef..fadc9637fd 100644 --- a/plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy +++ b/plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy @@ -632,7 +632,7 @@ class AwsBatchTaskHandler extends TaskHandler implements BatchHandler validateAwsBatchLabels(Map labels) { + if (!labels) return labels + + final strictMode = executor.session.config.navigate('nextflow.enable.strict', false) + final violations = [] + final result = new HashMap() + + for (Map.Entry entry : labels.entrySet()) { + final key = entry.getKey() + final value = entry.getValue() + + // Check for null keys or values and filter them out (not validation violations) + if (key == null) { + log.warn "AWS Batch label dropped due to null key: key=null, value=${value}" + continue + } + if (value == null) { + log.warn "AWS Batch label dropped due to null value: key=${key}, value=null" + continue + } + + final keyStr = key.toString() + final valueStr = value.toString() + + // Validate key length + if (keyStr.length() > 128) { + violations << "Label key exceeds 128 characters: '${keyStr}' (${keyStr.length()} chars)" + } + + // Validate value length + if (valueStr.length() > 256) { + violations << "Label value exceeds 256 characters: '${keyStr}' = '${valueStr}' (${valueStr.length()} chars)" + } + + // Validate key characters + if (!isValidAwsBatchTagString(keyStr)) { + violations << "Label key contains invalid characters: '${keyStr}' - only letters, numbers, spaces, and _ . : / = + - @ are allowed" + } + + // Validate value characters + if (!isValidAwsBatchTagString(valueStr)) { + violations << "Label value contains invalid characters: '${keyStr}' = '${valueStr}' - only letters, numbers, spaces, and _ . : / = + - @ are allowed" + } + + // Add valid entries to result + result[keyStr] = valueStr + } + + // Handle violations based on strict mode (but only for constraint violations, not null filtering) + if (violations) { + final message = "AWS Batch tag validation failed:\n${violations.collect{ ' - ' + it }.join('\n')}" + if (strictMode) { + throw new ProcessUnrecoverableException(message) + } else { + log.warn "${message}\nTags will be used as-is but may cause AWS Batch submission failures" + } + } + + return result + } + + /** + * Check if a string contains only characters allowed in AWS Batch tags. + * AWS Batch allows: letters, numbers, spaces, and: _ . : / = + - @ + * + * @param input The string to validate + * @return true if the string contains only valid characters + */ + protected boolean isValidAwsBatchTagString(String input, int maxLength = 128) { + if (!input) return false + if (input.length() > maxLength) return false + return input ==~ /^[a-zA-Z0-9\s_.\:\/=+\-@]*$/ + } + /** * @return The list of environment variables to be defined in the Batch job execution context */ diff --git a/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy b/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy index 3e950aa109..961393df37 100644 --- a/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy +++ b/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy @@ -58,6 +58,7 @@ import software.amazon.awssdk.services.batch.model.ResourceType import software.amazon.awssdk.services.batch.model.RetryStrategy import software.amazon.awssdk.services.batch.model.SubmitJobRequest import software.amazon.awssdk.services.batch.model.SubmitJobResponse +import spock.lang.See import spock.lang.Specification import spock.lang.Unroll /** @@ -522,10 +523,9 @@ class AwsBatchTaskHandlerTest extends Specification { vol2: '/here:/there:ro', vol3: '/this:/that:rw', ] - and: - handler.addVolumeMountsToContainer(mounts, containerModel) - + when: + handler.addVolumeMountsToContainer(mounts, containerModel) def container = containerModel.toBatchContainerProperties() then: container.volumes().size() == 4 @@ -578,7 +578,6 @@ class AwsBatchTaskHandlerTest extends Specification { result.containerProperties.logConfiguration == null result.containerProperties.mountPoints == null result.containerProperties.privileged == false - when: result = handler.makeJobDefRequest(task) then: @@ -907,7 +906,7 @@ class AwsBatchTaskHandlerTest extends Specification { then: 1 * handler.isCompleted() >> false 1 * handler.getMachineInfo() >> new CloudMachineInfo('x1.large', 'us-east-1b', PriceModel.spot) - + and: trace.native_id == 'xyz-123' trace.executorName == 'awsbatch' @@ -982,7 +981,14 @@ class AwsBatchTaskHandlerTest extends Specification { task.getName() >> 'batch-task' task.getConfig() >> new TaskConfig(memory: '8GB', cpus: 4, maxRetries: 2, errorStrategy: 'retry', resourceLabels:[a:'b']) - def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + def handler = Spy(AwsBatchTaskHandler) { getExecutor() >> executor } + handler.@executor = executor when: def req = handler.newSubmitRequest(task) @@ -1078,7 +1084,7 @@ class AwsBatchTaskHandlerTest extends Specification { expect: handler.normaliseJobId(JOB_ID) == EXPECTED - + where: JOB_ID | EXPECTED null | null @@ -1097,7 +1103,7 @@ class AwsBatchTaskHandlerTest extends Specification { task.getName() >> NAME and: result == EXPECTED - + where: ENV | NAME | EXPECTED [:] | 'foo' | 'foo' @@ -1134,8 +1140,323 @@ class AwsBatchTaskHandlerTest extends Specification { 2 | true | false | 2 and: null | true | true | 5 // <-- default to 5 - 0 | true | true | 5 // <-- default to 5 + 0 | true | true | 5 // <-- default to 5 1 | true | true | 1 2 | true | true | 2 } + + @Unroll + def 'should validate AWS Batch tag string constraints' () { + given: + def handler = Spy(AwsBatchTaskHandler) + + expect: + handler.isValidAwsBatchTagString(INPUT, MAX_LENGTH) == EXPECTED + + where: + INPUT | MAX_LENGTH | EXPECTED + // Valid strings that meet AWS constraints + 'validLabel' | 128 | true + 'valid-label_123' | 128 | true + 'valid.label:test/path=value+more' | 128 | true + 'label with spaces' | 128 | true + 'label-with@symbol' | 128 | true + and: + // Invalid characters for AWS Batch + 'label#with#hash' | 128 | false + 'label$with%special&chars' | 128 | false + 'label(with)brackets[and]braces{}' | 128 | false + 'label*with?wildcards' | 128 | false + 'unicode_λαβελ_test' | 128 | false + and: + // Length constraints (create a string longer than 128 chars) + ('very-long-label-that-exceeds-maximum-length-for-aws-batch-tags-which-is-128-characters-for-keys-and-256-for-values-test-extra-long-suffix-to-exceed-limit') | 128 | false + 'valid-short-label' | 128 | true + and: + // Edge cases + null | 128 | false + '' | 128 | false + ' ' | 128 | true // Spaces are valid + } + + @Unroll + def 'should validate AWS Batch labels and return validation results' () { + given: + def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> IS_STRICT_MODE + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + when: + def result = handler.validateAwsBatchLabels(INPUT) + + then: + result == EXPECTED_RESULT + + where: + INPUT | IS_STRICT_MODE | EXPECTED_RESULT + // Valid labels - should pass in both modes + [validKey: 'validValue'] | false | [validKey: 'validValue'] + [validKey: 'validValue'] | true | [validKey: 'validValue'] + ['valid-key_123': 'valid-value_456'] | false | ['valid-key_123': 'valid-value_456'] + ['valid-key_123': 'valid-value_456'] | true | ['valid-key_123': 'valid-value_456'] + and: + // Mixed valid/invalid - normal mode warns and passes through, strict mode should be tested separately + ['validKey': 'validValue', 'invalid#key': 'invalid$value'] | false | ['validKey': 'validValue', 'invalid#key': 'invalid$value'] + and: + // Null/empty handling + null | false | null + [:] | false | [:] + ['validKey': null, 'goodKey': 'goodValue'] | false | ['goodKey': 'goodValue'] + } + + def 'should validate labels in submit request with warnings in normal mode' () { + given: + def task = Mock(TaskRun) + task.getName() >> 'batch-task' + task.getConfig() >> new TaskConfig( + memory: '8GB', + cpus: 4, + resourceLabels: [ + 'validLabel': 'validValue', + 'invalid#key': 'invalid$value', // Invalid characters + ('keyThatIsTooLongForAwsBatchWhichHas128CharacterLimit' + 'AndWillCauseValidationToFailWithASpecificErrorMessageThatTellsTheUserWhatWentWrong'): 'value' + ] + ) + + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + def handler = Spy(AwsBatchTaskHandler) { getExecutor() >> executor } + handler.@executor = executor + + when: + def req = handler.newSubmitRequest(task) + then: + 1 * handler.getSubmitCommand() >> ['bash', '-c', 'test'] + 1 * handler.maxSpotAttempts() >> 0 + 1 * handler.getAwsOptions() >> new AwsOptions() + 1 * handler.getJobQueue(task) >> 'test-queue' + 1 * handler.getJobDefinition(task) >> 'test-job-def' + 1 * handler.getEnvironmentVars() >> [] + + and: 'labels should be passed through unchanged despite validation warnings' + def tags = req.tags() + tags.size() == 3 + tags['validLabel'] == 'validValue' + tags['invalid#key'] == 'invalid$value' // Unchanged despite invalid characters + tags.containsKey('keyThatIsTooLongForAwsBatchWhichHas128CharacterLimit' + 'AndWillCauseValidationToFailWithASpecificErrorMessageThatTellsTheUserWhatWentWrong') + req.propagateTags() == true + } + + def 'should throw exception in strict mode for invalid labels' () { + given: + def labels = [ + 'validLabel': 'validValue', + 'invalid#key': 'invalid$value', // Invalid characters should cause exception + ('keyThatIsTooLongForAwsBatchWhichHas128CharacterLimit' + 'AndWillCauseValidationToFailWithASpecificErrorMessageThatTellsTheUserWhatWentWrong'): 'value' + ] + + def config = ['nextflow': ['enable': ['strict': true]]] + def session = Mock(Session) { + getConfig() >> config + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + def handler = Spy(AwsBatchTaskHandler) + handler.@executor = executor + + when: + handler.validateAwsBatchLabels(labels) + + then: 'exception should be thrown for invalid labels in strict mode' + thrown(ProcessUnrecoverableException) + } + + def 'should pass through valid labels in both normal and strict mode' () { + given: + def task = Mock(TaskRun) + task.getName() >> 'batch-task' + task.getConfig() >> new TaskConfig( + resourceLabels: [ + 'validLabel': 'validValue', + 'valid-key_123': 'valid-value_456', + 'key.with:path/chars=test+more@symbol': 'value with spaces' + ] + ) + + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> strictMode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + def handler = Spy(AwsBatchTaskHandler) { getExecutor() >> executor } + handler.@executor = executor + + when: + def req = handler.newSubmitRequest(task) + then: + 1 * handler.getSubmitCommand() >> ['bash', '-c', 'test'] + 1 * handler.maxSpotAttempts() >> 0 + 1 * handler.getAwsOptions() >> new AwsOptions() + 1 * handler.getJobQueue(task) >> 'test-queue' + 1 * handler.getJobDefinition(task) >> 'test-job-def' + 1 * handler.getEnvironmentVars() >> [] + + and: 'valid labels should pass through unchanged' + def tags = req.tags() + tags.size() == 3 + tags['validLabel'] == 'validValue' + tags['valid-key_123'] == 'valid-value_456' + tags['key.with:path/chars=test+more@symbol'] == 'value with spaces' + req.propagateTags() == true + + where: + strictMode << [false, true] // Test both normal and strict modes + } + + @Unroll + @See("https://github.com/nextflow-io/nextflow/pull/6211#discussion_r2161928856") + def 'should handle null values in labels during validation'() { + given: + def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + expect: + handler.validateAwsBatchLabels(INPUT) == EXPECTED + + where: + INPUT | EXPECTED + // Basic null value case - this addresses the PR comment: "when the item is "item": null is the aws tag silently dropped?" + ['item': null, 'validKey': 'validValue'] | ['validKey': 'validValue'] + + // Multiple null values + ['key1': null, 'key2': 'value2', 'key3': null] | ['key2': 'value2'] + + // All null values + ['key1': null, 'key2': null] | [:] + + // Mix of null and empty string + ['nullValue': null, 'emptyValue': '', 'validValue': 'good'] | ['emptyValue': '', 'validValue': 'good'] + } + + @See("https://github.com/nextflow-io/nextflow/pull/6211#discussion_r2161928856") + def 'should handle null keys in labels during validation'() { + given: + def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + when: 'creating map with actual null key' + def labels = new HashMap() + labels.put(null, 'validValue') + labels.put('validKey', 'validValue') + def result = handler.validateAwsBatchLabels(labels) + + then: 'null key is dropped' + result.size() == 1 + result['validKey'] == 'validValue' + !result.containsKey(null) + } + + @See("https://github.com/nextflow-io/nextflow/pull/6211#discussion_r2161928856") + def 'should handle both null keys and values during validation'() { + given: + def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + when: 'creating map with null key and null values' + def labels = new HashMap() + labels.put(null, 'someValue') // null key + labels.put('nullValue', null) // null value + labels.put(null, null) // both null (overwrites previous null key) + labels.put('validKey', 'validValue') + def result = handler.validateAwsBatchLabels(labels) + + then: 'only valid entry remains' + result.size() == 1 + result['validKey'] == 'validValue' + !result.containsKey(null) + !result.containsKey('nullValue') + } + + @See("https://github.com/nextflow-io/nextflow/pull/6211#discussion_r2161928856") + def 'should verify logging behavior for null handling during validation'() { + given: + def handler = new AwsBatchTaskHandler() + def config = ['nextflow': ['enable': ['strict': false]]] + def session = Mock(Session) { + getConfig() >> config + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + def logAppender = Mock(ch.qos.logback.core.Appender) + def logger = (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger(AwsBatchTaskHandler) + logger.addAppender(logAppender) + logger.setLevel(ch.qos.logback.classic.Level.WARN) + + when: 'validating labels with null values' + def labels = ['item': null, 'validKey': 'validValue'] // This is the exact case from PR comment + handler.validateAwsBatchLabels(labels) + + then: 'warning is logged for null value' + 1 * logAppender.doAppend(_) >> { args -> + def event = args[0] as ch.qos.logback.classic.spi.ILoggingEvent + assert event.level == ch.qos.logback.classic.Level.WARN + assert event.formattedMessage.contains('AWS Batch label dropped due to null value: key=item, value=null') + } + + cleanup: + logger.detachAppender(logAppender) + } + + @See("https://github.com/nextflow-io/nextflow/pull/6211#discussion_r2161928856") + def 'should verify no silent dropping during validation - PR comment verification'() { + given: 'This test specifically addresses the PR comment about silent dropping' + def handler = Spy(AwsBatchTaskHandler) + def session = Mock(Session) { + getConfig() >> Mock(ConfigObject) { + navigate('nextflow.enable.strict', false) >> false // Normal mode + } + } + def executor = Mock(AwsBatchExecutor) { getSession() >> session } + handler.@executor = executor + + when: 'processing the exact scenario from PR comment' + def labels = ['item': null] // "when the item is "item": null is the aws tag silently dropped?" + def result = handler.validateAwsBatchLabels(labels) + + then: 'result is empty (tag is dropped)' + result == [:] + + and: 'the method properly logs the dropped label (verified by observing the actual log output in test execution)' + // The actual logging is verified by the "should verify logging behavior for null handling during validation" test above + // This test focuses on the functional behavior: null values are correctly dropped from the result + true // The key point is that silent dropping has been replaced with logged dropping + } } diff --git a/plugins/nf-amazon/src/test/nextflow/cloud/aws/config/AwsBatchConfigTest.groovy b/plugins/nf-amazon/src/test/nextflow/cloud/aws/config/AwsBatchConfigTest.groovy index a6ed97d28f..cfe1969c7f 100644 --- a/plugins/nf-amazon/src/test/nextflow/cloud/aws/config/AwsBatchConfigTest.groovy +++ b/plugins/nf-amazon/src/test/nextflow/cloud/aws/config/AwsBatchConfigTest.groovy @@ -153,4 +153,5 @@ class AwsBatchConfigTest extends Specification { [terminateUnschedulableJobs: false] | false [terminateUnschedulableJobs: true] | true } + }