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

Add field to NginxProxy to allow disabling HTTP2 #1925

Merged
merged 4 commits into from May 16, 2024

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented May 2, 2024

Proposed changes

Problem: We need a way to allow a user to disable HTTP2

Solution: Extend the NginxProxy CRD to include a DisableHTTP2 option which will apply to all servers

Testing: Unit and local testing, confirmed that the http2 directive is set correctly unless disabled. If disabled, confirmed that a condition is added to any GRPCRoutes.

Closes #1858

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Added configuration option to disable HTTP2 to the NginxProxy CRD

@github-actions github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels May 2, 2024
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.96%. Comparing base (869ac38) to head (1e02137).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1925      +/-   ##
==========================================
+ Coverage   86.90%   86.96%   +0.05%     
==========================================
  Files          88       89       +1     
  Lines        6056     6083      +27     
  Branches       50       50              
==========================================
+ Hits         5263     5290      +27     
  Misses        741      741              
  Partials       52       52              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ciarams87 ciarams87 force-pushed the feat/add-disable-http2-option branch 3 times, most recently from 5abffd1 to dc4828e Compare May 15, 2024 09:51
@ciarams87 ciarams87 marked this pull request as ready for review May 15, 2024 10:11
@ciarams87 ciarams87 requested a review from a team as a code owner May 15, 2024 10:11
apis/v1alpha1/nginxproxy_types.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/types.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 changed the title Add baseHTTPConfig to NginxProxy to allow disabling HTTP2 Add option to NginxProxy to allow disabling HTTP2 May 15, 2024
@ciarams87 ciarams87 changed the title Add option to NginxProxy to allow disabling HTTP2 Add field to NginxProxy to allow disabling HTTP2 May 15, 2024
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

Just a couple nits

@ciarams87 ciarams87 force-pushed the feat/add-disable-http2-option branch from 3b07215 to 8ba7bb1 Compare May 16, 2024 09:52
@ciarams87 ciarams87 force-pushed the feat/add-disable-http2-option branch from 8ba7bb1 to 9e09fbd Compare May 16, 2024 15:48
@ciarams87 ciarams87 force-pushed the feat/add-disable-http2-option branch from 9e09fbd to 1e02137 Compare May 16, 2024 16:30
@ciarams87 ciarams87 enabled auto-merge (squash) May 16, 2024 16:31
@ciarams87 ciarams87 merged commit 250792c into nginxinc:main May 16, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helm-chart Relates to helm chart release-notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add mechanism for user to disable HTTP/2 if required
4 participants