NIFI-3851 Add expression language support to AWS Endpoint Override UR…

[tequalsme]

NIFI-3851 Add expression language support to AWS Endpoint Override URL property

[jvwing]

Reviewing...

[jvwing]

@tequalsme Thanks for putting this together. The commit passes the full build, and manually running it with an S3 processor worked great.

But I would prefer not to add runner.setValidateExpressionUsage(false) to the unit tests for DyanmoDB, Kinesis, etc. They might use EL validation in the future, and I don't think it should be necessary based on precedents like ACCESS_KEY or PROXY_HOST that also use EL.

The problem appears to be a mismatch between those processors not configured to use the ENDPOINT_OVERRIDE property descriptor, but are nonetheless evaluating it when they call AbstractAWSProcessor::initializeRegionAndEndpoint. That situation predates your change, but adding EL causes the exception for those tests.

Instead, what would you think about the treatment given to REGION in that same method (AbstractAWSProcessor.java, ~line 210)?

protected void initializeRegionAndEndpoint(ProcessContext context) {
    // if the processor supports REGION, get the configured region.
    if (getSupportedPropertyDescriptors().contains(REGION)) {
        final String region = context.getProperty(REGION).getValue();
        if (region != null) {
            this.region = Region.getRegion(Regions.fromName(region));
            client.setRegion(this.region);
        } else {
            this.region = null;
        }
    }

    // if the endpoint override has been configured, set the endpoint.
    // (per Amazon docs this should only be configured at client creation)
    final String urlstr = StringUtils.trimToEmpty(context.getProperty(ENDPOINT_OVERRIDE).evaluateAttributeExpressions().getValue());
    if (!urlstr.isEmpty()) {
        this.client.setEndpoint(urlstr);
    }

}

The client is only modified when REGION is explicitly included on the concrete processor class. We could add a second if (getSupportedPropertyDescriptors().contains(ENDPOINT_OVERRIDE)) {... protecting the expression language from being evaluated for ENDPOINT_OVERRIDE. There is a similar, but different, approach used for PROXY_HOST on line 193 if (context.getProperty(PROXY_HOST).isSet()) {....

Now that I've made it this far, I'm not sure why there isn't such an if statement even without the expression language, like REGION. Maybe I'm missing something? Is this already familiar to you?

[tequalsme]

@jvwing Your suggestion sounds reasonable, I don't like all of the unrelated runner.setValidateExpressionUsage(false) either! I will update.

[jvwing]

Thanks again, @tequalsme. This has been merged to master.