feat: Print config on connection test #4301

Glutexo · 2024-12-07T16:22:30Z

All Pull Requests:

Check all that apply:

Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
No Sensitive Data in this change?
Is this PR to correct an issue?
Is this PR an enhancement?

Complete Description of Additions/Changes:

For non-legacy, --test-connection dumps a user-friendly connection configuration.

First, the authentication information is printed starting with type. For BASIC, username is printed. For CERT, certificate and key paths are verified and printed. In case of missing files or credentials, the connection test fails immediately.

Second, tested URLs (base, Ingress, Inventory, API cast) are listed and server type (production, staging, Satellite) is determined. HTTPS proxy information is included.

Card IDs:

CCT-963

m-horky

Reviewing without running the code first. The methods seem reasonably split, that's a good indicator of it being maintainable, good job on that.

My biggest problem with this approach is that you are using logs as public API. Your output, to be pretty-printed, is prefixed with a bunch of space characters, and the fact that the stdout is your API makes your tests very verbose and difficult to handle. There is no separation between the business logic and the presentation.

Instead, I'd much rather see the tests return some kinds of objects (class instances, named tuples, exceptions, whatever) that would contain information about what has gone wrong.

For example, in _test_url_config(), we could do something like

try:
    parsed = urllib3.util.url.parse_url(url)
except urllib3.exceptions.LocationParseError:
    return ConfigurationIssue(
        cause="Malformed URL",
        url=url,
        description="API URL"
    )

what would get pretty-printed somewhere else. This would make your tests much simpler and smaller.

I know the Core codebase does it, but please don't misuse logging for standard output. For this output-heavy command, let's use prints instead. I'd like to see logs as well, for both ok and failed findings, to allow support and engineers debug network issues, though

14:37:01.001 ... test_connection OK: test=proxy-url, result=https://proxy.internal
14:37:01.003 ... test_connection OK: test=proxy-connection, result=can establish connection

that could be presented as text

Running tests...
- Proxy configuration is valid: OK
- Proxy is reachable: OK
  ...

m-horky · 2025-01-20T11:11:42Z

insights/client/connection.py

+
+
+def _exception_root_cause(exception):
+    while True:


Are we sure this can't create an infinite loop?

m-horky · 2025-01-20T11:19:22Z

insights/client/constants.py

+    insights_ip_prod = "23.37.45.238"
+    insights_ip_stage = "23.53.5.13"
+    stable_public_url = "one.one.one.one"  # Public CloudFlare DNS
+    stable_public_ip = "1.1.1.1"  # Public CloudFlare DNS


While this looks smart, I'd be extremely worried to merge it. How can we be sure this will never change? Whole /11 range is owned by Akamai. The same goes to the DNS, I appreciate the effort, but I don't think we can do this in something that ships in RHEL.

For DNS diagnostics, I'd rather print "Unable to contact domain example.org (using DNS server a.b.c.d)." than use hardcoded ones.

m-horky · 2025-01-20T11:21:19Z

insights/client/connection.py

+    for error in body["errors"]:
+        if isinstance(error, dict) and "status" in error and error["status"] == 401 and "meta" in error and isinstance(error["meta"], dict) and "response_by" in error["meta"] and error["meta"]["response_by"] == "gateway":
+            return True


That's a long line! Wrap everything to 78 (or something higher but reasonable) please.

m-horky · 2025-01-20T11:21:48Z

insights/client/connection.py

+        if "verify" not in kwargs:
+            kwargs["verify"] = "/home/insights/simple-http-server/cert.pem"


m-horky · 2025-01-20T11:22:52Z

insights/client/connection.py

+        if self.authmethod == "BASIC":
+            logger.info("Authentication: login credentials (%s)", self.authmethod)
+
+            for desc, var, placeholder in [
+                ("Username", self.username, None),
+                ("Password", self.password, "********"),
+            ]:
+                if not var:
+                    error = ["  %s NOT SET.", desc]
+                    errors.append(error)
+
+                val = placeholder or var if var else "NOT SET"
+                logger.info("  %s: %s", desc, val)


Since 2025, BASIC authentication is not supported, we can drop this code.

m-horky · 2025-01-20T11:28:32Z

insights/tests/client/connection/test_test_connection.py

