Skip to content
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

fix: use wrapError function instead of Join #328

Closed
wants to merge 1 commit into from

Conversation

amircybersec
Copy link
Contributor

The UDP errors are currently not getting fully unwrapped by the following function in test-connectivity:

func unwrapAll(err error) error {
	for {
		unwrapped := errors.Unwrap(err)
		if unwrapped == nil {
			return err
		}
		err = unwrapped
	}
}

The appear like this in reports:

read message failed: read udp 10.10.10.214:51911->23.109.128.XX:5000: i/o timeout

Whereas it must appears as:

i/o timeout

This is caused by the use of errors.Join in resolver.go.

errors.Join creates a new error that contains multiple errors but doesn't maintain the proper unwrapping chain. To preserve the unwrapping chain, we should use a custom wrapError function.

This PR submits a patch to address this issue and the following report now does not contain other messages:

{
  "test": {
    "resolver": "8.8.8.8:53",
    "proto": "udp",
    "transport": "socks5://REDACTED@proxy.soax.com:5000|ss://REDACTED@amsterdam.server.com:443",
    "time": "2024-11-11T22:09:33Z",
    "duration_ms": 5001,
    "error": {
      "op": "receive",
      "posix_error": "ETIMEDOUT",
      "msg": "i/o timeout",
    }
  },
  "dns_queries": [
    {
      "query_name": "proxy.soax.com",
      "time": "2024-11-11T22:09:33Z",
      "duration_ms": 18,
      "answer_ips": [
        "23.109.148.44",
        "23.109.157.92",
        "23.109.90.116"
      ],
      "error": ""
    }
  ],
  "tcp_connections": [
    {
      "hostname": "proxy.soax.com",
      "ip": "23.109.148.44",
      "port": "5000",
      "error": "",
      "time": "2024-11-11T22:09:33Z",
      "duration_ms": 148
    }
  ],
  "udp_connections": [
    {
      "hostname": "23.109.128.28",
      "ip": "23.109.128.28",
      "port": "5000",
      "error": "",
      "time": "2024-11-11T22:09:34Z",
      "duration_ms": 0
    }
  ]
}

@amircybersec amircybersec requested a review from fortuna November 11, 2024 22:19
@@ -186,16 +186,16 @@ func queryDatagram(conn io.ReadWriter, q dnsmessage.Question) (*dnsmessage.Messa
err = nil
}
if err != nil {
return nil, &nestedError{ErrReceive, errors.Join(returnErr, fmt.Errorf("read message failed: %w", err))}
return nil, &nestedError{ErrReceive, wrapError(returnErr, fmt.Errorf("read message failed: %w", err))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantic here is broken.
These are not nested errors. They are sibling.
You are instead saying that the error is the last one, with unrelated sub-errors.

We need to define what semantic we want to represent in the UDP test.
Perhaps we only look at the first error? Call Unwrap() []error instead, extract the first one, then do the usual error handling.

If you need to look at the other errors, you need to figure out what that means. Still, that can live in the test code.

@amircybersec
Copy link
Contributor Author

@fortuna you are right. It is a better approach to change the unwrapping implementation in test-connectivity code instead. For example, with the follow implementation (instead of using unwrappAll), I can unwrap the both tcp and udp errors down to the underlying socket error. I am going to close this PR and open a new one to update test-connectivity code.

// findBaseError unwraps an error chain to find the most basic underlying error
func findBaseError(err error) error {
	for err != nil {
		// Try to unwrap as joined errors first
		if unwrapInterface, ok := err.(interface{ Unwrap() []error }); ok {
			errs := unwrapInterface.Unwrap()
			if len(errs) > 0 {
				// Take the last error in the joined slice as it's likely
				// to be the most specific one
				err = errs[len(errs)-1]
				continue
			}
		}

		// Try to unwrap as single error
		unwrapped := errors.Unwrap(err)
		if unwrapped == nil {
			// We've reached the base error
			return err
		}
		err = unwrapped
	}
	return err
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants