fips_enabled is either 1 or 0, but we return a string. So here it makes more sense to return something more meaningful like "Not supported by the platform" then just 'false'.

There are only two places the procedure is used in. What if we changed this to return True/False/None and fixed the expected output in the two places it is used?

Additionally, maybe I'd better use try: ... except FileNotFoundError: return None.

@vashirov You'll need to pattern match that somehow like:

if str == '1':
return True
else:
return False

I can update the patch with this.

@mhonek Exceptions are not logic. We should check explicitly, so we have neat flow control rather than a nested logic series. So I think I'll leave it like this. Other examples could be not just os.path.exists, but permission too (think selinux, containers, etc).

Better put, we want exceptions to be thrown to highlight real errors, rather than catch/reraise or using them as logic. We want our code to be explicit where we can, and fragile intentionally to highlight our assumptions.

Better put, we want exceptions to be thrown to highlight real errors, rather than catch/reraise or using them as logic. We want our code to be explicit where we can, and fragile intentionally to highlight our assumptions.

I don't see any contradiction with Matus's logic regarding "try: ... except FileNotFoundError: return None".
It is a common Python way to handle something that expected. Try to open and if the file is not found - stop and return None. If some other issue happens - raise the error.
I think this way of thinking was the main reason we got PEP-3151:
https://docs.python.org/3/whatsnew/3.3.html#pep-3151-reworking-the-os-and-io-exception-hierarchy

IMO, both ways (this and yours) are explicit and readable enough. So I don't mind if any of them will be used.

On Mon, 2018-08-06 at 22:23 +0000, Simon Pichugin wrote:

spichugi commented on the pull-request: 49885 - On some platform fips does not exist that you are following:
``

Better put, we want exceptions to be thrown to highlight real
errors, rather than catch/reraise or using them as logic. We want
our code to be explicit where we can, and fragile intentionally to
highlight our assumptions.

I don't see any contradiction with Matus's logic regarding "try: ...
except FileNotFoundError: return None".
It is a common Python way to handle something that expected. Try to
open and if the file is not found - stop and return None. If some
other issue happens - raise the error.

But we don't want to return None: we want to return False. So now we
have exceptions causing logic flow, which is not the right way to use
them.

I think this way of thinking was the main reason we got PEP-3151:

https://docs.python.org/3/whatsnew/3.3.html#pep-3151-reworking-the-os-and-io-exception-hierarchy

I think that's more about making exceptions communicative instead of
just "IOError", but that's my view point :)

IMO, both ways (this and yours) are explicit and readable enough. So
I don't mind if any of them will be used.

So .... ack?

``

To reply, visit the link below or just reply to this email
https://pagure.io/389-ds-base/pull-request/49886
--
Sincerely,

William

Nack, this doesn't work:

py.test-3 conftest.py 
============================= test session starts ==============================
platform linux -- Python 3.6.6, pytest-3.4.2, py-1.5.4, pluggy-0.6.0
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/_pytest/main.py", line 98, in wrap_session
INTERNALERROR>     config.hook.pytest_sessionstart(session=session)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 617, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 201, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/_pytest/terminal.py", line 399, in pytest_sessionstart
INTERNALERROR>     config=self.config, startdir=self.startdir)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 617, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 222, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/__init__.py", line 216, in <lambda>
INTERNALERROR>     firstresult=hook.spec_opts.get('firstresult'),
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 201, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 76, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/usr/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/workspace/ds/dirsrvtests/conftest.py", line 48, in pytest_report_header
INTERNALERROR>     header += "FIPS: " + is_fips()
INTERNALERROR> TypeError: must be str, not bool

Metadata for the report header should be string.

I want to clarify: the value printed in the header should be human-readable. This is intended for people who would read these test reports. 1 and 0 were good enough, since it's what was in /proc/sys/crypto/fips_enabled. But if we're covering a case, where FIPS is not available, it should be stated explicitly in the report, not an empty value.

Nack, this doesn't work:

Metadata for the report header should be string.

No. You should use string formatting not construction. There is a good reason why "%s" is how I build strings in python . I have updated the patch to correct this and see correctly:

platform linux -- Python 3.6.5, pytest-3.7.1, py-1.5.4, pluggy-0.7.1 -- /usr/bin/python3
cachedir: .pytest_cache
389-ds-base: not installed
nss: not installed
nspr: not installed
openldap: not installed
cyrus-sasl: 2.1.26-17.2
FIPS: False
rootdir: /home/william/development/389ds, inifile:

Ohhh, PS: we probably shouldn't be hardcoding assumptions that we are on an RPM system because we are now excluding our friends in Debian and Ubuntu from running these tests. This also fails on openSUSE to asserts the correct RPM versions. But that's another bug ....

