-
Notifications
You must be signed in to change notification settings - Fork 77
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
Bring QUIC Back #291
Bring QUIC Back #291
Conversation
Co-authored-by: Raytonne <[email protected]> Co-authored-by: lxwzy <[email protected]> Co-authored-by: pi-314159 <[email protected]> Signed-off-by: pi-314159 <[email protected]>
nginx/README-QUIC.md
Outdated
docker build -f Dockerfile-QUIC . | ||
``` | ||
|
||
After building, remember the SHA256 hash of the image from the last line of the output. |
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 looks complicated: Why not built to a name (and reference that below)?
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.
updated
nginx/README-QUIC.md
Outdated
- **Without Port Forwarding:** | ||
|
||
```bash | ||
docker run -d SHA256_OF_THE_IMAGE |
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.
Use name
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.
updated
nginx/README-QUIC.md
Outdated
- **With Port Forwarding:** | ||
|
||
```bash | ||
docker run -d -p 80:80 -p 443:443 -p 443:443/udp SHA256_OF_THE_IMAGE |
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.
Use name
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.
updated
nginx/README-QUIC.md
Outdated
|
||
Replace `CONTAINER_ID` with the ID obtained from the previous step. | ||
|
||
Inside the container, nginx configuration files are located in `/etc/nginx`, and the nginx executable is at `/usr/sbin/nginx`. |
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.
Hmm -- this leaves users pretty much at their own devices... Other demos have USAGE.md files to help people getting going...
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.
partially fixed by providing an example server configuration...
I think it's straightforward for users who have used nginx before...
Thanks for the new integration @pi-314159 ! Do I get it right that this drops a QUIC-enabled client- and server- demo adding back only a server side? Edit/add: I guess what I'm asking for is a (new) USAGE-QUIC.md file that tells people simply running the binary dockerimage how they can test-drive (and configure for all supported alg combinations) the image? Or don't you want to make this possible (as also the docker build-and-push CI commands are missing)? |
Thank you for adding this QUIC-demo @pi-314159, this is really useful also as addition to the test server. Also tagging @ajbozarth who is looking at updating the oqs-demos. Two remarks:
|
Are you going to deploy this on the IBM test server instance @bhess or are you expecting someone else to write code, config and documentation? |
This was my thinking. |
I'll add a QUIC enabled curl Dockerfile in the curl folder after this PR gets merged.
Previously, we were using quictls; BoringSSL lacked support for some algorithms. However, I plan to add hybrid algorithm implementations to BoringSSL this week.
One reason I didn't add such feature is that BoringSSL's |
Signed-off-by: PI <[email protected]>
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'll add a QUIC enabled curl Dockerfile in the curl folder after this PR gets merged.
First removing the (old) QUIC demo completely before adding back functionality that has been removed doesn't sound good. If you'd like to proceed that way (this PR first, another PR later), then please remove the "quic removal" logic from this PR and add it to the second PR so we know the for sure we have a new demo replacing the old one functionally.
Signed-off-by: PI <[email protected]>
@pi-314159 Thanks for the oqs-demos/.circleci/config.yml Lines 87 to 93 in e810c8a
|
@baentsch I have the permission to merge. I'm not very familiar with GitHub CI at the moment, so I'll take care of adding the code to build and push the Docker images a bit later. If that works for you, I'll go ahead and merge it now. Also, please check the boringssl PR |
Created #294 to do this "for good" for all docker image creation. So please add your code only to GH workflow files.
I'd have preferred to check everything's working locally (say in a docker network), but I assume you did (?) and don't find the time myself right now, so go ahead. |
Yes I've tested these Dockerfiles on my machine. Please test them when you have time! |
Signed-off-by: PI <[email protected]>
No description provided.