-
Notifications
You must be signed in to change notification settings - Fork 2
Beakerlib fix #8
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
Conversation
.travis.yml
Outdated
@@ -3,17 +3,17 @@ sudo: required | |||
env: | |||
matrix: | |||
# openssl tests | |||
- COMP=openssl OS_TYPE=centos OS_VERSION=5 | |||
#- COMP=openssl OS_TYPE=centos OS_VERSION=5 | |||
- COMP=openssl OS_TYPE=centos OS_VERSION=6 | |||
- COMP=openssl OS_TYPE=centos OS_VERSION=7 | |||
- COMP=openssl OS_TYPE=fedora OS_VERSION=24 |
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.
Regarding the recent release of Fedora 25: do you prefer explicit version assignment (24, 25, etc.) or the latest
tag would be sufficient? With the latest
tag we would avoid bumping the version every ~6 months (source).
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.
latest
sounds like a good choice considering fast release cycle of Fedora. But we should consider running the tests on other supported Fedora versions, which would require bumping anyway..
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.
Running tests on all supported Fedora versions definitely sounds like a good idea, but it also brings several issues, which need to be addressed (like nss in F25 not supporting SSLv3, etc.). Phase relevancy for specific Fedora releases will be necessary in these cases.
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.
in general, for Fedora, we care only about latest
I think it's a bit too early to say and we have too little resources to run on multiple Fedora versions, the probability of regressions is highest in newest versions anyway
scripts/test-runner.sh
Outdated
@@ -47,8 +47,13 @@ if [[ $OS_TYPE != "fedora" ]]; then | |||
$PKG_MAN -y install epel-release | |||
fi | |||
|
|||
# TEMPORARY WORKAROUND | |||
# TODO: Remove when a fixed official beakerlib version is released | |||
yum -y install https://sumsal.cz/dev/beakerlib-1.11centos-99.fc23.noarch.rpm |
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.
I don't like dependency on external infrastructure, even temporary
aren't we using it just for one little function? couldn't we copy-paste it instead of that?
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.
Even though we use only a one little function, which consist of 4 lines, there are several other patches, which need to be applied, so this function could work properly. I hate this solution as much as you, but without that, we have several other not-so-nice solutions:
- Store the patches in this repo and apply them before testing
- Store the built RPM in the repo instead of somewhere on an external infrastructure
As much as I'd like to avoid any of these options, we'll have to pick one of them, otherwise the incredibly slow release cycle of beakerlib would block our progress.
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.
Hm, I just noticed, that this fix is included in the testing version of beakerlib (Fedora rawhide & epel-testing), so we could finally get rid of this workaround. Will look into that.
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.
After a few hours of experimenting and debugging I have good news and bad news.
The good news is that for installing the latest version of beakerlib we can use repo epel-testing
on CentOS and updates-testing
on Fedora. Both repos contain the fixed version of beakerlib.
The bad news is that the new version fixes a bug in rlWaitForSocket
function. This function now actually waits for socket to close when --close
is specified. This would be great, but the way how it checks the socket is horrible:
local cmd="netstat -nla | grep -E '$grep_opt' >/dev/null"
This thing right here checks if the socket/port is listed in netstat's output, but this includes remote ports along with local ones, which is a problem. Many interoperability tests kill the server part, wait for its PID to disappear and then wait for the socket to close. But the nestat's output in this case contains client's socket as well. This thing worked before, because it grep'd only sockets in LISTEN
state. Now it greps all sockets, even those in TIME_WAIT
state, which is relevant for this case.
Long story short, each rlRun "rlWaitForSocket 4433 --close"
now takes around 50 seconds to complete, because of this:
# netstat -nla
Active Internet connections (servers and established)
Proto Recv-Q Send-Q Local Address Foreign Address State
...
tcp6 0 0 ::1:56226 ::1:4433 TIME_WAIT
This means, that many interoperability test now take several to dozens of hours to complete, which is totally useless, especially in Travis. Fix for this issue is not complicated, but it can take another several months to propagate to testing repos. @tomato42, @ep69 any ideas?
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.
@mrc0mmand I've just communicated with our beakerlib maintainer (dapospis) and he told me the -a argument was added there to address https://bugzilla.redhat.com/show_bug.cgi?id=1388422 . If you would be able to come up with a fix that satisfies needs of both the mentioned bugzilla and yours, we can get it to testing beakerlib in a matter of hours.
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.
yes, this is not a good change to beakerlib, just commented on the BZ
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.
Following Dalibor's recommendation, I filed a new bug where the progress on this issue can be tracked: BZ#1416014.
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.
so, can we drop that change from this PR?
Regarding the conversations in this PR, I made following changes:
As mentioned in one of the conversation threads, we're now using the testing version of beakerlib (updates-testing on Fedora, epel-testing on CentOS). Even though the testing sounds scary here, it'll ensure faster fixes for upcoming issues and it just works™. Edit: Fedora 25 introduces a new issue - unsupported SSLv3 in NSS. I guess simple conditions in affected tests should be enough to mitigate this. |
yes, testing if the code is not running on RHEL 5, 6 or 7 and expecting SSLv3 to fail is the correct solution |
The Travis fails are now caused by two issues:
Apart from that I'd say this branch is ready to merge. Then I'd continue with rebasing #5, fix its remaining issues with expect and merge it as well. |
why we can't merge this PR with changes in #9 what's the point of CI if we merge known broken commits? If it takes more changes to keep Travis passing, then lets put more change in single bucket. |
It's because the failing part of the gnutls/Interoperability/renegotiation-with-NSS was rewritten in #9, thus by fixing this issue in this PR we'd create a merge conflict. Edit: maybe we could disable that test case in this PR to get rid of a known fail |
ok, then let's disable it in this PR and reenable it in #9 |
So, after fixing the last blocking issues, I discovered another two. I've already fixed the first one in my testing branch (replacing 'latest' OS tag with the actual OS version), but the second one is giving me a headache. These ciphersuites are failing on F25 when used with NSS:
After going through the supported ciphersuites list provided by NSS listsuites utility, I found out[0] that
Explicitly enabling SSLv3 yields expected results:
Using selfserv with gnutls-cli (and the same settings) works as intended. Is this intentional or is it some strangeness/bug in tstclnt/strsclnt? [0]
|
No, that's a bug. The RC4 is disabled through Crypto Policy. If it affects only the client side then it's a bug how Crypto Policy is handled. |
Thanks for pointing out the crypto policies, it looks like that's the culprit. As can be seen below, this issue 'fixes' itself, when a F24 version of the crypto-policies package is used: Fedora 25
Fedora 25, crypto-policies from Fedora 24:
The F24 version of crypto-policies doesn't even contain a policy for NSS, so that may have some effect on this issue. Also, trying the scenario above with the latest version of crypto policies (rawhide) yields the same results as with the F25 package. |
Also, the upstream version (3.30 Beta) has the same behavior as the 3.28.1 with older crypto-policies package, so I guess the handling of Crypto Policies has been fixed there. |
Please rebase to get rid of the merge commit I'm assuming that you'll continue to work on this to fix the errors reported by travis, won't you? |
d82f6b1
to
9638b90
Compare
Rebased. And yes, I already included two fixes which should resolve some of the reported errors (393fa42 and 9638b90). |
The (hopefully) last remaining issues:
This could be potentially fixed by tweaking the
|
please do, and comment it out with the link to that bug as a reason for waiving
yes, it does look like this |
42b1888
to
2a4c4ce
Compare
scripts/test-setup.sh
Outdated
FAILED_CHECKS=0 | ||
FAILED_NAMES=() | ||
while read file; do | ||
if ! grep -Pzoq "rlGetTestState.*$" "$file"; then |
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.
why -z
option to grep? isn't -o
useless with -q
?
I don't know the Perl regular expressions, but usually $
specifies end of line, not end of file, and .*
won't match newlines, so I'm not exactly sure that this makes sure that the rlGetTestState
is "at their end"...
scripts/test-setup.sh
Outdated
@@ -17,7 +18,7 @@ CERTGEN_PATH="openssl/Library/certgen" | |||
FAILED_CHECKS=0 | |||
FAILED_NAMES=() | |||
while read file; do | |||
if ! grep -Pzoq "rlGetTestState.*$" "$file"; then | |||
if ! grep -Pzoq "rlGetTestState[[:space:]]*$" "$file"; then |
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.
that change should be in commit 31dcda2
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.
please state in 393fa4275129d03969514 why those tests are failing
# version tag with the actual OS version | ||
# So far the 'latest' tag is used only for Fedora | ||
if [[ $OS_VERSION == "latest" ]]; then | ||
OS_VERSION="$(awk '{ print $3; }' < /etc/fedora-release)" |
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.
that change should in the same commit as the change to .travis.yml
i=$(($i+1)) | ||
# | ||
# Crypto Policy handling seems to be broken in NSS on F25 for RC4 | ||
# BZ#1426267 |
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 it is broken, I don't think it is broken in a way that we can't enable those ciphers, is it?
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.
I meant, can't we make NSS on Fedora enable those ciphers? i.e. override the Crypto Policy?
The idea of those tests it to verify that those ciphers are implemented the same way in both libraries under test, not that the sets of disabled ciphers/protocols are the same...
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.
Fair point. This can be easily done by enforcing NSS to ignore the crypto policies entirely by setting the NSS_IGNORE_SYSTEM_POLICY
env variable to 1.
01cf1f8
to
a249677
Compare
Thanks for the feedback. I have updated the commits as follows:
Regarding the last requested change - I disabled the problematic ciphersuites in 38e3a79 because they were causing Travis fails, and I thought we want to merge this request when, and only when, it is passing. Also, the mentioned ciphersuites are disabled only on Fedora 25 (and above). I guess I could drop the |
scripts/test-setup.sh
Outdated
FAILED_CHECKS=0 | ||
FAILED_NAMES=() | ||
while read file; do | ||
if ! grep -Pzq "rlGetTestState[[:space:]]*$" "$file"; then |
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.
why -z
option is used?
won't it also match the case when rlGetTestState
is used inside rlRun
, or in general, inside phase?
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.
No, -z
replaces all newlines with a NUL character (\0
), so the $
anchor in this case matches the EOF.
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.
$ echo -e "rlGetTestState\nrlRun 'echo'" | grep -Pzq 'rlGetTestState[[:space:]]*$'
grep: unescaped ^ or $ not supported with -Pz
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.
Hm, that's interesting, I've never seen this error on any distro I tested this on. However, it returns 0 even in case when it should fail (on RHEL/CentOS 6.8+ and 7.3+):
E.g.:
# cat /etc/redhat-release
Red Hat Enterprise Linux Server release 6.8 (Santiago)
# echo -e "rlGetTestState\nrlRun 'echo'" | grep -Pzq 'rlGetTestState[[:space:]]*$'; echo $?
0
Nevertheless, it can be easily fixed by replacing the $
anchor with \z
, which matches the end of the input string, ignoring all line breaks (-z
is still necessary to avoid reading input line by line).
# cat /etc/redhat-release
Red Hat Enterprise Linux Server release 6.8 (Santiago)
# echo -e "rlGetTestState\nrlRun 'echo'" | grep -Pzq 'rlGetTestState[[:space:]]*\z'; echo $?
1
# echo -e "rlGetTestState\n\n \t\t \n" | grep -Pzq 'rlGetTestState[[:space:]]*\z'; echo $?
0
Same results apply for RHEL/CentOS 7.3 and Fedora 25.
Thanks.
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.
yes, \z
works, thanks!
i=$(($i+1)) | ||
# | ||
# Crypto Policy handling seems to be broken in NSS on F25 for RC4 | ||
# BZ#1426267 |
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.
I meant, can't we make NSS on Fedora enable those ciphers? i.e. override the Crypto Policy?
The idea of those tests it to verify that those ciphers are implemented the same way in both libraries under test, not that the sets of disabled ciphers/protocols are the same...
Check if all tests have rlGetTestState command to prevent false-positives
Centos 6 - unsupported Centos 7 - there is a bug, which was fixed in RHEL/Centos 7.3 => re-enable this test when 7.3 docker image is available
CentOS 5 is incompatible with the beakerlib workaround. Also, no tests were running on RHEL/CentOS 5 anyway (which can change in the future).
All disabled test by this commit are going to be updated in the near future.
This is a temporary workaround for BZ#1426267.
Currently released version of beakerlib doesn't contain an equivalent for function
rlIsRHEL
on CentOS. This causes unexpected fails on CentOS when this function is used for relevancy. I discussed this with beakerlib devels and the fix should be a part of the next release. Until then, I created a custom RPM with necessary patches, which can be used as a temporary workaround (this RPM is based on the released stable codebase).To avoid mass-editing all tests to support
rlIsRHEL
along withrlIsCentOS
, thetest-runner.sh
script adds a new function to the beakerlib library, which simply callsrlIsCentOS
whenrlIsRHEL
is called (CentOS-exclusive).This PR also disables CentOS 5 jobs, as they are slightly incompatible with this workaround (and these jobs were unused by tests so far).
If everything is all right, I'll rebase all branches of the remaining PRs, so we can get relevant test results.
Relevant bugs: BZ#1214190