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

Update how HTTP/2 is enabled #456

Closed
wants to merge 2 commits into from

Conversation

Cyb3r-Jak3
Copy link

@Cyb3r-Jak3 Cyb3r-Jak3 commented Nov 24, 2023

Type of Change

  • Something else: Updates how HTTP/2 is enabled using the new http2 on method instead of listen ... http2

What issue does this relate to?

Resolves #451

What should this PR do?

Changes how HTTP2 is enabled with

What are the acceptance criteria?

Verify that new HTTP/2 directive matches the docs.

Previous
firefox_AIwZwD86ZG

With changes
firefox_uKflHkIgPI

@MattIPv4 MattIPv4 self-requested a review November 27, 2023 21:43
@MattIPv4
Copy link
Contributor

MattIPv4 commented Dec 5, 2023

👀 I'm somewhat concerned this is going to be very breaking for most users? This directive isn't even in stable yet, and causes the entire configuration to error out. It was landed in 1.25.1 which is mainline, whereas 1.24.x is considered stable: https://nginx.org/en/CHANGES

NGINXConfig generally tries to stay as backwards compatible as possible, so until deprecated directives are completely removed, we generally keep using them to support the most versions possible.

@MattIPv4
Copy link
Contributor

MattIPv4 commented Dec 5, 2023

I think perhaps under the global NGINX tab, we need options to toggle what directives are being used? In the same 1.25.1 release, the ssl directive was also removed which NGINXConfig currently uses during the installation flow. Then again, perhaps that installation flow could be revisited to not require disabling SSL temporarily.

@Cyb3r-Jak3
Copy link
Author

Ah, I was following the mainline changes, not stable.

I agree that a toggle, maybe it could change between mainline and stable? I know that HTTP/3 support is now also in mainline version, so it would no longer require the custom build warning that is currently visible.

@MattIPv4
Copy link
Contributor

MattIPv4 commented Dec 6, 2023

Yeah, that sounds like a good way to approach it. Probably just a checkbox under the global NGINX tab for "enable NGINX mainline support" or smth.

@Cyb3r-Jak3
Copy link
Author

Should I include that as part of this PR or make a seperate one?

@MattIPv4
Copy link
Contributor

MattIPv4 commented Dec 6, 2023

I wouldn't worry about the SSL stuff in this PR, but adding the checkbox in this PR to allow the http2 change to be toggled would be great :)

@MattIPv4
Copy link
Contributor

MattIPv4 commented Feb 9, 2024

I'm going to close this PR out as there hasn't been any movement in a while. You're welcome to re-open or create a new one if you find the time to work on the changes :)

@MattIPv4 MattIPv4 closed this Feb 9, 2024
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.

Nginx 1.25.1+ [warn] the "listen ... http2" directive is deprecated
2 participants