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

feat: Add BackendTLS Policy support #1487

Merged
merged 13 commits into from
Feb 15, 2024
Merged

feat: Add BackendTLS Policy support #1487

merged 13 commits into from
Feb 15, 2024

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Jan 18, 2024 •
edited

Proposed changes

Problem: As a user of NGF
I want NGF to implement the BackendTLSPolicy
So that NGF can securely connect to my pods using TLS.

Solution:

  • Add support for BackendTLSPolicy as outlined in the spec.
  • Add a new experimental features flag, so that the experimental APIs are not required by default.
  • Extend conformance tests to support installing and running against the experimental APIs

Testing: Manual testing, unit testing, conformance testing with and without experimental features enabled (NOTE: no conformance test for Backend TLS policy currently exists - that work is tracked here)

Note: When running the conformance tests with the experimental APIs installed, an unrelated test failed (HTTPRouteInvalidParentRefNotMatchingSectionName). Looks like we have been missing this failure because of a bug in the Gateway API - see https://github.com/nginxinc/nginx-gateway-fabric/actions/runs/7574208095/job/20628086048#step:17:672 for the example output. The fix is to move the check for the existence of a listener for a HTTPRoute above where we check for a port in the ParentRef.

Closes #1262

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

@ciarams87 ciarams87 requested review from a team as code owners January 18, 2024 19:41
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels Jan 18, 2024
pleshakov
pleshakov reviewed Jan 19, 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.

Please see my comments, requests and suggestions. I haven't reviewed some unit tests code because of the anticipation they might changed because of the changes to the code they test.

cmd/gateway/commands.go Outdated Show resolved Hide resolved
deploy/helm-chart/templates/rbac.yaml Show resolved Hide resolved
examples/backend-tls/README.md Outdated Show resolved Hide resolved
examples/backend-tls/gateway.yaml Outdated Show resolved Hide resolved
examples/backend-tls/policy.yaml Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration.go Outdated Show resolved Hide resolved
internal/mode/static/state/dataplane/configuration.go Outdated Show resolved Hide resolved
site/content/includes/installation/install-gateway-api-resources.md Show resolved Hide resolved
site/content/overview/gateway-api-compatibility.md Outdated Show resolved Hide resolved
site/content/includes/installation/install-gateway-api-resources.md Show resolved Hide resolved
@ciarams87 ciarams87 marked this pull request as draft January 19, 2024 17:49
ADubhlaoich
ADubhlaoich approved these changes Jan 22, 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: specific issues I would have mentioned were already flagged by @pleshakov, so I will avoid redundancy.

Is there a desire for the example to become a documentation how-to use case?

@ciarams87 ciarams87 force-pushed the feat/backend-tls-policy branch 3 times, most recently from 64cae3f to 077a917 Compare January 31, 2024 16:55
@ciarams87 ciarams87 marked this pull request as ready for review January 31, 2024 17:04
@ciarams87 ciarams87 requested review from pleshakov and a team January 31, 2024 17:04
sjberman
sjberman reviewed Jan 31, 2024
View reviewed changes
conformance/scripts/install-gateway.sh Outdated Show resolved Hide resolved
conformance/scripts/uninstall-gateway.sh Outdated Show resolved Hide resolved
examples/backend-tls/README.md Outdated Show resolved Hide resolved
internal/mode/static/build_statuses.go Outdated Show resolved Hide resolved
internal/framework/status/backend_tls.go Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Show resolved Hide resolved
internal/mode/static/state/graph/backend_tls_policy.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_tls_policy.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/config_maps.go Outdated Show resolved Hide resolved
@sjberman
Copy link
Contributor

sjberman commented Feb 2, 2024

@ADubhlaoich will probably want to re-review the new guide that was added.

pleshakov
pleshakov reviewed Feb 3, 2024
View reviewed changes
cmd/gateway/commands.go Outdated Show resolved Hide resolved
cmd/gateway/commands.go Outdated Show resolved Hide resolved
cmd/gateway/commands.go Show resolved Hide resolved
docs/developer/quickstart.md Outdated Show resolved Hide resolved
internal/mode/static/manager.go Show resolved Hide resolved
internal/mode/static/nginx/config/servers.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/generator.go Outdated Show resolved Hide resolved
internal/framework/status/setters_test.go Show resolved Hide resolved
internal/mode/static/build_statuses.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the feat/backend-tls-policy branch 2 times, most recently from b331dc4 to e7dc901 Compare February 7, 2024 22:22
ADubhlaoich
ADubhlaoich approved these changes Feb 8, 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 on the docs side of things!

@pleshakov pleshakov mentioned this pull request Feb 12, 2024
Open
pleshakov
pleshakov reviewed Feb 13, 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.

Hi @ciarams87

I added a few suggestions and requests in places where readability could be improved.
I also noticed a bug (panic)
otherwise, this looks good to me

internal/mode/static/build_statuses_test.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/servers_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/change_processor.go Show resolved Hide resolved
internal/mode/static/state/change_processor_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/conditions/conditions.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_tls_policy.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_tls_policy.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_tls_policy_test.go Show resolved Hide resolved
internal/mode/static/state/graph/graph_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph_test.go Outdated Show resolved Hide resolved
kate-osborn
kate-osborn reviewed Feb 13, 2024
View reviewed changes
deploy/helm-chart/templates/rbac.yaml Show resolved Hide resolved
cmd/gateway/commands.go Outdated Show resolved Hide resolved
cmd/gateway/commands.go Show resolved Hide resolved
@ciarams87 ciarams87 mentioned this pull request Feb 14, 2024
Open
kate-osborn
kate-osborn approved these changes Feb 14, 2024
View reviewed changes
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.

🚀

@pleshakov pleshakov mentioned this pull request Feb 14, 2024
Merged
6 tasks
@ciarams87 ciarams87 merged commit 7596bf7 into nginxinc:main Feb 15, 2024
34 checks passed
@ciarams87 ciarams87 deleted the feat/backend-tls-policy branch February 15, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjberman sjberman sjberman approved these changes

@pleshakov pleshakov pleshakov approved these changes

@ADubhlaoich ADubhlaoich ADubhlaoich approved these changes

@kate-osborn kate-osborn kate-osborn approved these changes

@bjee19 bjee19 bjee19 approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart
Projects
NGINX Gateway Fabric
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BackendTLSPolicy
6 participants
@ciarams87 @sjberman @pleshakov @ADubhlaoich @kate-osborn @bjee19