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 <50597707+kate-osborn@users.noreply.github.com>

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 requested 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 requested 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 requested 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 <50597707+kate-osborn@users.noreply.github.com>

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 requested 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 nginxinc:feature/tls-passthrough Jul 8, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update nginx template for TLS passthrough #2166

update nginx template for TLS passthrough #2166

sarthyparty commented Jun 20, 2024 •

edited

Loading

sarthyparty commented Jun 20, 2024

kate-osborn left a comment

pleshakov Jun 24, 2024

sjberman left a comment

sjberman Jun 26, 2024

sarthyparty Jun 26, 2024

pleshakov commented Jun 27, 2024

kate-osborn commented Jun 27, 2024 •

edited

Loading

kate-osborn left a comment

codecov bot commented Jun 27, 2024 •

edited

Loading

update nginx template for TLS passthrough #2166

update nginx template for TLS passthrough #2166

Conversation

sarthyparty commented Jun 20, 2024 • edited Loading

Proposed changes

Checklist

Release notes

sarthyparty commented Jun 20, 2024

kate-osborn left a comment

Choose a reason for hiding this comment

pleshakov Jun 24, 2024

Choose a reason for hiding this comment

sjberman left a comment

Choose a reason for hiding this comment

sjberman Jun 26, 2024

Choose a reason for hiding this comment

sarthyparty Jun 26, 2024

Choose a reason for hiding this comment

pleshakov commented Jun 27, 2024

kate-osborn commented Jun 27, 2024 • edited Loading

kate-osborn left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2024 • edited Loading

Codecov Report

sarthyparty commented Jun 20, 2024 •

edited

Loading

kate-osborn commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading