-
Notifications
You must be signed in to change notification settings - Fork 170
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
[THREESCALE-10278] upgrade lua-resty-http to 0.17.1 #1434
[THREESCALE-10278] upgrade lua-resty-http to 0.17.1 #1434
Conversation
666b5e9
to
4728c85
Compare
03eaacf
to
33cbb7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good.
ad67268
to
4801707
Compare
resty-http was downgraded to 0.15 for ARM support 6a09893 Can we test that this image can be built out of this PR using ARM arch host? If you do not have available, I can ask some other member of the team to test it. |
So I tried to build ARM image using docker from
=> [ 8/16] RUN yum config-manager --add-repo http://packages.dev.3sca.net/dev_packages_3sca_net.repo 3.7s
=> ERROR [ 9/16] RUN yum install -y openresty-1.19.3 openresty-resty-1.19.3 openresty-opentelemetry-1.19.3 openresty-opentracing-1.19.3 opentracing-cpp-devel-1.3.0 6.9s
------
> [ 9/16] RUN yum install -y openresty-1.19.3 openresty-resty-1.19.3 openresty-opentelemetry-1.19.3 openresty-opentracing-1.19.3 opentracing-cpp-devel-1.3.0 libopentraci
cpp1-1.3.0 jaegertracing-cpp-client-0.3.1-13.el8:
4.901 Devel packages from 3Scale 81 kB/s | 220 kB 00:02
6.661 Error:
6.661 Problem 1: conflicting requests
6.661 - package jaegertracing-cpp-client-0.3.1-13.el8.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides libpthread.so.0(GLIBC_2.2.5)(64bit) needed by jaegertracing-cpp-client-0.3.1-13.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libpthread.so.0(GLIBC_2.3.2)(64bit) needed by jaegertracing-cpp-client-0.3.1-13.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libc.so.6(GLIBC_2.14)(64bit) needed by jaegertracing-cpp-client-0.3.1-13.el8.x86_64 from packages.dev.3sca.net
6.661 Problem 2: cannot install the best candidate for the job
6.661 - package openresty-1.19.3-23.el8.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides openresty-pcre >= 8.42-1 needed by openresty-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides openresty-zlib >= 1.2.11-3 needed by openresty-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 Problem 3: cannot install the best candidate for the job
6.661 - nothing provides openresty >= 1.19.3-23.el8 needed by openresty-resty-1.19.3-23.el8.noarch from packages.dev.3sca.net
6.661 Problem 4: cannot install the best candidate for the job
6.661 - package openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides libpthread.so.0(GLIBC_2.2.5)(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libpthread.so.0(GLIBC_2.3.2)(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides ld-linux-x86-64.so.2()(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides ld-linux-x86-64.so.2(GLIBC_2.3)(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libm.so.6(GLIBC_2.2.5)(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libpthread.so.0(GLIBC_2.12)(64bit) needed by openresty-opentelemetry-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 Problem 5: cannot install the best candidate for the job
6.661 - package openresty-opentracing-1.19.3-23.el8.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides libc.so.6(GLIBC_2.14)(64bit) needed by openresty-opentracing-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libopentracing.so.1()(64bit) needed by openresty-opentracing-1.19.3-23.el8.x86_64 from packages.dev.3sca.net
6.661 Problem 6: cannot install the best candidate for the job
6.661 - package opentracing-cpp-devel-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides libopentracing.so.1()(64bit) needed by opentracing-cpp-devel-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libopentracing_mocktracer.so.1()(64bit) needed by opentracing-cpp-devel-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libopentracing-cpp1 = 1.3.0-26.el8arches needed by opentracing-cpp-devel-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 Problem 7: cannot install the best candidate for the job
6.661 - package libopentracing-cpp1-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net does not have a compatible architecture
6.661 - nothing provides libc.so.6(GLIBC_2.14)(64bit) needed by libopentracing-cpp1-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 - nothing provides ld-linux-x86-64.so.2()(64bit) needed by libopentracing-cpp1-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 - nothing provides ld-linux-x86-64.so.2(GLIBC_2.3)(64bit) needed by libopentracing-cpp1-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.661 - nothing provides libdl.so.2(GLIBC_2.2.5)(64bit) needed by libopentracing-cpp1-1.3.0-26.el8arches.x86_64 from packages.dev.3sca.net
6.662 (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)
------
Dockerfile.devel:30 Checking http://packages.dev.3sca.net/ I don't see any aarch64 packages. Perhaps you have a better way to build the image? |
I’ve tried this but the verification steps won’t work OOTB on my system, due to missing RPMs for arm64 (aarch64). The packages we need (i.e. OpenResty 1.19.3-x and related ones within the same “family”, e.g. OpenTelemetry, OpenTracing, etc) are not available for this arch, neither in the default repos, nor in http://packages.dev.3sca.net/. Searching my notes after #1381, I found out a few interesting things. Starting with the fact that, back then, I only touched Another important piece is that apparently I failed then to build the devel/ci image while targeting linux/arm64 platform. What I have succeeded doing was building on darwin/arm64 for linux/amd64. To not completely diminish that as 100% useless, I recon it may have helped people to boot up the dev env container and run the test suite on a MacOS with Mx chip. But, in the end, we were just working around the limitations for running an arm64 image on an arm64 platform for dev purposes, but that’s all. Back to this PR... While still unable to 1. Smoke tests (devel/ci image)make dev-build IMAGE=quay.io/3scale/apicast-ci:openresty-1.19.3-pr1434-amd64
make development IMAGE=quay.io/3scale/apicast-ci:openresty-1.19.3-pr1434-amd64 Then, inside the development container: make dependencies
make busted
make prove Result: SUCCESS 2. “Connect via Proxy” (runtime image, Proxy w/ upstream using TLSv1.3)make runtime-build # <========= FAILED
cd dev-environments/https-proxy-upstream-tlsv1.3
make certs
make gateway
curl --resolve get.example.com:8080:127.0.0.1 -v "http://get.example.com:8080/?user_key=123"
docker compose -p https-proxy-upstream-tlsv13 logs -f proxy Result: FAILED 3. “Fetch config from 3scale” (devel/ci image)Inside the development container: THREESCALE_DEPLOYMENT_ENV=staging APICAST_LOG_LEVEL=debug APICAST_WORKER=1 APICAST_CONFIGURATION_LOADER=lazy APICAST_CONFIGURATION_CACHE=0 THREESCALE_PORTAL_ENDPOINT=https://[email protected] ./bin/apicast From the host: APICAST_IP=$(docker inspect apicast_build_0-development-1 | yq e -P '.[0].NetworkSettings.Networks.apicast_build_0_default.IPAddress' -)
curl -i -k -H "Host: example.com:443" -H "Accept: application/json" -H "Authorization: Bearer ${ACCESS_TOKEN}" "http://${APICAST_IP}:8080/foo" Output:
Result: SUCCESS (?) |
Thanks @guicassolato, for the last test, did you replace the placeholder values with actual 3scale values? It may also fail if you are using an older version of lua-resty-http (0.15), maybe delete the @eguzki so I guess we are good to merge this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. Very close to be merged. Just some enhancements proposed.
Thanks @guicassolato That was what I was missing. I was wondering how the hell you build dev image. The aim is to allow arm64 based users to develop APIcast. If they cannot build runtime image for arm64, that's unfortunate but not a blocking issue if they can still build and run amd64 images. |
f301699
to
29f940b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
just one change in the CHANGELOG requested and ready to be merged.
There is a failing check about codecov. It says that the tests only cover the 72% of the added code in this PR (not the overall code coverage). While we could be more restrictive about this, I tend to think that 70% of code coverage either for the patch or the project is good enough and should not be blocking the merge. |
29f940b
to
3c5199c
Compare
codecov is a strange one. It's fine now 😅 |
….lua To avoid having to call a resolver when used with proxies, all DNS resolution should happen inside the connect() method
3c5199c
to
9436b44
Compare
Rebased! |
@tkan145 the second part of the verification steps cannot be run. You did not specify the 3scale configuration fetched from 3scale API, hence I cannot reproduce. Nevertheless, I consider the first part good enough. And together with the (old) e2e tests passing, I consider the PR tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Updated verification steps. Because this has been approved. I will merge now |
What
Fix https://issues.redhat.com/browse/THREESCALE-10278
lua-resty-http 0.17.1 is also required for https://issues.redhat.com/browse/THREESCALE-5105
This PR is mainly a refactoring of existing code so no additional integration tests/unittests are added.
Verification Steps
Request should return 200
Replace
<user_key>
with the actual user key. The response should be HTTP/1.1 200 OKFrom the APIcast log, the request to fetch configuration file return 200