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.

Sign up for GitHub

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

Jump to bottom

Add NGINX extensions enhancement proposal #1567

Merged
merged 33 commits into from
Feb 29, 2024
Merged

Add NGINX extensions enhancement proposal #1567

merged 33 commits into from
Feb 29, 2024

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Adds enhancement proposal for NGINX extensions.

Closes #1411

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

@kate-osborn kate-osborn requested review from a team as code owners February 13, 2024 00:24
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 13, 2024
sjberman
sjberman reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@sjberman sjberman left a comment

Choose a reason for hiding this comment

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

Great job on this writeup, really thorough and well written.

docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
mpstefan
mpstefan approved these changes Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@mpstefan mpstefan left a comment

Choose a reason for hiding this comment

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

Left a few comments, but looks great! Thank you, Kate!

pleshakov
pleshakov reviewed Feb 14, 2024
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

this is great work!

docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
ciarams87
ciarams87 approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

🚀 amazing work Kate!

ADubhlaoich
ADubhlaoich reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM from the perspective of committing words into the repo.

I don't have anything else to add as I'm not deep enough in the codebase to offer meaningful feedback.

kate-osborn
kate-osborn commented Feb 20, 2024
View reviewed changes
Copy link
Contributor Author

@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.

Leaving a review for my in-person (🎉 ) code review with @f5yacobucci

docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Outdated Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
docs/proposals/nginx-extensions.md Show resolved Hide resolved
@kate-osborn kate-osborn force-pushed the docs/extensions-proposal branch 3 times, most recently from fd48acf to 114e522 Compare February 28, 2024 21:11
@kate-osborn
Appease linters
de7543d
@kate-osborn
Fix typos
fdb4313
@kate-osborn
Add more detail on target refs
39e14a3
@kate-osborn
Fix link
274c065
@kate-osborn
Update security section
0f6c990
@kate-osborn
Update Circuit Breaker
f077027
@kate-osborn
Update OTel
e4e61de
@kate-osborn
Resolver as plus only
0121fc0
@kate-osborn
Add note about GatewayClass role
b1217c8
@kate-osborn
Remove Gateway parameters ref
cb01a69
@kate-osborn
Add note about rate limiting
5faf17c
@kate-osborn
Update authentication extension to filter
7ef572d
@kate-osborn
Remove otel trace from gateway settings
fae899c
@kate-osborn
ClusterOperator -> Cluster Operator
c4cbe9c
@kate-osborn
Add note about propagating changes from GC to gateways
904438a
@kate-osborn
Add otel span attr to gateway settings
869974e
@kate-osborn
Add note about target ref restrictions in Policy section
5da37a0
@kate-osborn
Add note about OTel architecture documentation
f9a6994
@kate-osborn
Add namespace to observability future work
2424ed1
@kate-osborn
Call out propagate as a trace context option
5102683
@kate-osborn
Move timeouts to high priority
06aadc0
@kate-osborn
May instead of must for defaults/overrides
75da747
@kate-osborn
Remove circuit breaker API and add backup server to upstream settings
321f7ae
@kate-osborn
Update client and proxy settings policies
f639959
@kate-osborn kate-osborn merged commit 46973ca into nginxinc:main Feb 29, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADubhlaoich ADubhlaoich ADubhlaoich left review comments

@sindhushiv sindhushiv sindhushiv left review comments

@pleshakov pleshakov pleshakov approved these changes

@mpstefan mpstefan mpstefan approved these changes

@sjberman sjberman sjberman approved these changes

@ciarams87 ciarams87 ciarams87 approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
NGINX Gateway Fabric
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SPIKE] What extension points should each functionality group utilize?
7 participants
@kate-osborn @pleshakov @ADubhlaoich @mpstefan @sindhushiv @sjberman @ciarams87