-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
|
||
def assert_cn(endpoint, cn): | ||
host = "random" # any host would work | ||
subject_dict = get_server_certificate_subject(endpoint.public_ip, host, endpoint.port_ssl) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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!
f4a7fff
to
bbb9dea
Compare
Proposed changes
Adds tests for the default server for two cases: