Skip to content

Change the path for keylog_file on http2_flow_control autest#9136

Closed
maskit wants to merge 3 commits intoapache:masterfrom
maskit:fix_autest_h2_fc
Closed

Change the path for keylog_file on http2_flow_control autest#9136
maskit wants to merge 3 commits intoapache:masterfrom
maskit:fix_autest_h2_fc

Conversation

@maskit
Copy link
Copy Markdown
Member

@maskit maskit commented Oct 11, 2022

Our test environment doesn't allow ATS to make files under Test.RunDirectory. This PR changes the path to /tmp for now, although I think Autest should provide Test.TempDirectory.

$ git grep tls_session_keys.txt
tests/gold_tests/h2/http2_flow_control.test.py: 'proxy.config.ssl.keylog_file': os.path.join(Test.RunDirectory, 'tls_session_keys.txt'),
tests/gold_tests/pluginTest/multiplexer/multiplexer.test.py: 'proxy.config.ssl.keylog_file': '/tmp/tls_session_keys.txt',

@maskit maskit added the AuTest label Oct 11, 2022
@maskit maskit added this to the 10.0.0 milestone Oct 11, 2022
@maskit maskit self-assigned this Oct 11, 2022
@maskit maskit requested a review from bneradt October 12, 2022 02:43
'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
'proxy.config.dns.nameservers': '127.0.0.1:{0}'.format(self._dns.Variables.Port),
'proxy.config.ssl.keylog_file': os.path.join(Test.RunDirectory, 'tls_session_keys.txt'),
'proxy.config.ssl.keylog_file': '/tmp/tls_session_keys.txt',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A downside to this is that it won't be in the sandbox then. CI, for instance, won't have this in the artifacts directory on failure. Can we instead put this in Traffic Server's log directory? (Like where diags.log is.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Log directory worked as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After thinking about the problem you faced some, I think it could be helpful to just have the extension create the keylog file for all TLS traffic in a consistent way. This will avoid the situation where test writers choose random locations for the keylog file (like I've clearly done so far) and sometimes choosing a location which doesn't work in some CI environments. What do you think of this?

#9137

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. The key logging is not a heavy task and the data size is small. I think we can enable it on all tests that use TLS/QUIC.

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Oct 12, 2022

[approve ci autest]

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Oct 13, 2022

The issue was resolved by #9137.

@maskit maskit closed this Oct 13, 2022
@zwoop zwoop removed this from the 10.0.0 milestone Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants