-
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-9193] upstream TLS v1.3 #1400
Conversation
d5e4a20
to
a0b5dd0
Compare
gateway/src/resty/http/proxy.lua
Outdated
if not port then | ||
port = default_port(uri) | ||
end | ||
|
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.
Sorry for being nosy. But wouldn't removing this block make the port
on the next line nil and cause the http and https uri to use the same pool?
I think we should make it local instead, ie
local port = default_port(uri)
Above is just my observation, please correct me if I'm wrong
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.
be my guest and noisy anytime :) You are totally right.
Fixing it now
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.
done!
c280eb9
to
a94c9ca
Compare
if not port then | ||
port = default_port(uri) | ||
end | ||
local port = default_port(uri) |
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.
Why was this change made? It seems to break existing behaviour
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.
no, it's not breaking existing behavior.
port
was not declared local, hence it was accessing some (not existing) global variable. Nginx was rightfully complaining about that because it could be causing race conditions. It seems me that there used to be some local port
declared and it was removed. 🤷
@@ -1,3 +1,4 @@ | |||
--- |
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.
Why make it multi-doc?
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.
This header does not make it multi-doc, it is just a document start marker that does not do any harm and pleases my IDE because I have a lint tool that requires document start markers. https://yamllint.readthedocs.io/en/stable/rules.html#module-yamllint.rules.document_start
- $(DOCKER_COMPOSE) down --volumes --remove-orphans | ||
- $(DOCKER_COMPOSE) -f $(PROVE_DOCKER_COMPOSE_FILE) down --volumes --remove-orphans | ||
- $(DOCKER_COMPOSE) -f $(DEVEL_DOCKER_COMPOSE_FILE) -f $(DEVEL_DOCKER_COMPOSE_VOLMOUNT_FILE) down --volumes --remove-orphans | ||
$(DOCKER) compose down --volumes --remove-orphans |
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.
I think this removes any potential compatibility with podman-compose
(not sure if it was working beforehand), as it is a separate command and not a subcommand for podman
.
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.
In this repo, we use docker
, not podman. I do not think it works with podman, even before this change. Check this PR for a discussion regarding Podman vs Docker. I would be happy to move to podman/buildah. But it needs work and maybe it cannot be done.
This is about moving forward to docker compose V2. Docker compose V1 is deprecated https://www.docker.com/blog/new-docker-compose-v2-and-v1-deprecation/
@@ -106,10 +98,9 @@ executors: | |||
docker: | |||
working_directory: /opt/app-root/apicast | |||
docker: | |||
- image: docker:stable | |||
- image: docker:23.0.2-cli-alpine3.17 |
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.
as in, docker stable is not good enough?
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.
docker:stable
does not implement docker V2 (there is not docker compose
command)
GKmV2MkFbWsoDCf1Z6I9h877Dpi+ilT5aX20Fv/eMpfUXCXwkciV2Yk9gtDmVJdR | ||
emAt/f7ldcVZ1dymTZIoKTVQCXexZliISq88ZwlmHbphF8LHC/0awC4XOvsDFHmg | ||
iAa0ukVxqPWNmpGp4wuRoa+Tpg== | ||
-----END PRIVATE KEY----- |
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.
We're adding a private key to repo? (I'm guessing it's for test purposes but want to make sure it's by accident)
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.
it is a self generated cert key used to deploy testing env. Not shipped in the apicast image. It will expire in 10 years.
Co-authored-by: Thomas Maas <[email protected]>
47cf0dc
to
f2d8897
Compare
What
Fixes https://issues.redhat.com/browse/THREESCALE-9193
This PR enables upstream powered by TLS v1.3 in two scenarios:
APIcast <> upstream TLS v1.3
APIcast <> HTTP proxy <> upstream TLS v1.3
Adds two docker environments:
docker-compose.upstream-tls.yml
anddocker-compose.forward-proxy.yml
to reproduce both scenarios.Addition of
proxy_ssl_protocols
was somewhat straightforward. From proxy_ssl_protocols doc, the TLSv1.3 parameter is used by default since 1.23.4. Currently APIcast is using 1.19.3.When a proxy is used, APIcast opens the (plain) connection to the proxy and sends CONNECT. Then APIcast starts the
ssl_handshake
using lua-resty-http library. The ssl_handshake docs leads to the tcpsock:sslhandshake doc, which in turn leads to lua-nginx-module lua_ssl_protocolsVerification steps for
APIcast <> upstream TLS v1.3
docker-compose.upstream-tls.yml
envThe
200 Ok
is enough to know that TLS v1.3 connection has been create between APIcast and upstreamVerification steps for
APIcast <> HTTP proxy <> upstream TLS v1.3
docker-compose.forward-proxy.yml
envThe
200 Ok
is enough to know that TLS v1.3 connection has been create between APIcast and upstream through the proxy