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

HTTPv3 and QUIC related changes #353

Merged
merged 18 commits into from
Aug 15, 2023
Merged

HTTPv3 and QUIC related changes #353

merged 18 commits into from
Aug 15, 2023

Conversation

oxpa
Copy link
Contributor

@oxpa oxpa commented Aug 3, 2023

Proposed changes

Add quic and http3 capabilities to the role

Checklist

  • I have read the CONTRIBUTING document
  • I have added Molecule tests that prove my fix is effective or that my feature works
  • I have checked that any relevant Molecule tests pass after adding my changes
  • I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md)

@oxpa oxpa requested a review from alessfg as a code owner August 3, 2023 21:27
@oxpa
Copy link
Contributor Author

oxpa commented Aug 3, 2023

my main concern at the moment is that 'quic' requires specific tls version and ssl certs configured. And I don't check for sanity there letting nginx fail configuration test. Not sure if this is the right approach

@alessfg alessfg added the feature New feature or request label Aug 4, 2023
@alessfg alessfg added this to the 0.7.1 milestone Aug 4, 2023
@alessfg
Copy link
Collaborator

alessfg commented Aug 7, 2023

We could check if the user is running a supported *ssl library version by checking if http3 is enabled via the dictionary and if so, checking for various *ssl libraries (see https://github.com/nginxinc/ansible-role-nginx-config/blob/main/tasks/config/template-config.yml#L46-L57 for an example on how to check for specific dictionary keys).

Providing a host key does not seem to be a hard requirement -- if I understand the docs correctly, nginx should create a new one if the directive is not specified.

defaults/main/template.yml Outdated Show resolved Hide resolved
Comment on lines 86 to 88
{% if main['quic'] is defined and main['quic']['bpf'] is defined and main['quic']['bpf'] is boolean %}
quic_bpf {{ main['quic']['bpf'] | ternary('on', 'off') }};
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As convenient as it is, this really should be included in the http3/quic template. Directives in this template belong exclusively to the ngx_core_module.

Instead, I would tweak the nginx.conf.j2 template to something along the lines of

{% if nginx_config_main_template['config']['main'] is defined %}
{% from 'core.j2' import main with context %}
{{ main(nginx_config_main_template['config']['main']) }}
{%- endif %}
{% if nginx_config_main_template['config']['main']['quic'] is defined %}
{% from 'http/modules.j2' import main with context %}
...
{%- endif %}

And we can then use scopes (!) to make sure that only the quic_bpf parameter/directive can be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, IMO, bpf settings should be moved to core module in nginx as well. But I get the point. I'll move it out.

templates/http/modules.j2 Show resolved Hide resolved
Comment on lines 355 to 386
{# NGINX HTTP v3 -- ngx_http_v3_module #}
{% macro http3(http3) %}
{% if http3['enabled'] is defined and http3['enabled'] is boolean %}
http3 {{ http3['enabled'] | ternary('on', 'off') }};
{% endif %}
{% if http3['hq'] is defined and http3['hq'] is boolean %}
http3_hq {{ http3['hq'] | ternary('on', 'off') }};
{% endif %}
{% if http3['max_concurrent_streams'] is defined and http3['max_concurrent_streams'] is number %}
http3_max_concurrent_streams {{ http3['max_concurrent_streams'] }};
{% endif %}
{% if http3['stream_buffer_size'] is defined %}
http3_stream_buffer_size {{ http3['stream_buffer_size'] }};
{% endif %}

{% endmacro %}

{# NGINX QUIC -- ngix_http_v3_module #}
{% macro quic(quic) %}
{% if quic['active_connection_id_limit'] is defined and quic['active_connection_id_limit'] is number %}
quic_active_connection_id_limit {{ quic['active_connection_id_limit'] }};
{% endif %}
{% if quic['gso'] is defined and quic['gso'] is boolean %}
quic_gso {{ quic['gso'] | ternary('on','off') }};
{% endif %}
{% if quic['host_key'] is defined %}
quic_host_key {{ quic['host_key'] }};
{% endif %}
{% if quic['retry'] is defined and quic['retry'] is boolean %}
quic_retry {{ quic['retry'] | ternary('on','off') }};
{% endif %}
{% endmacro %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would ideally have these two within the same macro block since they are both part of the same module. I don't know what the best way to do that would be, but maybe we could do something where the resulting dictionary would be:

httpv3:
  http3:
    hq: false
    max_concurrent_streams: 100
  quic:
    bpf: false
    gso: false

or:

http3:
  hq: false
  max_concurrent_streams: 100
  quic:
    bpf: false
    gso: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels like keeping quic and http3 configuration separate is a sensible thing to do. Even if they are listed on a single page in the manual.
In nginx, quic related code is in event/quic and http3 is in http/v3. So it's not quite the same module. It's just a convenient way of representing it for now.

If anything other than http3 will start using quic - it will appear as a separate module with the same options and we'll reuse this template.
And the manual is written the way it is, because in C code this is not a problem: quic is already a separate entity and if quic moves into a separate module no configuration will have to be changed.

Hope this makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does!

oxpa and others added 2 commits August 7, 2023 13:06
improve readability

Co-authored-by: Alessandro Fael Garcia <[email protected]>
improve readability

Co-authored-by: Alessandro Fael Garcia <[email protected]>

{% endmacro %}

{# NGINX QUIC -- ngix_http_v3_module #}
Copy link
Collaborator

@alessfg alessfg Aug 7, 2023

Choose a reason for hiding this comment

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

Suggested change
{# NGINX QUIC -- ngix_http_v3_module #}
{# NGINX QUIC -- ngx_http_quic_module #}{# This module is not "documented" but it does exist internally #}

defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
@@ -83,6 +83,9 @@ timer_resolution {{ main['timer_resolution'] }};
{% if main['working_directory'] is defined %}
working_directory {{ main['working_directory'] }};
{% endif %}
{% if main['quic'] is defined and main['quic']['bpf'] is defined and main['quic']['bpf'] is boolean %}
Copy link
Collaborator

@alessfg alessfg Aug 7, 2023

Choose a reason for hiding this comment

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

Suggested change
{% if main['quic'] is defined and main['quic']['bpf'] is defined and main['quic']['bpf'] is boolean %}
{% if main['quic']['bpf'] is defined and main['quic']['bpf'] is boolean %}{# ngx_http_quic_module #}{# This does not belong here but we are making an exception #}

We can simplify this a little bit I think

Copy link
Collaborator

@alessfg alessfg left a comment

Choose a reason for hiding this comment

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

Made a few extra suggestions :)

oxpa and others added 3 commits August 15, 2023 13:14
Co-authored-by: Alessandro Fael Garcia <[email protected]>
Co-authored-by: Alessandro Fael Garcia <[email protected]>
@alessfg
Copy link
Collaborator

alessfg commented Aug 15, 2023

One final ask, can you please update the CHANGELOG?

@alessfg alessfg merged commit 11b838a into nginxinc:main Aug 15, 2023
6 checks passed
@alessfg alessfg linked an issue Aug 15, 2023 that may be closed by this pull request
@alessfg alessfg mentioned this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Development

Successfully merging this pull request may close these issues.

Question - QUIC config
2 participants