You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "anmolnar (via GitHub)" <gi...@apache.org> on 2026/05/11 17:00:47 UTC
[PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
anmolnar opened a new pull request, #2467:
URL: https://github.com/apache/phoenix/pull/2467
### What changes were proposed in this pull request?
Parse ZooKeeper TLS properties from `hbase-site.xml` and add them to `sqlline` command line args.
### Why are the changes needed?
Phoenix `sqlline` thick client is unable to connect to TLS-only ZooKeeper ensemble.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Locally.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3236634221
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +129,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
Review Comment:
We should check if '`client.secure' in zkcfg` too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3226486570
##########
bin/sqlline.py:
##########
@@ -94,6 +94,7 @@ def kill_child():
java_cmd = phoenix_utils.java + ' ' + phoenix_utils.jvm_module_flags + \
' ' + opts + \
+ ' ' + getattr(phoenix_utils, "zk_tls_args", "") + \
Review Comment:
No, we cannot. If it doesn't get any value in the utils, it will be undefined and this code would throw an exception (tested).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#issuecomment-4433402773
@richardantal I moved `zk_tls_args` to module level in order to be defined at all times.
I'm not sure about the exception handler. What should be the logic inside the exception handler? I don't want the error to be swallowed and ignored, because if there's anything wrong with the configuration, the tool should refuse to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3227141600
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
I prefer the later option, but I other variables are not defined at top level of the file, just with `global`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3236637796
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
There could be better error when some of the keys are missing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3226496787
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
Default "" doesn't make any difference in Python. The field will be undefined. We have two options: use the `getattr` call as below, or define `zk_tls_args` at top level of this file.
Certainly I can add exception handler, but there's no exception handling anywhere else in this file. Does it make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3226496787
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
Default "" doesn't make any difference for this field. The field will be undefined. We have two options: use the `getattr` call as below, or define `zk_tls_args` at top level of this file.
Certainly I can add exception handler, but there's no exception handling anywhere else in this file. Does it make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3225567218
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
Can you please add Exception handling to this?
We want to continue if there is some problem with parsing the zookeeper parameters.
Please add a default "" value to zk_tls_args in case we don't have hbase.zookeeper.property.client.secure property.
##########
bin/sqlline.py:
##########
@@ -94,6 +94,7 @@ def kill_child():
java_cmd = phoenix_utils.java + ' ' + phoenix_utils.jvm_module_flags + \
' ' + opts + \
+ ' ' + getattr(phoenix_utils, "zk_tls_args", "") + \
Review Comment:
If we make sure zk_tls_args has some value, we could have `phoenix_utils.zk_tls_args` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3227103617
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
I think so, yes
There are much more thinks that could go wrong here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3236637796
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
'client.secure' in zkcfg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2467:
URL: https://github.com/apache/phoenix/pull/2467#discussion_r3237611033
##########
bin/phoenix_utils.py:
##########
@@ -126,6 +127,21 @@ def setPath():
# Try to provide something valid
hbase_conf_dir = '.'
+ global zk_tls_args
+ root = ET.parse(os.path.join(hbase_conf_dir, "hbase-site.xml")).getroot()
+ zk_hbase_prefix = "hbase.zookeeper.property."
+ zkcfg = {
+ prop.find("name").text[len(zk_hbase_prefix):]: prop.find("value").text
+ for prop in root.findall("property")
+ if prop.find("name").text.startswith(zk_hbase_prefix)
+ }
+ if zkcfg.get('client.secure').lower() == 'true':
+ zk_tls_args = '-Dzookeeper.client.secure=true ' + \
+ '-Dzookeeper.clientCnxnSocket=' + zkcfg['clientCnxnSocket'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \
+ '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' '
+
Review Comment:
This is done @richardantal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7848. Use ZK TLS properties from HBase config if present [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal merged PR #2467:
URL: https://github.com/apache/phoenix/pull/2467
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org