Egress via Ingress VirtualServer Resource #3491

chase-kiefer · 2023-01-26T22:42:50Z

Proposed changes

NSM would like the ability to egress traffic through a KIC VirtualServer. This PR adds the functionality and templating necessary to configure a VS resource for NSM egress. It diverges from the pattern implemented for ingress by using a CRD field instead of an annotation.

added internalRoute field to the virtualserver CRD
updated templates for internal routes in virtualserver for n+ and oss
added unit test to validate virtual server internal routes
updated virtualServerConfigurator type to have an enableInternalRoutes boolean
updated virtualserver configuration items to include internRoute docs

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

chase-kiefer · 2023-01-26T22:45:58Z

@nginxinc/service-mesh Looking for a review from someone on our team also. I can't add us as reviewers, so just adding a comment tagging our team.

codecov-commenter · 2023-01-26T22:49:09Z

Codecov Report

Merging #3491 (ae6aa50) into main (96d28b2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3491      +/-   ##
==========================================
+ Coverage   52.34%   52.36%   +0.01%     
==========================================
  Files          59       59              
  Lines       16880    16890      +10     
==========================================
+ Hits         8836     8844       +8     
- Misses       7747     7749       +2     
  Partials      297      297

Impacted Files	Coverage Δ
internal/configs/version2/http.go	`0.00% <ø> (ø)`
internal/configs/virtualserver.go	`95.16% <100.00%> (+0.02%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

internal/configs/annotations.go

internal/configs/version2/nginx-plus.virtualserver.tmpl

internal/configs/version2/nginx.virtualserver.tmpl

jbyers19

LGTM

internal/configs/version2/nginx.virtualserver.tmpl

internal/configs/virtualserver_test.go

sjberman

LGTM, be sure to update commit messages and PR description with the new approach.

deployments/common/crds/k8s.nginx.org_virtualservers.yaml

deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml

…resource - added internalRoute field to the virtualserver CRD - added templates for internal routes in virtualserver templates for n+ and oss - added unit test to validate virtualserver internal routes - added enableInternalRoutes boolean to virtualServerConfigurator type - updated virtualserver configuration items to include internRoute docs

internal/configs/virtualserver_test.go

ciarams87

Thanks so much @chase-kiefer ! Great work 😄

internal/configs/version2/nginx-plus.virtualserver.tmpl

internal/configs/virtualserver.go

lucacome · 2023-03-21T05:35:48Z

@chase-kiefer I think we need a couple of changes:

we're missing the check for the CLI argument enable-internal-routes, if the flag is not set and we add internalRoute in the VirtualServer we should throw a warning and not write the config for it.
we should align the logic and variables with the Ingress side to make it easier to maintain the code. Right now in the template we have InternalRouteServer for the Spiffe server certs and SpiffeCerts for the client certs, but in Ingress we use SpiffeCerts for the server certs and SpiffeClientCerts for the client certs. I'm fine with whatever names you think make the most sense, I'd just like to have them consistent. Maybe for the sake of merging this PR, you can just use the same logic of Ingress and then refactor for both later. Up to you.

Also #3602 might remove the CLI arguments, so we might need to change the logic and look at the annotation instead...

- Add warning to catch cases where a virtual server internal route should not be created - Switch variable names to match ingress naming scheme

for more information, see https://pre-commit.ci

internal/configs/virtualserver.go

…outes

* Add ability for nginx service mesh to egress through a virtualserver resource - added internalRoute field to the virtualserver CRD - added templates for internal routes in virtualserver templates for n+ and oss - added unit test to validate virtualserver internal routes - added enableInternalRoutes boolean to virtualServerConfigurator type - updated virtualserver configuration items to include internRoute docs * Add a description for the InternalRoute field in the VS CRD * Add test case for nsmEgress being true in TestIsTLSEnabled * Update the isTLSEnabled function for clarity * Reverse function params for isTLSEnabled * Add virtual server internal route validation and warning - Add warning to catch cases where a virtual server internal route should not be created - Switch variable names to match ingress naming scheme * Add refactored VS templates to avoid duplicate listen blocks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add conditional to prevent SpiffeClientCerts being set for internal routes * Fix unit tests --------- Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Tomás Ó hAodha <86358393+tomasohaodha@users.noreply.github.com> Co-authored-by: Venktesh Shivam Patel <ve.patel@f5.com> (cherry picked from commit 36ac2ef)

* Add ability for nginx service mesh to egress through a virtualserver resource - added internalRoute field to the virtualserver CRD - added templates for internal routes in virtualserver templates for n+ and oss - added unit test to validate virtualserver internal routes - added enableInternalRoutes boolean to virtualServerConfigurator type - updated virtualserver configuration items to include internRoute docs * Add a description for the InternalRoute field in the VS CRD * Add test case for nsmEgress being true in TestIsTLSEnabled * Update the isTLSEnabled function for clarity * Reverse function params for isTLSEnabled * Add virtual server internal route validation and warning - Add warning to catch cases where a virtual server internal route should not be created - Switch variable names to match ingress naming scheme * Add refactored VS templates to avoid duplicate listen blocks * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add conditional to prevent SpiffeClientCerts being set for internal routes * Fix unit tests --------- Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Tomás Ó hAodha <86358393+tomasohaodha@users.noreply.github.com> Co-authored-by: Venktesh Shivam Patel <ve.patel@f5.com> (cherry picked from commit 36ac2ef) Co-authored-by: Chase Kiefer <112438922+chase-kiefer@users.noreply.github.com>

chase-kiefer requested a review from a team as a code owner January 26, 2023 22:42

chase-kiefer requested review from ciarams87, jjngx and shaun-nx January 26, 2023 22:42

chase-kiefer force-pushed the nsm-virtualserver-egress branch from c64939f to 570c589 Compare January 26, 2023 22:48

sjberman reviewed Jan 27, 2023

View reviewed changes

internal/configs/annotations.go Outdated Show resolved Hide resolved

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved

internal/configs/version2/nginx.virtualserver.tmpl Outdated Show resolved Hide resolved

jbyers19 approved these changes Jan 27, 2023

View reviewed changes

chase-kiefer force-pushed the nsm-virtualserver-egress branch 2 times, most recently from 0f677bc to 54e2ad0 Compare January 30, 2023 17:43

sjberman approved these changes Jan 30, 2023

View reviewed changes

ciarams87 reviewed Feb 2, 2023

View reviewed changes

internal/configs/version2/nginx.virtualserver.tmpl Outdated Show resolved Hide resolved

ciarams87 reviewed Feb 2, 2023

View reviewed changes

internal/configs/version2/nginx.virtualserver.tmpl Outdated Show resolved Hide resolved

brianehlert reviewed Feb 8, 2023

View reviewed changes

internal/configs/virtualserver_test.go Outdated Show resolved Hide resolved

github-actions bot added the helm_chart Pull requests that update the Helm Chart label Feb 27, 2023

sjberman reviewed Feb 27, 2023

View reviewed changes

deployments/common/crds/k8s.nginx.org_virtualservers.yaml Show resolved Hide resolved

deployments/helm-chart/crds/k8s.nginx.org_virtualservers.yaml Show resolved Hide resolved

github-actions bot added the documentation Pull requests/issues for documentation label Feb 27, 2023

chase-kiefer force-pushed the nsm-virtualserver-egress branch from 9222a59 to 7d1ca30 Compare February 27, 2023 21:06

Add a description for the InternalRoute field in the VS CRD

0ba0623

ciarams87 reviewed Mar 7, 2023

View reviewed changes

internal/configs/virtualserver_test.go Show resolved Hide resolved

Add test case for nsmEgress being true in TestIsTLSEnabled

a9f6424

ciarams87 approved these changes Mar 9, 2023

View reviewed changes

Merge branch 'main' into nsm-virtualserver-egress

4008dce

ciarams87 enabled auto-merge (squash) March 9, 2023 16:45

Merge branch 'main' into nsm-virtualserver-egress

8d32c11

lucacome reviewed Mar 10, 2023

View reviewed changes

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved

internal/configs/virtualserver.go Outdated Show resolved Hide resolved

Update the isTLSEnabled function for clarity

2328523

auto-merge was automatically disabled March 10, 2023 17:40
Head branch was pushed to by a user without write access

chase-kiefer and others added 2 commits March 10, 2023 11:51

Reverse function params for isTLSEnabled

5f968d6

Merge branch 'main' into nsm-virtualserver-egress

a9a1e87

Add virtual server internal route validation and warning

43ce778

- Add warning to catch cases where a virtual server internal route should not be created - Switch variable names to match ingress naming scheme

chase-kiefer requested review from lucacome and removed request for jjngx and shaun-nx March 21, 2023 20:05

chase-kiefer and others added 2 commits March 28, 2023 09:38

Add refactored VS templates to avoid duplicate listen blocks

9dc96f5

[pre-commit.ci] auto fixes from pre-commit.com hooks

458584c

for more information, see https://pre-commit.ci

ciarams87 self-requested a review March 29, 2023 14:33

Merge branch 'main' into nsm-virtualserver-egress

57aa6ab

ciarams87 requested a review from vepatel March 30, 2023 16:01

ciarams87 approved these changes Mar 31, 2023

View reviewed changes

Merge branch 'main' into nsm-virtualserver-egress

3362bab

lucacome reviewed Apr 1, 2023

View reviewed changes

internal/configs/virtualserver.go Outdated Show resolved Hide resolved

vepatel approved these changes Apr 5, 2023

View reviewed changes

chase-kiefer and others added 6 commits April 5, 2023 15:29

Add conditional to prevent SpiffeClientCerts being set for internal r…

84f9d46

…outes

Fix unit tests

c5b4717

Merge branch 'main' into nsm-virtualserver-egress

4113a58

Merge branch 'main' into nsm-virtualserver-egress

d30cdee

Merge branch 'main' into nsm-virtualserver-egress

7b216cf

Merge branch 'main' into nsm-virtualserver-egress

ae6aa50

tomasohaodha merged commit 36ac2ef into nginxinc:main Apr 14, 2023
59 checks passed

chase-kiefer mentioned this pull request Apr 14, 2023

Revert "docs: add documentation for egress via VirtualServer" nginxinc/nginx-service-mesh#278

Merged

brianehlert added this to the v3.1.1 milestone Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egress via Ingress VirtualServer Resource #3491

Egress via Ingress VirtualServer Resource #3491

chase-kiefer commented Jan 26, 2023 •

edited

chase-kiefer commented Jan 26, 2023 •

edited

codecov-commenter commented Jan 26, 2023 •

edited by codecov bot

jbyers19 left a comment

sjberman left a comment

ciarams87 left a comment

lucacome commented Mar 21, 2023

Egress via Ingress VirtualServer Resource #3491

Egress via Ingress VirtualServer Resource #3491

Conversation

chase-kiefer commented Jan 26, 2023 • edited

Proposed changes

Checklist

chase-kiefer commented Jan 26, 2023 • edited

codecov-commenter commented Jan 26, 2023 • edited by codecov bot

Codecov Report

jbyers19 left a comment

Choose a reason for hiding this comment

sjberman left a comment

Choose a reason for hiding this comment

ciarams87 left a comment

Choose a reason for hiding this comment

lucacome commented Mar 21, 2023

chase-kiefer commented Jan 26, 2023 •

edited

chase-kiefer commented Jan 26, 2023 •

edited

codecov-commenter commented Jan 26, 2023 •

edited by codecov bot