update nginx template for TLS passthrough #2166

sarthyparty · 2024-06-20T20:12:34Z

Proposed changes

Problem: Nginx templates didn't support TLS passthrough inside stream block

Solution: I added nginx template for stream servers and also added a Layer4Server type to data plane

Testing: Tested the conversion from data plane type to nginx template

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #ISSUE

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.

sarthyparty · 2024-06-20T20:14:51Z

I didn't add anything to data plane for upstreams yet, I was thinking of reusing the existing type BackendGroup and just adding an IsStream boolean

kate-osborn

@sarthyparty nice work! Please see my review

charts/nginx-gateway-fabric/templates/deployment.yaml

internal/mode/static/nginx/conf/nginx.conf

internal/mode/static/nginx/config/generator.go

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/stream_servers_test.go

internal/mode/static/nginx/config/upstreams.go

internal/mode/static/state/dataplane/types.go

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/servers.go

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/generator_test.go

internal/mode/static/state/dataplane/types.go

internal/mode/static/nginx/config/maps.go

pleshakov · 2024-06-24T21:57:28Z

internal/mode/static/nginx/config/maps.go

+				Parameters: []http.MapParameter{
+					{
+						Value:  t.Hostname,
+						Result: "unix:/var/lib/nginx/" + t.Hostname + fmt.Sprint(t.Port) + ".sock",


Hostnames can have * which are not valid characters for file names.

are fine :)

server { listen unix:/var/run/*.server.sock; access_log off; return 200 "hello\n"; }

ls /var/run/*.sock '/var/run/*.server.sock'

internal/mode/static/nginx/conf/nginx.conf

sjberman

Can we rebase the feature branch feature/tls-passthrough on main to reduce the diff in this PR? Some extra stuff in there from other unrelated commits.

sjberman · 2024-06-26T21:04:47Z

internal/mode/static/handler.go

@@ -170,7 +170,7 @@ func newEventHandlerImpl(cfg eventHandlerConfig) *eventHandlerImpl {

 func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Logger, batch events.EventBatch) {
 	start := time.Now()
-	logger.V(1).Info("Started processing event batch")
+	logger.V(1).Info("Started processing event batch hello", "name", "sarthak")


Debugging log can be removed.

pleshakov · 2024-06-27T15:06:32Z

would it make sense to enable GitHub pipeline including linting for the branch feature/tls-passthrough ?

kate-osborn · 2024-06-27T15:45:56Z

would it make sense to enable GitHub pipeline including linting for the branch feature/tls-passthrough ?

Yes, definitely, good catch.

@lucacome what's the best way to do that? Should I just add the feature branch name to the on push in ci.yml on the feature branch?

kate-osborn

Leaving a partial review. I'm about half-way through, but want to give you something to work on while I finish.

internal/mode/static/nginx/conf/nginx.conf

internal/mode/static/nginx/config/generator_test.go

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/maps_test.go

internal/mode/static/nginx/config/maps.go

codecov · 2024-06-27T22:21:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.79%. Comparing base (04cfbc3) to head (3f5ee8b).

Additional details and impacted files

@@                     Coverage Diff                     @@
##           feature/tls-passthrough    #2166      +/-   ##
===========================================================
+ Coverage                    87.59%   87.79%   +0.20%     
===========================================================
  Files                           96       98       +2     
  Lines                         6698     6810     +112     
  Branches                        50       50              
===========================================================
+ Hits                          5867     5979     +112     
  Misses                         774      774              
  Partials                        57       57

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

internal/mode/static/nginx/config/servers.go

internal/mode/static/nginx/config/servers_template.go

internal/mode/static/nginx/config/servers_test.go

internal/mode/static/nginx/config/stream_servers.go

internal/mode/static/nginx/config/stream_servers_test.go

internal/mode/static/nginx/config/upstreams.go

internal/mode/static/state/dataplane/configuration.go

internal/mode/static/nginx/config/base_http_config_test.go

Problem: nginx configuration templates didn't support TLS passthrough Solution: I added a template setup fro stream servers

Co-authored-by: Kate Osborn <[email protected]>

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/maps_test.go

internal/mode/static/nginx/config/stream_servers_test.go

internal/mode/static/nginx/config/upstreams.go

internal/mode/static/nginx/config/upstreams_test.go

internal/mode/static/nginx/conf/nginx-plus.conf

internal/mode/static/nginx/conf/nginx.conf

internal/mode/static/nginx/config/servers.go

internal/mode/static/nginx/config/generator.go

internal/mode/static/nginx/config/maps.go

internal/mode/static/nginx/config/sockets.go

internal/mode/static/nginx/config/maps_template.go

internal/mode/static/nginx/config/stream_servers_test.go

internal/mode/static/nginx/config/upstreams.go

internal/mode/static/nginx/config/upstreams_test.go

Update nginx template for TLS passthrough Problem: nginx configuration templates didn't support TLS passthrough Solution: I added a template setup fro stream servers

github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Jun 20, 2024

sarthyparty requested a review from kate-osborn June 20, 2024 20:13

sarthyparty force-pushed the feature/add-nginx-template-config-for-tls-route branch 2 times, most recently from d76b1c4 to d48dba2 Compare June 20, 2024 20:23

kate-osborn suggested changes Jun 21, 2024

View reviewed changes

sarthyparty force-pushed the feature/add-nginx-template-config-for-tls-route branch from eb22a09 to fa2812f Compare June 21, 2024 19:27

kate-osborn suggested changes Jun 21, 2024

View reviewed changes

pleshakov reviewed Jun 24, 2024

View reviewed changes

sarthyparty force-pushed the feature/add-nginx-template-config-for-tls-route branch from a5e35b7 to af649df Compare June 26, 2024 20:34

github-actions bot added the documentation Improvements or additions to documentation label Jun 26, 2024

sarthyparty marked this pull request as ready for review June 26, 2024 20:35

sarthyparty requested review from a team as code owners June 26, 2024 20:35

sjberman reviewed Jun 26, 2024

View reviewed changes

sarthyparty force-pushed the feature/add-nginx-template-config-for-tls-route branch from 23f8b38 to 47ba96e Compare June 26, 2024 21:37

github-actions bot removed the documentation Improvements or additions to documentation label Jun 26, 2024

kate-osborn suggested changes Jun 27, 2024

View reviewed changes

sjberman reviewed Jun 28, 2024

View reviewed changes

internal/mode/static/nginx/config/base_http_config_test.go Outdated Show resolved Hide resolved

sarthyparty requested a review from kate-osborn July 1, 2024 20:49

sarthyparty and others added 5 commits July 1, 2024 14:52

Update nginx template for TLS passthrough

67fd5fc

Problem: nginx configuration templates didn't support TLS passthrough Solution: I added a template setup fro stream servers

add https server support and manifests made

0ff40ac

add hash directives to stream block

7fd449e

Update comment in generator.go

ffce60f

Co-authored-by: Kate Osborn <[email protected]>

remove pointer for map slice and fix tests

a491af2

sarthyparty added 3 commits July 2, 2024 14:28

update tests for stream maps and servers

c28a566

add stream upstream tests, remove invalid upstream ref

bb7e505

fix a code nit

e90646f

sarthyparty requested a review from kate-osborn July 2, 2024 20:40

kate-osborn suggested changes Jul 2, 2024

View reviewed changes

update server and map tests

83c0565

sarthyparty requested review from pleshakov, kate-osborn and sjberman July 2, 2024 22:50

sjberman reviewed Jul 3, 2024

View reviewed changes

internal/mode/static/nginx/conf/nginx-plus.conf Outdated Show resolved Hide resolved

internal/mode/static/nginx/conf/nginx.conf Outdated Show resolved Hide resolved

internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved

sarthyparty added 2 commits July 3, 2024 08:58

rename tlsservers

2135010

remove extra spaces in conf files

82bf8c4

sarthyparty requested a review from sjberman July 3, 2024 14:59

sjberman approved these changes Jul 3, 2024

View reviewed changes

pleshakov suggested changes Jul 5, 2024

View reviewed changes

sarthyparty added 3 commits July 5, 2024 11:26

remove escape slash and add stream folder to slice

d023f28

update socket name creation and map template

fb857b0

update upstream zone size and test

b98951e

sarthyparty requested a review from pleshakov July 5, 2024 18:50

update comment for zone size upstream

3f5ee8b

pleshakov approved these changes Jul 8, 2024

View reviewed changes

sarthyparty merged commit 1bc8be4 into nginx:feature/tls-passthrough Jul 8, 2024

update nginx template for TLS passthrough #2166

update nginx template for TLS passthrough #2166

Uh oh!

Conversation

sarthyparty commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Release notes

Uh oh!

sarthyparty commented Jun 20, 2024

Uh oh!

kate-osborn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pleshakov Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjberman left a comment

Choose a reason for hiding this comment

Uh oh!

sjberman Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

sarthyparty Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

pleshakov commented Jun 27, 2024

Uh oh!

kate-osborn commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kate-osborn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarthyparty commented Jun 20, 2024 •

edited

Loading

kate-osborn commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading