RHSecurityCompliance / contest

Content Testing for ComplianceAsCode/content
Other
4 stars 7 forks source link

Use dynamic ports for BackgroundHTTPServer #223

Closed comps closed 1 month ago

comps commented 1 month ago

This is a follow-up to https://github.com/RHSecurityCompliance/contest/pull/210/commits/3277a788bb19f9b6b4f1e62665413936d705f255 from https://github.com/RHSecurityCompliance/contest/pull/210.

I've tested

/hardening/anaconda/stig$
/hardening/image-builder/stig$
/scanning/disa-alignment/anaconda$

(the main/only tests impacted by this) and they all seem to pass/warn just fine, so I presume it works.

I also ran some multi-profile runs of anaconda / image-builder to make sure there isn't anything persistent that would break 2 or more tests running sequentially on a single system.

comps commented 1 month ago

For the record (in case you get the idea during review), this:

-srv = util.BackgroundHTTPServer(virt.NETWORK_HOST, 0)
-with srv:
+with util.BackgroundHTTPServer(virt.NETWORK_HOST, 0) as srv:

doesn't work - the __enter__ and __exit__ functions are methods of srv, which (the same object) is then further used inside the with block. The as syntax is for when __enter__ itself returns something to be used in the block.

I guess we could return self from __enter__, but maybe that would be even more confusing for readers / maintainers, as there would be 2 not-so-obvious ways to get the object.

comps commented 1 month ago

I guess we could return self from enter, but maybe that would be even more confusing for readers / maintainers, as there would be 2 not-so-obvious ways to get the object.

What do you mean by there would be 2 not-so-obvious ways to get the object? If we return self from __enter__ and use it only in with+as, then there would be basically "only" one way we would be okay with.

The problem is the enter_context() cases where we need to

  1. get the server object, for adding files to
  2. add files to the object
  3. start the object as a server

with some other http-server-unrelated code in between.

It's a subtle difference but we would essentially be doing the equivalent of

with ExitStack() as stack:
    f = open('file.txt')
    stack.enter_context(f)

which is not the same as

with ExitStack() as stack:
    stack.enter_context(open('file.txt'))

due to how the context manager is called.


Maybe I should side-step this issue and a .start() function, to remove the confusion of the cases mentioned above,

with util.BackgroundHTTPServer('127.0.0.1', 0) as srv:
    srv.add_file(...)
    srv.start()   # can be swapped with add_file()
    ...

or

srv = util.BackgroundHTTPServer('127.0.0.1', 0)
srv.add_file(...)
stack.enter_context(srv.start())   # can be swapped with add_file()

?

comps commented 1 month ago

I've pushed an alternate approach - have a look at the module help for lib/util/httpsrv.py.

Hopefully this makes things a bit clearer.

comps commented 1 month ago

Tested on -t '/hardening/image-builder/(ospp|stig)$' -t '/hardening/anaconda/(ospp|stig)$' -t '/scanning/disa-alignment/anaconda$' --arch x86_64 --rhel 9 --rhel 8 and it seems to work.

mildas commented 1 month ago

Yep, code looks good

Nit-pick: f8af92cb95adb3f800d0816f6bae37b1bade89e4 is unnecessary now. Could you drop it?

comps commented 1 month ago

Yep, code looks good

Nit-pick: f8af92c is unnecessary now. Could you drop it?

Done.