[fix][fn] Support customizing TLS config for function download command #20482

michaeljmarshall · 2023-06-02T18:59:40Z

Motivation

We support configuring custom TLS trusted certs when running functions, but not when downloading them. This PR fixes that by mapping the configuration in the same way we do with functions:

pulsar/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java

Lines 425 to 434 in f04252b

    
           args.add("--use_tls"); 
        
           args.add(Boolean.toString(authConfig.isUseTls())); 
        
           args.add("--tls_allow_insecure"); 
        
           args.add(Boolean.toString(authConfig.isTlsAllowInsecureConnection())); 
        
           args.add("--hostname_verification_enabled"); 
        
           args.add(Boolean.toString(authConfig.isTlsHostnameVerificationEnable())); 
        
           if (isNotBlank(authConfig.getTlsTrustCertsFilePath())) { 
        
               args.add("--tls_trust_cert_path"); 
        
               args.add(authConfig.getTlsTrustCertsFilePath()); 
        
           }

Modifications

Configure using TLS, allowing insecure TLS, hostname verification, and the custom trusted cert path

Verifying this change

I added a new test.

Documentation

doc-not-needed

codecov-commenter · 2023-06-02T21:42:07Z

Codecov Report

Merging #20482 (69c28fe) into master (e220a5d) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master   #20482      +/-   ##
============================================
+ Coverage     72.59%   72.94%   +0.34%     
- Complexity    31719    31907     +188     
============================================
  Files          1867     1867              
  Lines        138523   138529       +6     
  Branches      15204    15205       +1     
============================================
+ Hits         100555   101044     +489     
+ Misses        29955    29469     -486     
- Partials       8013     8016       +3

Flag	Coverage Δ
inttests	`24.20% <0.00%> (+0.06%)`	⬆️
systests	`25.11% <0.00%> (+0.12%)`	⬆️
unittests	`72.21% <100.00%> (+0.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...unctions/runtime/kubernetes/KubernetesRuntime.java	`38.96% <100.00%> (+0.61%)`	⬆️

... and 118 files with indirect coverage changes

tisonkun

LGTM.

Although, I don't think such a new feature should be picked to any previous versions.

May or may not we loose the rule for 3.0 the LTS version, but at least not for 2.9 which is end of life.

tisonkun

Sorry. I notice that it respects existing passed values instead of new options. Then it's reasonable to pick, while still may not for 2.9 (and 2.10?) which is EOL.

tisonkun · 2023-06-02T23:47:51Z

Merging...

michaeljmarshall · 2023-06-03T03:32:57Z

Sorry. I notice that it respects existing passed values instead of new options. Then it's reasonable to pick, while still may not for 2.9 (and 2.10?) which is EOL.

I haven't seen us declare 2.9 EOL, and the PIP 47 that would indicate that it is EOL hasn't really been followed. I'm going to cherry pick it, and then if it gets released, it's there.

michaeljmarshall · 2023-06-03T03:57:55Z

Looks like it doesn't apply cleanly. I'll have to investigate cherry picking it a bit more.

#20482) (cherry picked from commit ceed19c)

### Motivation #20482 broke the function download command with this error: ``` Expected a command, got false Usage: pulsar-admin [options] [command] [command options] Options: --admin-url Admin Service URL to which to connect. Default: http://localhost:8080/ ``` The problem is that the TLS args for hostname verification and for insecure TLS are zero arity, and therefore, we should not add the `false` or the `true` arguments. ### Modifications * Correct the changes made to the TLS args passed to the `pulsar-admin` CLI tool ### Documentation - [x] `doc-not-needed`

### Motivation #20482 broke the function download command with this error: ``` Expected a command, got false Usage: pulsar-admin [options] [command] [command options] Options: --admin-url Admin Service URL to which to connect. Default: http://localhost:8080/ ``` The problem is that the TLS args for hostname verification and for insecure TLS are zero arity, and therefore, we should not add the `false` or the `true` arguments. ### Modifications * Correct the changes made to the TLS args passed to the `pulsar-admin` CLI tool ### Documentation - [x] `doc-not-needed` (cherry picked from commit 50b9a93)

### Motivation This PR is a combination of #20482 and #20513 because cherry picking those PRs produced too many conflicts. ### Modifications * Made the same addition as the source PRs, though the code has changed a bit, so I had to make a few extra changes. ### Documentation - [x] `doc-not-needed`

### Motivation This PR is a combination of #20482 and #20513 because cherry picking those PRs produced too many conflicts. ### Modifications * Made the same addition as the source PRs, though the code has changed a bit, so I had to make a few extra changes. ### Documentation - [x] `doc-not-needed` (cherry picked from commit 8c9c6e9)

apache#20482) (cherry picked from commit ceed19c)

### Motivation apache#20482 broke the function download command with this error: ``` Expected a command, got false Usage: pulsar-admin [options] [command] [command options] Options: --admin-url Admin Service URL to which to connect. Default: http://localhost:8080/ ``` The problem is that the TLS args for hostname verification and for insecure TLS are zero arity, and therefore, we should not add the `false` or the `true` arguments. ### Modifications * Correct the changes made to the TLS args passed to the `pulsar-admin` CLI tool ### Documentation - [x] `doc-not-needed` (cherry picked from commit 50b9a93)

michaeljmarshall · 2023-06-27T21:55:41Z

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

[fix][fn] Support customizing TLS config for function download command

02820f4

michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug area/function doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.5 release/2.9.6 release/2.11.2 release/3.0.1 labels Jun 2, 2023

michaeljmarshall added this to the 3.1.0 milestone Jun 2, 2023

michaeljmarshall requested review from lhotari, cbornet and nicoloboschi June 2, 2023 18:59

michaeljmarshall self-assigned this Jun 2, 2023

Do not set unused arg --use-tls

69c28fe

lhotari approved these changes Jun 2, 2023

View reviewed changes

michaeljmarshall requested review from freeznet, nlu90 and wolfstudy June 2, 2023 21:43

tisonkun approved these changes Jun 2, 2023

View reviewed changes

tisonkun reviewed Jun 2, 2023

View reviewed changes

tisonkun merged commit ceed19c into apache:master Jun 2, 2023

michaeljmarshall deleted the allow-function-download-with-custom-cert branch June 3, 2023 03:30

michaeljmarshall added a commit that referenced this pull request Jun 3, 2023

[fix][fn] Support customizing TLS config for function download command (

cd08427

#20482) (cherry picked from commit ceed19c)

michaeljmarshall added the cherry-picked/branch-3.0 label Jun 3, 2023

michaeljmarshall mentioned this pull request Jun 6, 2023

[fix][fn] TLS args admin download command use zero arity #20513

Merged

1 task

michaeljmarshall mentioned this pull request Jun 7, 2023

[fix][fn] Configure pulsar admin for TLS #20533

Merged

1 task

michaeljmarshall added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Jun 7, 2023

michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Jun 7, 2023

[fix][fn] Support customizing TLS config for function download command (

a144970

apache#20482) (cherry picked from commit ceed19c)

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][fn] Support customizing TLS config for function download command #20482

[fix][fn] Support customizing TLS config for function download command #20482

michaeljmarshall commented Jun 2, 2023

codecov-commenter commented Jun 2, 2023

tisonkun left a comment

tisonkun left a comment

tisonkun commented Jun 2, 2023

michaeljmarshall commented Jun 3, 2023

michaeljmarshall commented Jun 3, 2023

michaeljmarshall commented Jun 27, 2023

	args.add("--use_tls");
	args.add(Boolean.toString(authConfig.isUseTls()));
	args.add("--tls_allow_insecure");
	args.add(Boolean.toString(authConfig.isTlsAllowInsecureConnection()));
	args.add("--hostname_verification_enabled");
	args.add(Boolean.toString(authConfig.isTlsHostnameVerificationEnable()));
	if (isNotBlank(authConfig.getTlsTrustCertsFilePath())) {
	args.add("--tls_trust_cert_path");
	args.add(authConfig.getTlsTrustCertsFilePath());
	}

[fix][fn] Support customizing TLS config for function download command #20482

[fix][fn] Support customizing TLS config for function download command #20482

Conversation

michaeljmarshall commented Jun 2, 2023

Motivation

Modifications

Verifying this change

Documentation

codecov-commenter commented Jun 2, 2023

Codecov Report

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun commented Jun 2, 2023

michaeljmarshall commented Jun 3, 2023

michaeljmarshall commented Jun 3, 2023

michaeljmarshall commented Jun 27, 2023