+        (None, None, "NOT SET", "NOT SET", ["Username NOT SET", "Password NOT SET"]),
+    ],
+)
+def test_test_connection_basic_auth_incomplete_credentials_log(


BASIC auth is something we don't need to care about anymore.

For non-legacy, --test-connection dumps a user-friendly connection configuration. First, the authentication information is printed starting with type. For BASIC, username is printed. For CERT, certificate and key paths are verified and printed. In case of missing files or credentials, the connection test fails immediately. Second, tested URLs (base, Ingress, Inventory, API cast) are listed and server type (production, staging, Satellite) is determined. HTTPS proxy information is included. feat: Improve test URLs output _test_urls and _legacy_test_urls output is nicer, with clear SUCCESS/FAILURE statement. URLs are consistently listed, so is legacy fallback. With --verbose turned on, more information about requests, responses and errors are printed. The readability of the output improved drastically, with only little changes to the logging and tiny touches to the logic. The generic HTTP method logs information about the request. To make the log messages blend nicely into the connection test, introduced logging-related arguments: * To keep the output concise by default, but more helpful with --verbose, log_level suppresses HTTP details. * To match indentation with messages outside the request method, log_prefix allows to add spaces to the beginning. chore: Use return for flow control Exceptions in _(legacy_)test_urls are merely used for control-flow. Known ones are re-thrown and re-caught in test_connection, unknown ones are not caught at all. Return is more appropriate: _test_urls passes the result, test_connection decides how to handle it. feat: Test GET from Inventory Inventory is tested along with Ingress and an API ping. Hosts are listed as the most basic Inventory GET request. feat: Check connection In case of DNS failure. The DNS is queried, then a connection is established to the resolved IP. If resolving fails, a hard-coded IP is tried for production or staging. In case of either failure, DNS query for a public CloudFlare URL one.one.one.one and its IP 1.1.1.1 is tried. feat: Recognize more errors * 429 Too Many requests means the rate limit was hit. * 401 Unauthorized from gateway means the username/password is invalid. * SSLError means the key/certificate pair is invalid. * SSL: WRONG_VERSION_NUMBER in the SSLError means that HTTPS has been used to contact an HTTP server. * ConnectionTimeout and ReadTimeout may mean the connection is slow. feat: Detect proxy errors HTTPS proxy introduces several possible error cases, similar to the actual remote server connection: * proxy name resolution (DNS) error, * proxy connection error, * proxy authentication error. The proxy authentication error can only be recognized by a string in the underlying OSError: the outer exception is a plain remote server connection error. Although the proxy is used for HTTPS connection, the actual communication for the proxy itself is HTTP. Thus, specifying a HTTPS protocol for the proxy causes a specific WRONG_VERSION_NUMBER SSL error. feat: Validate URLs urlparse from Python stdlib doesn’t fail on an invalid URL. parse_url from urllib3 used by requests does though. Invalid base URL or proxy URL raises thus an uncaught exception. Card IDs: * CCT-963 Signed-off-by: Štěpán Tomsa <stomsa@redhat.com>

Glutexo · 2025-01-30T15:10:56Z

Copying my JIRA comment:

To focus on another task, I am leaving this pull request to Petter. I pushed everything I have done so far.

Matyáš’s review comments need to be addressed. Most importantly his request to split the presentation (error messages) from the logic (the actual connection tests). The test URLs method should return an object, which is then separately translated to the output. Doing so, the function can be mocked and tests are less brittle. (I was thinking about putting this in a follow-up card, keeping the current Insights Client style for now. Matyáš doesn’t want to merge without the split though.)

Other things that need to be done are:

The messages suggesting to check Red Hat Status page should only be printed if the Client connects to Red Hat servers, not Satellite.
Existing tests (currently marked as skipped) need to be either fixed or removed.
The pull request description and the commit message should describe its content. Currently, it’s an unedited squash of multiple outdated commit messages.

I thought it would be appropriate to change all the non-debug test connection log messages to the INFO level: even a failure is not an error, just an expected output. Doing so would remove log level mixing and simplify the tests a lot: a simple function expanding a list of messages into caplog record tuples would save a lot of repetition.

Personally, I would think parametrize the insights_connection fixture configured by the insights_config marker. That would allow using the fixture in the few tests that currently create the object manually. See Parametrizing fixtures in Pytest documentation.

Glutexo self-assigned this Dec 7, 2024

Glutexo added the client These issues represent work to be done by the "client" team. label Dec 7, 2024

Glutexo force-pushed the test-connection branch 3 times, most recently from ca48f81 to e3c1f79 Compare December 20, 2024 19:28

Glutexo force-pushed the test-connection branch 5 times, most recently from 5ba1101 to 27550e3 Compare January 3, 2025 13:01

Glutexo force-pushed the test-connection branch 4 times, most recently from 7a1a1b4 to b7d593c Compare January 12, 2025 14:29

Glutexo force-pushed the test-connection branch 6 times, most recently from 0afafe9 to 7561e75 Compare January 20, 2025 10:51

Glutexo marked this pull request as ready for review January 20, 2025 10:57

m-horky suggested changes Jan 20, 2025

View reviewed changes

Glutexo force-pushed the test-connection branch 6 times, most recently from d08127f to e0eae5d Compare January 21, 2025 14:31

Glutexo force-pushed the test-connection branch from e0eae5d to 2d23bc6 Compare January 30, 2025 14:51

Glutexo force-pushed the test-connection branch from 2d23bc6 to 302347a Compare January 30, 2025 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Print config on connection test #4301

feat: Print config on connection test #4301

Glutexo commented Dec 7, 2024

m-horky left a comment

m-horky Jan 20, 2025

m-horky Jan 20, 2025

m-horky Jan 20, 2025

m-horky Jan 20, 2025

m-horky Jan 20, 2025

m-horky Jan 20, 2025

Glutexo commented Jan 30, 2025

		if "verify" not in kwargs:
		kwargs["verify"] = "/home/insights/simple-http-server/cert.pem"

feat: Print config on connection test #4301

Are you sure you want to change the base?

feat: Print config on connection test #4301

Conversation

Glutexo commented Dec 7, 2024

All Pull Requests:

Complete Description of Additions/Changes:

m-horky left a comment

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

m-horky Jan 20, 2025

Choose a reason for hiding this comment

Glutexo commented Jan 30, 2025