FIX: Scan files when a single server is online

[nbianca]

This commits makes sure that even when a single server is online, the plugin is available. This commit also makes sure that a random and online server is chosen when using with_session without any specified socket. It used to use the first one.

[ZogStriP]

Any chances this could be tested? 🤔

[romanrizzi]

This is more or less what I had in mind but we need some more changes. Right now, #with_session closes the socket, so the available socket you find cannot be used to scan a file.

[romanrizzi]

We should change #target_online? to do a PING without opening a session. This way, the socket don't get closed. I checked ClamAV's docs and it doesn't look necessary.

Besides any test we could write for this, we need to try it locally just to be sure it will work as we expect. We completely mock ClamAV on tests.

[nbianca]

Any chances this could be tested? 🤔

I do not think testing would help because we would have to mock all the socket-related logic and thus lose a lot of the benefits of testing. In other words, the tests would just check that the mocks behave as we want.

PING without opening a session

This is interesting, you mean pinging without a session and then reusing the same socket to do the scan, right?

I will also try to run ClamAV in Docker and test it with that.

[romanrizzi]

This is interesting, you mean pinging without a session and then reusing the same socket to do the scan, right?

Yes, doing the PING without going through #with_session to avoid closing the socket before using it.

I believe we'll also need a timeout In our IO#select calls, and assume it's not available if it returns nil. I was too optimistic when I wrote this code 😅

[nbianca]

I am not sure it works using the same socket for PING and some other commands. I tried it like this, but the last response looks empty.

pry(main)> socket = TCPSocket.new("127.0.0.1", 3310, connect_timeout: 3)
=> #<TCPSocket:fd 20, AF_INET, 127.0.0.1, 58433>
pry(main)> socket.sendmsg_nonblock("zPING\0", 0, nil)
=> 6
pry(main)> socket.recvmsg_nonblock(25)
=> ["PONG\x00", #<Addrinfo: empty-sockaddr SOCK_STREAM>, 128]
pry(main)> socket.sendmsg_nonblock("zPING\0", 0, nil)
=> 6
pry(main)> socket.recvmsg_nonblock(25)
=> ["", #<Addrinfo: empty-sockaddr SOCK_STREAM>, 128]

[nbianca]

@romanrizzi, I looked into this further and I have found out that only one command can be sent on a socket and then we must close it and reopen it unless we use the sessions. This is strange because it means we must put the PING inside the session and make sure we don't close the session if we want to scan a file.

[romanrizzi]

We'll have to organize things differently to iterate over different connections and use it only if ClamAV is online. Here's how I imagine this should work:

1. We expose a way to lazily iterate over all sockets, returning the first ClamAV response we get:

class ClamAVServicesPool
  def on_online_target
    response = nil

    service_instance.targets.each do |target|
      socket = build_socket(target)
      next if socket.nil?

      response = yield(socket)
      break if response
    end

    response
  end
end

2. We use it and pass it a block that does what we want. Here's an example of how we check if the target is online:

class ClamAV
  def ping(socket)
    write_in_socket(socket, "zPING\0")
  end

  def accepting_connections?
    response = clamav_services_pool.on_online_target do |socket|
      with_session(socket) { ping(socket) }
    end

    available = response.present? && clean_msg(response) == "PONG"
    update_status(!available)
    available
  end
end

Of course, this is the easiest part but the spirit of the example is that we separate responsabilities: ClamAVServicePool is in charge of opening connections against a server, which means the machine is up. On the other hand, ClamAV takes care of checking the daemon is running inside that machine to update status, stats, or scan files. It checks if it can write on the socket, and if it respond to a ping, returning early if it doesn't, which means we repeat the process on the next socket.

[nbianca]

I think I have a better suggestion and that is to refactor ClamAVServicesPool to be what the name says: a pool of services, instead of a pool of sockets. Then, the service instance can be used to connect as many times as needed and check if the server replies to PING requests.

However, I did not get yet to fix the specs, but I manually tested it and it works well.

[romanrizzi]

@nbianca - I reorganized the code a bit following your suggestion! Although they are a multiple entities playing different roles, I think this makes the code easier to follow.

Please feel free to revert or make any changes you want if you think there's a better solution.

[nbianca]

@romanrizzi, I like the changes. Do you think we can merge it?