Skip to content

fix: use openssl111 in openresty dir in precedence#3603

Merged
membphis merged 10 commits into
apache:masterfrom
tokers:fix/compatible-of-openssl111
Feb 20, 2021
Merged

fix: use openssl111 in openresty dir in precedence#3603
membphis merged 10 commits into
apache:masterfrom
tokers:fix/compatible-of-openssl111

Conversation

@tokers

@tokers tokers commented Feb 19, 2021

Copy link
Copy Markdown
Contributor

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

Comment thread doc/install-dependencies.md Outdated

- 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.

@spacewander spacewander Feb 19, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

membphis commented Feb 19, 2021

Copy link
Copy Markdown
Member

@tokers

tokers commented Feb 19, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

@tokers

tokers commented Feb 19, 2021

Copy link
Copy Markdown
Contributor Author

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?

Comment thread utils/linux-install-openresty.sh Outdated
if [ "$OPENRESTY_VERSION" == "default" ]; then
sudo apt-get install openresty-openssl111-debug-dev
else
verlt $OPENRESTY_VERSION 1.17.8.1

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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