CiscoCloud / distributive

Unit testing for the cloud
Apache License 2.0
147 stars 20 forks source link

Change Port to a connection attempt #109

Closed langston-barrett closed 8 years ago

langston-barrett commented 8 years ago

Fixes issue #102

nyanshak commented 8 years ago

Since one of the goals is to be as "platform agnostic" as possible, I should bring up that this network checking code relies on procfs, which will not exist on some systems, notably FreeBSD, Mac OS X, and probably Windows (not sure about this). Basically, this code is checking /proc/net/{tcp, udp}, but that may not exist.

All hope is not lost, as I believe the Go stlib net abstracts this, allowing you to do something like in this file: https://github.com/coolaj86/golang-test-port/blob/master/test-port.go.

import (
        "log"
        "net"
)

func main() {
        var status string
        conn, err := net.Dial("tcp", "127.0.0.1:80")
        if err != nil {
                log.Println("Connection error:", err)
                status = "Unreachable"
        } else {
                status = "Online"
                defer conn.Close()
        }
        log.Println(status)
}
langston-barrett commented 8 years ago

@nyanshak This is exactly the problem that this PR will solve. Port has been parsing data from procfs, but now will just rely on a connection attempt.

nyanshak commented 8 years ago

Commit f0b1d3 still has this issue. I thought it was about to be merged in, but if it's still in-flight, ignore me. (disappear)

langston-barrett commented 8 years ago

@nyanshak This is about to be merged in, and I don't believe that it experiences this issue, but maybe I'm missing something. The change here is that PortOpen now actually attempts to connect to the port (using CanConnect, which is very similar to the code you posted), instead of relying on procfs. Since the Port, PortTCP, and PortUDP checks all use PortOpen, this will change all of them to not rely on procfs.

nyanshak commented 8 years ago

In checks/network.go:

func (chk Port) Status() (int, string, error) {
    if netstatus.PortOpen("tcp", chk.port) || netstatus.PortOpen("udp", chk.port) {
        return errutil.Success()
    }
    // convert ports to string to send to errutil.GenericError
    var strPorts []string
    openPorts := append(netstatus.OpenPorts("tcp"), netstatus.OpenPorts("udp")...)
    for _, port := range openPorts {
        strPorts = append(strPorts, fmt.Sprint(port))
    }
    return errutil.GenericError("Port not open", fmt.Sprint(chk.port), strPorts)
}

That calls netstatus.PortOpen as well as netstatus.OpenPorts. However, this commit only fixes the /net/proc/{tcp, udp} issue in OpenPorts, but not in PortOpen.

langston-barrett commented 8 years ago

@nyanshak You're correct that in reporting that the port was closed, distributive will attempt to parse data from procfs to report some of the open ports on the system. Maybe we can change this in the future :) otherwise, if this PR looks good, I'm gonna go ahead and merge it.

nyanshak commented 8 years ago

True, it probably could stand to be fixed separately.

Here's what I see for (say) users-and-groups.json (if I fail the tests):

./distributive -f samples/users-and-groups.json --verbosity info                                                                                               14:33:00  ☁  fix/port ☀
INFO[0000] Creating checklist(s)...                      path=samples/users-and-groups.json type=file
INFO[0000] Running checklist UserHasHomeDir
INFO[0000] Running checklist UserHasUID
INFO[0000] Running checklist UserHasGID
INFO[0000] Running checklist UserHasHomeDir
INFO[0000] Running checklist UserExists
INFO[0000] Running checklist GroupExists
INFO[0000] Running checklist UserInGroup
INFO[0000] Running checklist GroupExists
INFO[0000] Running checklist GroupID
INFO[0000] Report from checklist                         checklist=Group checks report=↴
Total: 9
Passed: 5
Failed: 4
Other: 0
User does not have expected HomeDir:
User: root
Given: /root
User does not exist: lb
User does not exist: lb
Group not found:
    Specified: root
    Actual: nobody, nogroup, wheel, daemon, kmem, sys, tty, operator, mail, bin, procview, procmod, owner, everyone, _taskgated, group, staff, _networkd, _installassistant, _lp, _postfix, _postdrop, certusers, _keytabusers, _scsd, _ces, _appstore, utmp, authedusers, interactusers, netusers, consoleusers, _mcxalr, _appleevents, _geod, _serialnumberd, _devdocs, _sandbox, localaccounts, netaccounts, _mdnsresponder, _uucp, _ard, dialer, network, _www, _eppc, _cvs, _svn, _mysql

Whereas I see this for samples/network.json (when it fails):

[distributive] ./distributive -f samples/network.json --verbosity info                                                                                                        14:33:05  ☁  fix/port ☀
INFO[0000] Creating checklist(s)...                      path=samples/network.json type=file
INFO[0000] Running checklist RoutingTableDestination
INFO[0000] Running checklist PortUDP
INFO[0000] Running checklist InterfaceExists
INFO[0000] Running checklist InterfaceExists
INFO[0000] Running checklist InterfaceExists
INFO[0000] Running checklist PortTCP
INFO[0000] Running checklist UDP
INFO[0000] Running checklist Up
INFO[0000] Running checklist UDP
INFO[0000] Running checklist RoutingTableDestination
INFO[0000] Running checklist IP4
INFO[0000] Running checklist RoutingTableInterface
INFO[0000] Running checklist IP6
INFO[0000] Running checklist Gateway
INFO[0000] Running checklist Host
INFO[0000] Running checklist TCP
INFO[0000] Running checklist RoutingTableDestination
INFO[0000] Running checklist GatewayInterface
INFO[0000] Running checklist UDPTimeout
INFO[0000] Running checklist TCPTimeout
INFO[0000] Running checklist Port
INFO[0000] Running checklist RoutingTableDestination
INFO[0000] Running checklist GatewayInterface
FATA[0000] Couldn't read file/dir                        error=open /proc/net/tcp: no such file or directory path=/proc/net/tcp

Seems the error handling could be a bit better here but maybe a discussion for a separate issue.

(I don't see any other issues with this)