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.

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

Support match in TransportServer health checks #1633

Merged
merged 3 commits into from
Jun 2, 2021
Merged

Conversation

pleshakov
Copy link
Contributor

Proposed changes

Support match in TransportServer health checks
Based on the original work from @soneillf5

@github-actions github-actions bot added documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels May 26, 2021
Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

Can you add a test to the python test suite that confirms the functionality of this change?

@pleshakov pleshakov requested a review from soneillf5 May 27, 2021 19:50
@soneillf5 soneillf5 self-requested a review May 31, 2021 08:25
Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

I need to leave a comment to request changes.

@pleshakov pleshakov requested a review from vepatel June 1, 2021 04:36
@@ -407,3 +408,121 @@ def test_tcp_request_load_balanced_method(
client.close()

assert len(endpoints) is 3

@pytest.mark.skip_for_nginx_oss
def test_tcp_passing_healthcheck_with_match(
Copy link
Contributor

Choose a reason for hiding this comment

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

@pleshakov these tests also looks like a decent candidates to use parameterization to pass various ts/cases, see: https://github.com/nginxinc/kubernetes-ingress/blob/master/tests/suite/test_ingress_mtls.py#L82

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's fine for now that its added and working but we don't need to verify all the attribs as nginx plus is doing lb and the UI is provided by the same.

@@ -526,10 +526,13 @@ func TestGenerateUnixSocket(t *testing.T) {

func TestGenerateTransportServerHealthChecks(t *testing.T) {
upstreamName := "dns-tcp"
generatedUpsteamName := "ts_namespace_name_dns-tcp"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there one for dns-udp too?

@pleshakov pleshakov merged commit 89ba98e into master Jun 2, 2021
@pleshakov pleshakov deleted the feature/ts-hc-match branch June 2, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants