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

Add tests for default server #1537

Merged
merged 1 commit into from Apr 19, 2021
Merged

Conversation

pleshakov
Copy link
Contributor

Proposed changes

Adds tests for the default server for two cases:

  • when the default TLS secret is configured
  • when the default TLS secret is not configured


def assert_cn(endpoint, cn):
host = "random" # any host would work
subject_dict = get_server_certificate_subject(endpoint.public_ip, host, endpoint.port_ssl)
Copy link
Contributor

@soneillf5 soneillf5 Apr 16, 2021

Choose a reason for hiding this comment

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

Can we pass null or empty string or update the function to take the parameter optionally ? Or remove the host param if its not needed?

Copy link
Contributor Author

@pleshakov pleshakov Apr 16, 2021

Choose a reason for hiding this comment

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

while to test the default server we can use any host value, I think it's better to control that host value here in assert_cn, rather than coming up with the default value in get_server_certificate_subject. the other places we use that function (test_tls.py, test_virtual_server_tls.py and test_wildcard_tls_secret.py) all need to set a specific host header and can't rely on any default value.

the purpose of the default server is to handle requests for unknown hosts - hosts for which there is no Ingress/VS resources. So that's why random works. But it could as well be "example.com" or "1.1.1.1".

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.

LGTM

Copy link
Contributor

@vepatel vepatel left a comment

Choose a reason for hiding this comment

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

@pleshakov can we mark them as @pytest.mark.ingresses as current one will only run them in nightly!

@pleshakov pleshakov force-pushed the add-tests-for-default-server branch from f4a7fff to bbb9dea Compare April 16, 2021 22:39
@pleshakov pleshakov requested a review from vepatel April 16, 2021 22:39
@pleshakov pleshakov merged commit cd14e80 into master Apr 19, 2021
@pleshakov pleshakov deleted the add-tests-for-default-server branch April 19, 2021 15:39
@lucacome lucacome added the chore Pull requests for routine tasks label Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants