Verify at least one URL is present for cluster creation #19866

michaeljmarshall · 2023-03-20T18:21:20Z

Search before asking

I searched in the issues and found nothing similar.

Motivation

With #19151 and #19762, we verify that cluster urls are valid. It seems like we could also verify that there is at least one serviceUrl/serviceUrlTls and at least one brokerServiceUrl/brokerServiceUrlTls set.

Solution

Update the following code to add a check that some of those variables are set:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java

Lines 412 to 430 in e829672

    
           public void checkPropertiesIfPresent() throws IllegalArgumentException { 
        
               URIPreconditions.checkURIIfPresent(getServiceUrl(), 
        
                       uri -> Objects.equals(uri.getScheme(), "http"), 
        
                       "Illegal service url, example: http://pulsar.example.com:8080"); 
        
               URIPreconditions.checkURIIfPresent(getServiceUrlTls(), 
        
                       uri -> Objects.equals(uri.getScheme(), "https"), 
        
                       "Illegal service tls url, example: https://pulsar.example.com:8443"); 
        
               URIPreconditions.checkURIIfPresent(getBrokerServiceUrl(), 
        
                       uri -> Objects.equals(uri.getScheme(), "pulsar"), 
        
                       "Illegal broker service url, example: pulsar://pulsar.example.com:6650"); 
        
               URIPreconditions.checkURIIfPresent(getBrokerServiceUrlTls(), 
        
                       uri -> Objects.equals(uri.getScheme(), "pulsar+ssl"), 
        
                       "Illegal broker service tls url, example: pulsar+ssl://pulsar.example.com:6651"); 
        
               URIPreconditions.checkURIIfPresent(getProxyServiceUrl(), 
        
                       uri -> Objects.equals(uri.getScheme(), "pulsar") 
        
                               || Objects.equals(uri.getScheme(), "pulsar+ssl"), 
        
                       "Illegal proxy service url, example: pulsar+ssl://ats-proxy.example.com:4443 " 
        
                               + "or pulsar://ats-proxy.example.com:4080"); 
        
           }

Alternatives

No response

Anything else?

No response

Are you willing to submit a PR?

I'm willing to submit a PR!

The text was updated successfully, but these errors were encountered:

niharrathod · 2023-03-21T17:45:24Z

I can take up this. Could you please assign this to me?

michaeljmarshall · 2023-03-21T18:54:02Z

Sure, thanks @niharrathod

* Introduced validation 1. at least one valid serviceUrl/serviceUrlTls set 2. at least one valid brokerServiceUrl/brokerServiceUrlTls set

…sent for cluster creation apache#19866 * Introduced validation 1. at least one valid serviceUrl/serviceUrlTls set 2. at least one valid brokerServiceUrl/brokerServiceUrlTls set

niharrathod · 2023-04-06T04:44:47Z

@michaeljmarshall : FYI

unassigning myself

I tried to fix this and raised PR. In-fact actual change is easy and small, But the same change breaks almost all unit test-cases around broker sub-systems. So, overall fix becomes really huge.

If anyone looking into this issue and wants help, feel free to DM me.

michaeljmarshall · 2023-04-06T05:50:59Z

Thanks for your work @niharrathod. I didn't realize this requirement would have such wide reaching impact.

github-actions · 2023-05-07T01:58:38Z

The issue had no activity for 30 days, mark with Stale label.

JooHyukKim · 2023-05-13T11:47:02Z

@michaeljmarshall Would you consider, starting the verification as an option? Then enable the verification module-by-module

JooHyukKim · 2023-05-13T11:47:24Z

That way we can minimize change scope.

michaeljmarshall · 2023-05-17T18:39:04Z

I am not sure what the right solution is. Given that it sounds like there is a large diff, it might be more complicated than it is worth.

JooHyukKim · 2023-05-18T12:15:46Z

I am not sure what the right solution is. Given that it sounds like there is a large diff, it might be more complicated than it is worth.

The required changes being large, it is daunting IMHO. But I am just saying the verification (what this issue is trying to solve) still seem useful, especially when users need to rush --failing fast will help reduce time wasted. 👍🏻

So, taking this plan further, when pushing through, we can choose either of 2 possible scenarios below.

A. Gradual change.

Add new configuration option checkNeededUrlExist.
2. With usage CluterData.builder().checkNeededUrlExist(false).build();.
3. Value of checkNeededUrlExist will be false by default.
Document that you can create cluster safely by enabling checkNeededUrlExist(true)
Document that checkNeededUrlExist(true) will be enabled by default at some point.
Slowly fix broken tests.

Pros: breaking changes into bits.
Cons: Harder to revert.

B. All at once

We can pick up where #19965 was left off.

tisonkun · 2023-05-21T04:09:50Z

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

Lines 1469 to 1480 in aa7decc

    
           if (pulsar.getConfiguration().isTlsEnabled()) { 
        
               clientBuilder 
        
                   .serviceUrl(isNotBlank(cluster.getBrokerServiceUrlTls()) 
        
                       ? cluster.getBrokerServiceUrlTls() : cluster.getServiceUrlTls()) 
        
                   .enableTls(true) 
        
                   .tlsTrustCertsFilePath(pulsar.getConfiguration().getBrokerClientTrustCertsFilePath()) 
        
                   .allowTlsInsecureConnection(pulsar.getConfiguration().isTlsAllowInsecureConnection()) 
        
                   .enableTlsHostnameVerification(pulsar.getConfiguration().isTlsHostnameVerificationEnabled()); 
        
           } else { 
        
               clientBuilder.serviceUrl(isNotBlank(cluster.getBrokerServiceUrl()) 
        
                   ? cluster.getBrokerServiceUrl() : cluster.getServiceUrl()); 
        
           }

I'm now suspecting the condition of this issue -

From the source code it seems we actually tolerate only one of ServiceUrl or BrokerServiceUrl configured.

Perhaps we lift this check to checkPropertiesIfPresent and that should not break existing tests.

JooHyukKim · 2023-05-21T09:34:58Z

I'm now suspecting the condition of this issue -

From the source code it seems we actually tolerate only one of ServiceUrl or BrokerServiceUrl configured.

Perhaps we lift this check to checkPropertiesIfPresent and that should not break existing tests.

Is that so? I will try to apply the changes and verify later today 👍🏻

JooHyukKim · 2023-06-05T01:47:13Z

Perhaps we lift this check to checkPropertiesIfPresent and that should not break existing tests.

May I ask you what you meant by "lift this check", @tisonkun ?

From my perspective, whatever we do, we still need to fix all tests that configures only one of ServiceUrl or BrokerServiceUrl.

tisonkun · 2023-06-07T04:19:27Z

@JooHyukKim If you take a closer look at #19866 (comment), it supports fallback to serviceUrl when brokerServiceUrl is not configured.

The other use case of ClientDataImpl is:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java

Lines 239 to 245 in aa7decc

    
           if (StringUtils.isBlank(peerClusterData.getBrokerServiceUrl()) 
        
                   && StringUtils.isBlank(peerClusterData.getBrokerServiceUrlTls())) { 
        
               validationFuture.complete(newLookupErrorResponse(ServerError.MetadataError, 
        
                       "Redirected cluster's brokerService url is not configured", 
        
                       requestId)); 
        
               return; 
        
           }

The requirement is checked in place.

So...I must say here we have a series of config problems:

Can serviceUrl be a legacy option key of brokerServiceUrl?
Can serviceUrlTls be a legacy option key of brokerServiceUrlTls?
What's more vague, tlsEnable is deprecated and it seems no options fall back here, but it's dependent by NamespaceService to choose among all of these URLs .

Perhaps we can open a new issue to investigate these issues and see if we can have a clear model.

we still need to fix all tests that configures only one of ServiceUrl or BrokerServiceUrl.

Theoretically, I agree, while it's already different from the proposal of this issue:

verify that there is at least one serviceUrl/serviceUrlTls and at least one brokerServiceUrl/brokerServiceUrlTls set.

... although, if BrokerServiceUrl takes precedence over ServiceUrl, perhaps a warning is enough. That said, a clear model of these options is somehow missing.

JooHyukKim · 2023-06-08T11:23:48Z

My apologies for the delay, @tisonkun! 🙌 Here is my analysis and proposal in semi-PIP style, referencing #16691, thought it will be easier to understand. Let me know what you think 👍🏻👍🏻

Proposal : Log warning on failed verifcation

Analysis : current Implementation

Done about handling of BrokerServiceUrl vs ServiceUrl.
Below are the most obvious places that I could find with the help of IntelliJ IDE.
Further investigation(searching) shall be done only if we agree necessary.

Method Location	Behavior	URL
`WebSocketService.createClientInstance`	`BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used.	Link 1
`BrokerService.getReplicationClient`	`BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used.	Link 2
`NameSpaceService.getNamespaceClient`	`BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used.	Link 3
`TopicLookupBase.lookupTopicAsync`	Verifies at least one of `BrokerServiceUrl` and `ServiceUrl` is not blank.	Link 4

....with this search result and considering additional checks like using tlsEnable are spread around, throwing an exception on failed URL verification seems to cost more than what it's worth.

Proposal & Conculsion

although, if BrokerServiceUrl takes precedence over ServiceUrl, perhaps a warning is enough. That said, a clear model
of these options is somehow missing.

I also think a warning is a viable starting option at this point. Like you said, we still do not have a clear model of
these options. Even before clarifying the configuration model in the future, users can already enjoy the benefit from a proper warning on misconfiguration.

tisonkun · 2023-06-21T05:17:14Z

We have a good start point with @JooHyukKim's #20541. There is not yet a consensus on if we want to apply strict rules on these options. We can see how the warnings hint users.

Closing as no consensus...

If anyone has a proposal for changing the current behavior (I suspect we don't need eagerly), please open a new issue with the details.

lhotari · 2024-06-16T07:56:30Z

For Pulsar, it's sufficient that one of the 4 urls is present. Pulsar client will use http/https based lookup if the Pulsar binary protocol url is missing.

michaeljmarshall added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages good first issue Good for newcomers labels Mar 20, 2023

michaeljmarshall mentioned this issue Mar 20, 2023

[fix][broker] Fix create cluster with empty url #19762

Merged

5 tasks

sijie mentioned this issue Mar 20, 2023

ISSUE-19866: Verify at least one URL is present for cluster creation streamnative/pulsar-archived#5638

Open

2 tasks

michaeljmarshall assigned niharrathod Mar 21, 2023

niharrathod added a commit to niharrathod/pulsar that referenced this issue Mar 22, 2023

improvement apache#19866 :

77c6062

* Introduced validation 1. at least one valid serviceUrl/serviceUrlTls set 2. at least one valid brokerServiceUrl/brokerServiceUrlTls set

niharrathod mentioned this issue Mar 22, 2023

[fix][broker]: validation for at least one TlsUrl / nonTlsUrl is present for cluster creation niharrathod/pulsar#2

Closed

15 tasks

This was referenced Mar 25, 2023

[fix][broker]: validation for at least one TlsUrl / nonTlsUrl is present for cluster creation niharrathod/pulsar#3

Closed

[fix][broker]: validation for at least one TlsUrl / nonTlsUrl is present for cluster creation/update #19965

Closed

niharrathod removed their assignment Apr 6, 2023

github-actions bot added the Stale label May 7, 2023

github-actions bot removed the Stale label May 14, 2023

JooHyukKim mentioned this issue Jun 8, 2023

[improve][broker] Log warning on missing URL configuration for cluster creation/update #20541

Merged

14 tasks

tisonkun closed this as completed Jun 21, 2023

harshithasudhakar mentioned this issue Jul 17, 2023

[improve] [broker] Verify at least one URL is present for cluster creation #20821

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify at least one URL is present for cluster creation #19866

Verify at least one URL is present for cluster creation #19866

michaeljmarshall commented Mar 20, 2023

niharrathod commented Mar 21, 2023

michaeljmarshall commented Mar 21, 2023

niharrathod commented Apr 6, 2023

michaeljmarshall commented Apr 6, 2023 •

edited

Loading

github-actions bot commented May 7, 2023

JooHyukKim commented May 13, 2023

JooHyukKim commented May 13, 2023

michaeljmarshall commented May 17, 2023

JooHyukKim commented May 18, 2023

tisonkun commented May 21, 2023 •

edited

Loading

JooHyukKim commented May 21, 2023

JooHyukKim commented Jun 5, 2023

tisonkun commented Jun 7, 2023 •

edited

Loading

JooHyukKim commented Jun 8, 2023 •

edited

Loading

tisonkun commented Jun 21, 2023

lhotari commented Jun 16, 2024

Verify at least one URL is present for cluster creation #19866

Verify at least one URL is present for cluster creation #19866

Comments

michaeljmarshall commented Mar 20, 2023

Search before asking

Motivation

Solution

Alternatives

Anything else?

Are you willing to submit a PR?

niharrathod commented Mar 21, 2023

michaeljmarshall commented Mar 21, 2023

niharrathod commented Apr 6, 2023

michaeljmarshall commented Apr 6, 2023 • edited Loading

github-actions bot commented May 7, 2023

JooHyukKim commented May 13, 2023

JooHyukKim commented May 13, 2023

michaeljmarshall commented May 17, 2023

JooHyukKim commented May 18, 2023

A. Gradual change.

B. All at once

tisonkun commented May 21, 2023 • edited Loading

JooHyukKim commented May 21, 2023

JooHyukKim commented Jun 5, 2023

tisonkun commented Jun 7, 2023 • edited Loading

JooHyukKim commented Jun 8, 2023 • edited Loading

Proposal : Log warning on failed verifcation

Analysis : current Implementation

Proposal & Conculsion

tisonkun commented Jun 21, 2023

lhotari commented Jun 16, 2024

michaeljmarshall commented Apr 6, 2023 •

edited

Loading

tisonkun commented May 21, 2023 •

edited

Loading

tisonkun commented Jun 7, 2023 •

edited

Loading

JooHyukKim commented Jun 8, 2023 •

edited

Loading