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

michaeljmarshall · 2023-06-06T15:49:15Z

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

doc-not-needed

eolivelli

+1

This issue has been discovered by our CI , in which we run additional tests on k8s functions.
Unfortunately they are not easily portable to the Apache OSS repo, as they need too many resources.

Maybe we could setup something with test contains and k3s (@nicoloboschi recently started to write some small scale tests with the helm chart and docker)

lhotari

LGTM

dave2wave

LGTM

FYI - we check multiple Pulsar branches on a daily basis based on docker images built daily.

codecov-commenter · 2023-06-06T21:53:16Z

Codecov Report

Merging #20513 (903af20) into master (3b862ae) will increase coverage by 36.12%.
The diff coverage is 60.27%.

@@              Coverage Diff              @@
##             master   #20513       +/-   ##
=============================================
+ Coverage     36.78%   72.90%   +36.12%     
- Complexity    12059    31916    +19857     
=============================================
  Files          1690     1867      +177     
  Lines        129001   138546     +9545     
  Branches      14041    15212     +1171     
=============================================
+ Hits          47453   101011    +53558     
+ Misses        75294    29504    -45790     
- Partials       6254     8031     +1777

Flag	Coverage Δ
inttests	`24.16% <15.06%> (+0.02%)`	⬆️
systests	`24.99% <0.00%> (+0.03%)`	⬆️
unittests	`72.20% <60.27%> (+40.26%)`	⬆️

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

Impacted Files	Coverage Δ
.../apache/pulsar/broker/admin/impl/PackagesBase.java	`76.19% <ø> (+23.89%)`	⬆️
...rg/apache/pulsar/broker/service/BrokerService.java	`80.44% <ø> (+24.12%)`	⬆️
.../apache/pulsar/functions/instance/ContextImpl.java	`61.00% <0.00%> (+18.41%)`	⬆️
...unctions/runtime/kubernetes/KubernetesRuntime.java	`38.73% <50.00%> (+38.73%)`	⬆️
...s/runtime/kubernetes/KubernetesRuntimeFactory.java	`80.21% <50.00%> (+80.21%)`	⬆️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	`72.05% <56.86%> (+41.36%)`	⬆️
...ker/authorization/PulsarAuthorizationProvider.java	`71.42% <100.00%> (+39.74%)`	⬆️
.../org/apache/pulsar/broker/admin/AdminResource.java	`79.57% <100.00%> (+35.76%)`	⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`64.00% <100.00%> (+53.83%)`	⬆️

... and 1424 files with indirect coverage changes

mattisonchao · 2023-06-07T01:18:52Z

Is it a minor break for admin cli?

michaeljmarshall · 2023-06-07T01:21:28Z

There is no change to the admin CLI in this PR.

### 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)

lhotari · 2023-06-07T05:37:32Z

Currently there is inconsistency across the different tools. For example in LocalRunner, there is arity = 1 for these parameters:

pulsar/pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java

Lines 177 to 184 in ec102fb

    
           @Parameter(names = "--useTls", description = "Use tls connection\n", hidden = true, arity = 1) 
        
           protected boolean useTls; 
        
           @Parameter(names = "--tlsAllowInsecureConnection", description = "Allow insecure tls connection\n", 
        
                   hidden = true, arity = 1) 
        
           protected boolean tlsAllowInsecureConnection; 
        
           @Parameter(names = "--tlsHostNameVerificationEnabled", description = "Enable hostname verification", hidden = true 
        
                   , arity = 1) 
        
           protected boolean tlsHostNameVerificationEnabled;

https://jcommander.org/#_boolean

I think that JCommander's design is bad in this aspect. Changing arity would be a breaking change.

michaeljmarshall · 2023-06-07T05:52:31Z

For what it's worth, I'll need to introduce a breaking change to make PIP 273 with hostname verification work because arity 0 assumes your default is false. This PR didn't change any of those though.

michaeljmarshall · 2023-06-07T18:57:15Z

Cherry picking this to 2.11, 2.10, and 2.9 by #20533

### 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)

### 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:40Z

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] TLS args admin download command use zero arity

903af20

michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/function doc-not-needed Your PR changes do not impact docs ready-to-test labels Jun 6, 2023

michaeljmarshall added this to the 3.1.0 milestone Jun 6, 2023

michaeljmarshall requested review from lhotari, nicoloboschi and dave2wave June 6, 2023 15:49

michaeljmarshall self-assigned this Jun 6, 2023

eolivelli approved these changes Jun 6, 2023

View reviewed changes

lhotari approved these changes Jun 6, 2023

View reviewed changes

dave2wave approved these changes Jun 6, 2023

View reviewed changes

michaeljmarshall added release/2.10.5 release/2.9.6 release/3.0.1 labels Jun 6, 2023

mattisonchao approved these changes Jun 7, 2023

View reviewed changes

michaeljmarshall merged commit 50b9a93 into apache:master Jun 7, 2023

michaeljmarshall deleted the fix-func-args branch June 7, 2023 02:46

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

michaeljmarshall mentioned this pull request Jun 7, 2023

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

Merged

1 task

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

liangyepianzhou added the cherry-picked/branch-2.10 label Jul 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaeljmarshall commented Jun 6, 2023

eolivelli left a comment

lhotari left a comment

dave2wave left a comment

codecov-commenter commented Jun 6, 2023

mattisonchao commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

lhotari commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

michaeljmarshall commented Jun 27, 2023

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

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

Conversation

michaeljmarshall commented Jun 6, 2023

Motivation

Modifications

Documentation

eolivelli left a comment

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

dave2wave left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 6, 2023

Codecov Report

mattisonchao commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

lhotari commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

michaeljmarshall commented Jun 7, 2023

michaeljmarshall commented Jun 27, 2023