ADBDEV-6683 TLS options implemented for PXF FDW #146

dkovalev1 · 2024-12-04T10:48:44Z

TLS options implemented for PXF FDW

The requirement for using TLS is implemented for PXF FDW. The
communication takes place between the segment node and the PXF server and is
pointless without the PXF server. The testing shall be done in conjunction with
PXF server changes and is out of scope for this commit.

To test TLS support there is a new test container Dockerfile_ssl, which adds
PXF_SSL variables and additional SSL-related setup.

external-table/src/libchurl.c

dkovalev1 · 2024-12-17T04:16:03Z

bender build

dkovalev1 · 2024-12-17T06:10:52Z

bender build

RekGRpth · 2024-12-17T15:24:33Z

I don't like the fact that more or less similar files from external tables and fdw have become more different.

dkovalev1 · 2024-12-17T15:38:08Z

I don't like the fact that more or less similar files from external tables and fdw have become more different.

Yes, it looks not good if you are talking about libchurl. But someone put them there and started to diverge them. IMHO proper solution would be to have a single copy of libchurl and use it from both external tables and fwd. But it is out of scope of this task.

fdw/pxf_option.h

fdw/pxf_option.c

RekGRpth · 2024-12-20T04:03:20Z

fdw/pxf_bridge.c

+	if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {
+		ssl_options = churl_make_ssl_options(pxfsstate->options);
+	}
+
+	pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers, ssl_options);
+	if (ssl_options != NULL) {
+		free_churl_ssl_options(ssl_options);
+	}


why not simply

Suggested change

if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {

ssl_options = churl_make_ssl_options(pxfsstate->options);

}

pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers, ssl_options);

if (ssl_options != NULL) {

free_churl_ssl_options(ssl_options);

}

pxfsstate->churl_handle = churl_init_download(pxfsstate->uri.data, pxfsstate->churl_headers);

if (strcmp("https", pxfsstate->options->pxf_protocol) == 0) {

churl_set_ssl_options(pxfsstate->churl_handle, pxfsstate->options);

}

and in external table do similar thing

ssl_options has a type churl_ssl_options which has defined in libchurl, while pxfsstate->options is of type PxfOptions defined in the Pxf. Idea of making churl_ssl_options was to not introduce into libchurl extra dependency of higher level PXF.

The churl_set_ssl_options function could be called differently and placed outside of libchurl.

SSL logically looks very like an connection properties and I decided to set them as a part of libchurl.

Why not do something similar to pxfsstate->churl_headers with a common type for SSL options?

pxfsstate->churl_headers is a void pointer and does not discover specific type structure. Unlike it, churl_ssl_options is typed. Placing it to the pxfsstate breaks the fact that pxfsstate does not now discover churl-dependent things.
Also, instances of churl_ssl_options are used locally and independently, I don't see any need to put it into pxfsstate.

whitehawk · 2024-12-20T04:03:49Z

fdw/pxf_bridge.c

 	pxfcstate->pxf_host = pstrdup(pxfsstate->options->pxf_host);
+	if (ssl_options != 0) {


Why we compare not with NULL? And why do we need to allocate the second time? Can't we use the first allocation?

I had to allocate it second time to have it in the transaction's memory context. It's not related to the first time allocation.

Why not to make the first allocation already in the transaction's memory context?

These instances are independent logically. Second allocation underscores this fact.

This is more change-proof, it's easier to keep them independent in case of further modifications.

We won't save much of space and time if reuse memory in other context.

whitehawk · 2024-12-24T01:14:52Z

fdw/pxf_option.c

@@ -242,6 +255,16 @@ pxf_fdw_validator(PG_FUNCTION_ARGS)
 								errmsg("the %s option cannot be defined at the foreign-data wrapper level",
 									   FDW_OPTION_DISABLE_PPD)));
 		}
+		else if (strcmp(def->defname, FDW_OPTION_SSL_VERIFY_PEER) == 0)


Can't we move FDW_OPTION_SSL_VERIFY_PEER also to valid_options[] and eliminate this check?

valid_options and ValidateOption() has a check about option placement, while this check for FDW_OPTION_SSL_VERIFY_PEER also has a type validation which I find rather important.
Of course, it's still possible to add more check types to ValidateOption() but it's rather serious refactoring which is not related to current PR. I'd leave it as is.

TLS options implemented for PXF external tables The requirement for using TLS is implemented for PXF external tables. Communication takes place between the segment node and the PXF server and is pointless without the PXF server. Testing should be done in conjunction with PXF server changes and is out of scope for this commit. PXF external tables are controlled by environment variables. The following options are affected by this commit: PXF_PROTOCOL: Type string, default http. Defines the protocol for communication with the PXF server. Possible values are http and https. PXF_SSL_CACERT: Type string, default /home/gpadmin/arenadata_configs/cacert.pem. Specifies the absolute path to the CA certificate that signed the server certificate. Refers to CURLOPT_CAINFO. PXF_SSL_CERT: Type string, default client.pem. Specifies the path to the client certificate. Refers to CURLOPT_SSLCERT. PXF_SSL_CERT_TYPE: Type string, default pem. Defines the type of the client certificate. Refers to CURLOPT_SSLCERTTYPE. PXF_SSL_KEY: Type string, default empty. Defines the path to the client's SSL private key. Refers to CURLOPT_SSLKEY. PXF_SSL_KEYPASSWD: Type string, default empty. Defines the password for the client SSL private key. Refers to CURLOPT_KEYPASSWD. PXF_SSL_VERIFY_PEER: Type long, default 1. Defines whether the client should verify the server certificate. Refers to CURLOPT_SSL_VERIFYPEER. --------- Co-authored-by: Denis Kovalev <[email protected]>