But if we're covering a case, where FIPS is not available, it should be stated explicitly in the report, not an empty value.

There is also no difference between a "not available' and ... not available IE the /proc files don't exist. They both are "FIPS is not on this platform". That's why I've left it as False for both.

No. You should use string formatting not construction. There is a good reason why "%s" is how I build strings in python .

There is a good reason why the return type of the function was a string in the first place. But you had your own assumptions and your patch didn't work.

I have updated the patch to correct this and see correctly:

Thanks, I can confirm that it no longer fails with INTERNALERROR.

Ohhh, PS: we probably shouldn't be hardcoding assumptions that we are on an RPM system because we are now excluding our friends in Debian and Ubuntu from running these tests. This also fails on openSUSE to asserts the correct RPM versions. But that's another bug ....

How we are excluding from running tests exactly? On non-rpm systems tests can be run, this information is in the report only and doesn't affect test execution. The tests will fail though, because of other incompatibilities and hard coded values in lib389.

There is also no difference between a "not available' and ... not available IE the /proc files don't exist. They both are "FIPS is not on this platform". That's why I've left it as False for both.

There is no difference for whom? To me, as a test results reviewer, there is. Platform with disabled FIPS and FIPS-not-available-at-all are completely different things. Information that is printed here is for humans, not for the code that will run differently depending on FIPS state.
So please cover 3 cases:
FIPS is available on the platform and enabled
FIPS is available on the platform and disabled
FIPS is not available on the platform

How we are excluding from running tests exactly? On non-rpm systems
tests can be run, this information is in the report only and
doesn't affect test execution. The tests will fail though, because of
other incompatibilities and hard coded values in lib389.

I didn't say "excluding from running", I said "excluding". Treating
them as an afterthought. I think we would (and should) think better
about how we treat non-RH distros in the future, and my note here is
for us to start thinking about it. The fact that this issue exists is a
reflection of that because FIPS is not available on other systems.

There is no difference for whom? To me, as a test results reviewer,
there is. Platform with disabled FIPS and FIPS-not-available-at-all
are completely different things. Information that is printed here is
for humans, not for the code that will run differently depending on
FIPS state.
So please cover 3 cases:
FIPS is available on the platform and enabled
FIPS is available on the platform and disabled
FIPS is not available on the platform

FIPS not available or FIPS disabled are the same thing. It's the
abscence of FIPS. The only state's that matter are "enabled" or "not".
That's why I used bool, not an enum here.

Flipping that, all you care about is if FIPS is enabled, because then
it's likely to break everything ;)

We have no tests that rely on the state of disabled vs not avail, so
the distinction is not necessary.

--
Sincerely,

William

Ohh, and if I haven't mentioned, this is blocking tests running on all
non-RH platforms, so please can this just be reviewed and done with?
It's now preventing me from contributing to the project :(

I didn't say "excluding from running", I said "excluding". Treating
them as an afterthought. I think we would (and should) think better
about how we treat non-RH distros in the future, and my note here is
for us to start thinking about it. The fact that this issue exists is a
reflection of that because FIPS is not available on other systems.

I should've put emphasis on running. I totally agree with you about testing on other systems. But this issue is only one small problem, there are a lot of assumptions in lib389 about the underlying system, that excludes from testing. If you try to run this on Debian/Ubuntu, you would see what I'm talking about.

FIPS not available or FIPS disabled are the same thing. It's the
abscence of FIPS. The only state's that matter are "enabled" or "not".
That's why I used bool, not an enum here.

No. If there is an absence of FIPS, that means I can't turn it on or off in tests. And if I see that FIPS is absent on a platform where it should be (RHEL, Fedora), that means I have some underlying problems with the kernel (CONFIG_CRYPTO_FIPS is off or it's Fedora docker image on Debian/Ubuntu, or it's not a Linux kernel at all).

Flipping that, all you care about is if FIPS is enabled, because then
it's likely to break everything ;)

In fact information about FIPS was added here because we had failures in some tests, which were previously passing, and after troubleshooting we found that our provisioner was picking FIPS-enabled images instead of regular ones by mistake.
And I care not just about enabled/disabled, but about the absence too.

We have no tests that rely on the state of disabled vs not avail, so
the distinction is not necessary.

Don't have them in upstream yet. We have downstream tests that rely on a state of FIPS.

Ohh, and if I haven't mentioned, this is blocking tests running on all
non-RH platforms, so please can this just be reviewed and done with?
It's now preventing me from contributing to the project :(

Please do the changes I asked since it directly affects me. And in the meantime you can use this:

$ py.test-3 --help | grep noconftest
  --noconftest          Don't load any conftest.py files.

Thank you.

