Skip to content

Add support for multiple gateways #3275

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.

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

Open
wants to merge 17 commits into
base: change/control-data-plane-split
Choose a base branch
from

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Apr 1, 2025

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: Users want to be able to configure multiple gateways

Solution: Allows users to configure multiple gateways

Testing:

  1. Added unit tests and fixed functional tests

Manual testing: tracking it here

Closes #3219
Closes #3213
Closes #1443

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.

Add support for multiple gateways

@github-actions github-actions bot added enhancement New feature or request tests Pull requests that update tests labels Apr 1, 2025
@salonichf5 salonichf5 changed the title Add support for multiple gateways Do Not Review: Add support for multiple gateways Apr 1, 2025
@salonichf5 salonichf5 force-pushed the feat/multiple-gateways branch from 8b7192b to 33a4cb7 Compare April 2, 2025 16:11
@salonichf5 salonichf5 force-pushed the feat/multiple-gateways branch 2 times, most recently from 2e13294 to efd9397 Compare April 7, 2025 09:11
@sjberman sjberman force-pushed the feat/multiple-gateways branch 2 times, most recently from 61c9184 to 896523a Compare April 14, 2025 19:47
@sjberman sjberman changed the title Do Not Review: Add support for multiple gateways Add support for multiple gateways Apr 14, 2025
@sjberman sjberman force-pushed the feat/multiple-gateways branch from 896523a to 0ff8cfa Compare April 14, 2025 19:55
@sjberman sjberman marked this pull request as ready for review April 14, 2025 19:55
@sjberman sjberman requested a review from a team as a code owner April 14, 2025 19:55
@salonichf5 salonichf5 force-pushed the feat/multiple-gateways branch from c1dd270 to d4dcee1 Compare April 15, 2025 20:34
@sjberman sjberman force-pushed the feat/multiple-gateways branch from d4dcee1 to c1dd270 Compare April 15, 2025 20:38
@nginx nginx deleted a comment from codecov bot Apr 17, 2025
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 94.44444% with 35 lines in your changes missing coverage. Please review.

Project coverage is 86.72%. Comparing base (6337c97) to head (8713cd6).
Report is 185 commits behind head on change/control-data-plane-split.

Files with missing lines Patch % Lines
internal/mode/static/manager.go 6.66% 14 Missing ⚠️
internal/mode/static/handler.go 88.70% 4 Missing and 3 partials ⚠️
cmd/gateway/commands.go 14.28% 6 Missing ⚠️
...ernal/mode/static/state/dataplane/configuration.go 96.87% 2 Missing and 1 partial ⚠️
...nal/mode/static/nginx/config/policies/validator.go 77.77% 1 Missing and 1 partial ⚠️
internal/mode/static/state/graph/route_common.go 98.24% 1 Missing and 1 partial ⚠️
...rnal/mode/static/state/graph/backend_tls_policy.go 95.83% 1 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                         @@
##           change/control-data-plane-split    #3275      +/-   ##
===================================================================
- Coverage                            89.74%   86.72%   -3.02%     
===================================================================
  Files                                  109      125      +16     
  Lines                                11150    14445    +3295     
  Branches                                50       62      +12     
===================================================================
+ Hits                                 10007    12528    +2521     
- Misses                                1083     1780     +697     
- Partials                                60      137      +77     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@@ -301,7 +310,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
Namespace: "test",
},
Spec: v1alpha3.BackendTLSPolicySpec{
TargetRefs: targetRefNormalCase,
TargetRefs: targetRefInvalidKind,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this change not affect the outcome of the test?

Copy link
Contributor Author

@salonichf5 salonichf5 Apr 17, 2025

Choose a reason for hiding this comment

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

I have no idea how this test was passing before, this is a valid case where type is invalid, the normal case targetRef kind was secret which was the allowed type for cert ref kind.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a bug? Could be worth an investigation (out of scope for this, but could make a note of it to revisit after this is done).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, let me make a note of it!

Comment on lines +3681 to +3688
func TestBindRoutesToListeners(t *testing.T) {
t.Parallel()
g := NewWithT(t)

g.Expect(func() {
bindRoutesToListeners(nil, nil, nil, nil)
}).ToNot(Panic())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this test case really needed? I know you're probably just trying to get coverage on the panic line, but is it even possible for us to call it like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is possible to call it like this. This was just for coverage and also for the fact that we do have events with no gateways to update so wanted to be cautious

Copy link
Collaborator

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

LGTM, nice work.

I think once we get conformance tests working and we can run our full suite of all tests, we can get a better feeling of any issues that may have arisen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release-notes tests Pull requests that update tests
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

2 participants