hilltracer · 2024-12-27T15:05:43Z

Please provide how can we test all changed functions inside pxf_bridge.c? Is it enough simple select for testing PxfBridgeImportStart, PxfBridgeCancel, PxfBridgeExportStart? Please specify how these functions are called after query execution.

fdw/libchurl.c

fdw/libchurl.h

fdw/pxf_bridge.c

dkovalev1 · 2025-01-16T06:34:40Z

Please provide how can we test all changed functions inside pxf_bridge.c? Is it enough simple select for testing PxfBridgeImportStart, PxfBridgeCancel, PxfBridgeExportStart? Please specify how these functions are called after query execution.

To test changes easier and more stable I introduced test fdw/sql/pxf_fdw_ssl.sql. To run it we need to have support for SSL in PXF, that's why this PR has been rebased to feature/6581. To have a possibility to run both SSL related and non-SSL tests, a new container appeared. This scenario covers both PxfBridgeImportStart and PxfBridgeExportStart. PxfBridgeCancel can not be tested easily, only by interrupting long running query manually.

Implementation according to SRS

675c1dd

RekGRpth reviewed Dec 9, 2024

View reviewed changes

external-table/src/libchurl.c Outdated Show resolved Hide resolved

dkovalev1 added 2 commits December 16, 2024 08:42

Revert unwanted change

0592f3c

Revert unwanted change

eadd22e

RekGRpth reviewed Dec 16, 2024

View reviewed changes

external-table/src/libchurl.c Outdated Show resolved Hide resolved

dkovalev1 added 3 commits December 16, 2024 08:44

Merge branch 'pxf-6.x' into ADBDEV-6683

c62d1b5

Implement validation

3386d35

Merge branch 'pxf-6.x' into ADBDEV-6683

70f5bfb

Rework ssl options initialization for Pxf bridge

e82be59

This comment was marked as resolved.

Sign in to view

whitehawk reviewed Dec 20, 2024

View reviewed changes

fdw/pxf_option.h Outdated Show resolved Hide resolved

fdw/pxf_option.c Outdated Show resolved Hide resolved

RekGRpth reviewed Dec 20, 2024

View reviewed changes

whitehawk reviewed Dec 20, 2024

View reviewed changes

dkovalev1 changed the base branch from pxf-6.x to feature/ADBDEV-6267 December 23, 2024 04:30

dkovalev1 added 4 commits December 23, 2024 09:30

Merge branch 'feature/ADBDEV-6267' into ADBDEV-6683

0075a05

Adjust indentation

97b5a3a

Move SSL options validation to the ValidateOption

74f2f3d

Use NULL instead of 0

607a381

whitehawk reviewed Dec 24, 2024

View reviewed changes

hilltracer self-requested a review December 24, 2024 12:02

hilltracer reviewed Dec 27, 2024

View reviewed changes

fdw/libchurl.c Outdated Show resolved Hide resolved

fdw/libchurl.h Outdated Show resolved Hide resolved

fdw/libchurl.h Outdated Show resolved Hide resolved

fdw/libchurl.h Outdated Show resolved Hide resolved

fdw/pxf_bridge.c Outdated Show resolved Hide resolved

dkovalev1 added 2 commits January 9, 2025 08:48

Whitespace changes

3f89be8

remove extra tabs

5ec9b9d

dkovalev1 changed the base branch from feature/ADBDEV-6267 to feature/ADBDEV-6581 January 15, 2025 05:10

dkovalev1 added 5 commits January 15, 2025 10:11

Merge branch 'feature/ADBDEV-6581' into ADBDEV-6683

695aa30

Implement FDW SSL automated test in container

f0a5d5b

Add writable test

d638c69

Merge branch 'feature/ADBDEV-6267' into ADBDEV-6683

5e13f4c

Move SSL related testing to separated container

54981e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADBDEV-6683 TLS options implemented for PXF FDW #146

ADBDEV-6683 TLS options implemented for PXF FDW #146

dkovalev1 commented Dec 4, 2024 •

edited

Loading

dkovalev1 commented Dec 17, 2024

dkovalev1 commented Dec 17, 2024

RekGRpth commented Dec 17, 2024

dkovalev1 commented Dec 17, 2024

This comment was marked as resolved.

RekGRpth Dec 20, 2024 •

edited

Loading

dkovalev1 Dec 23, 2024

RekGRpth Dec 23, 2024

dkovalev1 Dec 23, 2024

RekGRpth Dec 23, 2024

dkovalev1 Dec 27, 2024

whitehawk Dec 20, 2024

dkovalev1 Dec 23, 2024

whitehawk Dec 23, 2024

dkovalev1 Dec 27, 2024

whitehawk Dec 24, 2024

dkovalev1 Dec 24, 2024

hilltracer commented Dec 27, 2024 •

edited

Loading

dkovalev1 commented Jan 16, 2025

		pxfcstate->pxf_host = pstrdup(pxfsstate->options->pxf_host);
		if (ssl_options != 0) {

ADBDEV-6683 TLS options implemented for PXF FDW #146

Are you sure you want to change the base?

ADBDEV-6683 TLS options implemented for PXF FDW #146

Conversation

dkovalev1 commented Dec 4, 2024 • edited Loading

dkovalev1 commented Dec 17, 2024

dkovalev1 commented Dec 17, 2024

RekGRpth commented Dec 17, 2024

dkovalev1 commented Dec 17, 2024

This comment was marked as resolved.

RekGRpth Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hilltracer commented Dec 27, 2024 • edited Loading

dkovalev1 commented Jan 16, 2025

dkovalev1 commented Dec 4, 2024 •

edited

Loading

RekGRpth Dec 20, 2024 •

edited

Loading

hilltracer commented Dec 27, 2024 •

edited

Loading