FIPS not available or FIPS disabled are the same thing. It's the
abscence of FIPS. The only state's that matter are "enabled" or
"not".
That's why I used bool, not an enum here.

No. If there is an absence of FIPS, that means I can't turn it on or
off in tests. And if I see that FIPS is absent on a platform where it
should be (RHEL, Fedora), that means I have some underlying problems
with the kernel (CONFIG_CRYPTO_FIPS is off or it's Fedora docker
image on Debian/Ubuntu, or it's not a Linux kernel at all).

While I disagree, I have fixed it to the way you want. You now get an
enum of "ENABLED", "DISABLED" or "NOT_AVAILABLE".

Please check the updated PR content. Thanks! :)

>>> print(FIPSState.ENABLED.value)
(0,)

It is returning a tuple here instead of 0. Also I would prefer actual human-readable string like this:

class FIPSState(Enum):
    ENABLED = 'Enabled'
    DISABLED = 'Disabled'
    NOT_AVAILABLE = 'Not available on this platform'

Perhaps it should be 'FIPSState.NOT_AVAILABLE.value'?

Same here

Thank you for your enum implementation, I've left my comments. Other than that I'm happy with the change.

You don't want to make it string return. You want to return the enum to allow future pattern match/debugging etc. Instead, add a def str to the enum, then when it's pushed to %s, it's formatted correctly. That's why you use %s not concat in string building to get nice formatting like this.

New code is pushed, if you ack, please merge too :)

You don't want to make it string return.

I need a string to populate the header for py.test. That's all. This function is not in lib389, it's in conftest.py in dirsrvtests, which is used for test execution only. I'm talking about this report:

py.test-3 conftest.py 
============================= test session starts ==============================
platform linux -- Python 3.6.6, pytest-3.4.2, py-1.5.4, pluggy-0.6.0
389-ds-base: 1.4.0.13-1.fc28
nss: 3.38.0-1.0.fc28
nspr: 4.19.0-1.fc28
openldap: 2.4.46-2.fc28
cyrus-sasl: 2.1.27-0.2rc7.fc28
FIPS: disabled
rootdir: /workspace/ds/dirsrvtests, inifile:
collected 0 items                                                              
========================= no tests ran in 0.04 seconds =========================

You want to return the enum to allow future pattern match/debugging etc.

Right now it is used only once during py.test report generation, see above. And the debugging consists of me looking at the report and our provisioning system. If you're talking about usage in tests, then it should be in lib389. But we can deal with this later, when the tests arrive.

Instead, add a def str to the enum, then when it's pushed to %s, it's formatted correctly. That's why you use %s not concat in string building to get nice formatting like this.

String formatting aside, why do you use an implicit tuple (note the comma at the end)?

ENABLED = 'enabled',

And then return first element of that tuple:

return self.value[0]

I'm genuinely curious, what was the intention behind this?

New code is pushed, if you ack, please merge too :)

You have my ack, but it needs a rebase to master first.

You don't want to make it string return.

I need a string to populate the header for py.test. That's all. This function is not in lib389, it's in conftest.py in dirsrvtests, which is used for test execution only. I'm talking about this report:
py.test-3 conftest.py
============================= test session starts ==============================
platform linux -- Python 3.6.6, pytest-3.4.2, py-1.5.4, pluggy-0.6.0
389-ds-base: 1.4.0.13-1.fc28
nss: 3.38.0-1.0.fc28
nspr: 4.19.0-1.fc28
openldap: 2.4.46-2.fc28
cyrus-sasl: 2.1.27-0.2rc7.fc28
FIPS: disabled
rootdir: /workspace/ds/dirsrvtests, inifile:
collected 0 items

========================= no tests ran in 0.04 seconds =========================

Yes: for now we may be using a string only, but later the ability to have proper enums and matching is going to be important potentially. Better to do it correct now, than try to fix it later, especially given python is weakly typed it makes it neally impossible to change interfaces later.

Right now it is used only once during py.test report generation, see above. And the debugging consists of me looking at the report and our provisioning system. If you're talking about usage in tests, then it should be in lib389. But we can deal with this later, when the tests arrive.

Instead, add a def str to the enum, then when it's pushed to %s, it's formatted correctly. That's why you use %s not concat in string building to get nice formatting like this.

String formatting aside, why do you use an implicit tuple (note the comma at the end)?
ENABLED = 'enabled',

Is it? I thought that was the Enum syntax (all documentation lists the Enum's as having comma delimits between the values).

I'm genuinely curious, what was the intention behind this?

Follow docs? I'll test and fix this because you are right (I also didn't know about implicit tuples :| )

New code is pushed, if you ack, please merge too :)

You have my ack, but it needs a rebase to master first.

Of course.

I'll fix the implicit tuple and merge.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2945

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.