You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/08/28 20:29:17 UTC
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7859
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Thrift commit taken from:
https://github.com/henryr/thrift/commit/bd586c35297f5581c179ed57982c12d70a37e661
"This patch slightly changes the TLS configuration code to remove the
compile-time checks that were used to disable 1.1+ support based on the
OpenSSL library on the build machine.
Instead, we define the symbols we need ourselves if necessary, and rely
on runtime behaviour to catch errors or unsupported configurations."
Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
---
A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
1 file changed, 77 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/59/7859/1
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#4).
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Thrift commit taken from:
https://github.com/henryr/thrift/commit/9eea77c
"This patch slightly changes the TLS configuration code to remove the
compile-time checks that were used to disable 1.1+ support based on the
OpenSSL library on the build machine.
Instead, we define the symbols we need ourselves if necessary, and rely
on runtime behaviour to catch errors or unsupported configurations."
Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
---
M buildall.sh
A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
2 files changed, 93 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/59/7859/4
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 4
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Thrift commit taken from:
https://github.com/henryr/thrift/commit/d828da9
"This patch slightly changes the TLS configuration code to remove the
compile-time checks that were used to disable 1.1+ support based on the
OpenSSL library on the build machine.
Instead, we define the symbols we need ourselves if necessary, and rely
on runtime behaviour to catch errors or unsupported configurations."
Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
---
M buildall.sh
A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
2 files changed, 93 insertions(+), 1 deletion(-)
Approvals:
Henry Robinson: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 5
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch:
PS4, Line 35: SSL_OP_NO_TLSv1_2 0x08000000L
> I'm a tad bit confused; if OpenSSL can't understand these options, and we p
SSL_CTX_set_options returns the options that are set after that call takes effect. So if you & those with the bitmask you passed in, you can tell whether or not the options took effect.
See line 81 for how that's handled.
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 4
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch:
PS4, Line 35: SSL_OP_NO_TLSv1_2 0x08000000L
I'm a tad bit confused; if OpenSSL can't understand these options, and we pass them into SSL_CTX_set_options(), what's the expected behavior?
Ideally it would be to return with an error (as opposed to undefined behavior), but have we tested that's what happens?
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 4
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4:
(1 comment)
Also fix a bug in how the options are checked after setting.
http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch:
PS4, Line 35: SSL_OP_NO_TLSv1_2 0x08000000L
> Thanks for the explanation. I think it would be good to add a comment to th
I think that comment would just explain in words what the code does - it's pretty clear that if the return value from SSL_CTX_set_options isn't what we expect, we throw an error.
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 4
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7859
to look at the new patch set (#5).
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Thrift commit taken from:
https://github.com/henryr/thrift/commit/d828da9
"This patch slightly changes the TLS configuration code to remove the
compile-time checks that were used to disable 1.1+ support based on the
OpenSSL library on the build machine.
Instead, we define the symbols we need ourselves if necessary, and rely
on runtime behaviour to catch errors or unsupported configurations."
Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
---
M buildall.sh
A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
2 files changed, 93 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/59/7859/5
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 5
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#3).
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Thrift commit taken from:
https://github.com/henryr/thrift/commit/bd586c35297f5581c179ed57982c12d70a37e661
"This patch slightly changes the TLS configuration code to remove the
compile-time checks that were used to disable 1.1+ support based on the
OpenSSL library on the build machine.
Instead, we define the symbols we need ourselves if necessary, and rely
on runtime behaviour to catch errors or unsupported configurations."
Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
---
M buildall.sh
A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
2 files changed, 79 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/59/7859/3
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 3
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 5: Code-Review+2 Verified+1
Verified by a toolchain build.
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 5
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
LGTM, just need to add a comment as mentioned below, so it's clear while reading the code.
http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch
File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch:
PS4, Line 35: SSL_OP_NO_TLSv1_2 0x08000000L
> SSL_CTX_set_options returns the options that are set after that call takes
Thanks for the explanation. I think it would be good to add a comment to that effect at L81.
--
To view, visit http://gerrit.cloudera.org:8080/7859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e
Gerrit-PatchSet: 4
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes