Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use openssl111 in openresty dir in precedence #3603

Merged
merged 10 commits into from
Feb 20, 2021

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Feb 19, 2021

What this PR does / why we need it:

We use luasec since 5399a31, however, it uses the openssl libs and headers to compile itself, currently, we set two variables $OPENSSL_LIBDIR (e.g. /usr/local/openresty/openssl/lib) and $OPENSSL_INCDIR (e.g. /usr/local/openresty/openssl/include).

However, OpenResty 1.17.8 or higher version actually uses openssl111 as the dir name of openssl. So in this PR, we detect the existence of dir openssl111 and use it in precedence.

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@@ -39,6 +39,8 @@

- On some platforms, installing LuaRocks via the package manager will cause Lua to be upgraded to Lua 5.3, so we recommend installing LuaRocks via source code. if you install OpenResty and its OpenSSL develop library (openresty-openssl-devel for rpm and openresty-openssl-dev for deb) via the official repository, then [we provide a script for automatic installation](../utils/linux-install-luarocks.sh). If you compile OpenResty yourself, you can refer to the above script and change the path in it. If you don't specify the OpenSSL library path when you compile, you don't need to configure the OpenSSL variables in LuaRocks, because the system's OpenSSL is used by default. If the OpenSSL library is specified at compile time, then you need to ensure that LuaRocks' OpenSSL configuration is consistent with OpenResty's.

- If you are using OpenResty/1.17.8 or higher version, please installing openresty-openssl111-devel instead of openresty-openssl-devel.
Copy link
Member

@spacewander spacewander Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should warn for low version instead.
And also update the CI scripts.
And also update the Chinese doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the low version of OpenResty?

Also, Do we still need to update the Chinese?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should install openresty-openssl111-devel by default and install openresty-openssl-devel for OpenResty 1.15.8.
And we need to update the Chinese docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we only support > openresty 1.17.8.1, can it be easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we have a transition time, for now, we still need this.

@membphis
Copy link
Member

membphis commented Feb 19, 2021

@tokers
Copy link
Contributor Author

tokers commented Feb 19, 2021

more docs need to be updated:

https://github.com/apache/apisix/blob/master/doc/install-dependencies.md#centos-7

image

Do you view the file on my branch? It was updated already.

@membphis
Copy link
Member

Do you view the file on my branch? It was updated already.

https://github.com/apache/apisix/blob/22827880f46e50cc657371ed36a5ae76f412fa57/README.md#configure-and-installation

We need to update README.md.

image

@tokers
Copy link
Contributor Author

tokers commented Feb 19, 2021

Do you view the file on my branch? It was updated already.

https://github.com/apache/apisix/blob/22827880f46e50cc657371ed36a5ae76f412fa57/README.md#configure-and-installation

We need to update README.md.

image

The change belongs to the dependencies installation, which is covered in the install-dependencies.md, Is there anything should also be change there?

if [ "$OPENRESTY_VERSION" == "default" ]; then
sudo apt-get install openresty-openssl111-debug-dev
else
verlt $OPENRESTY_VERSION 1.17.8.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simply check if the version is 1.15.8.2 so there is no need to comment out set -euo pipefail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@membphis membphis linked an issue Feb 20, 2021 that may be closed by this pull request
sudo apt-get install "$openresty" lua5.1 liblua5.1-0-dev openresty-openssl-debug-dev
sudo apt-get install "$openresty" lua5.1 liblua5.1-0-dev

if [ "$OPENRESTY_VERSION" == "1.15.8.2" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will not support version 1.15.8.2 soon.

@membphis
Copy link
Member

The change belongs to the dependencies installation, which is covered in the install-dependencies.md, Is there anything should also be change there?

all fine now. thanks for your explaining

@membphis membphis merged commit bee574f into apache:master Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request help: need to update the readme document
3 participants