Fix Minor Windows Issues

newrelic/admin/record_deploy.py

@@ -12,8 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import os
-import pwd
+import getpass
 
 from newrelic.admin import command, usage
 from newrelic.common import agent_http, encoding_utils
@@ -79,7 +78,7 @@ def record_deploy(
         path = f"/v2/applications/{app_id}/deployments.json"
 
         if user is None:
-            user = pwd.getpwuid(os.getuid()).pw_gecos
+            user = getpass.getuser()
 
         deployment = {}
         deployment["revision"] = revision

newrelic/config.py

@@ -1194,10 +1194,11 @@ def _module_function_glob(module, object_path):
             # Skip adding all class methods on failure
             pass
 
-        # Under the hood uses fnmatch, which uses os.path.normcase
-        # On windows this would cause issues with case insensitivity,
-        # but on all other operating systems there should be no issues.
-        return fnmatch.filter(available_functions, object_path)
+        # Globbing must be done using fnmatch.fnmatchcase as
+        # fnmatch.filter and fnmatch.fnmatch use os.path.normcase
+        # which cause case insensitivity issues on Windows.
+
+        return [func for func in available_functions if fnmatch.fnmatchcase(func, object_path)]
 
 
 # Setup wsgi application wrapper defined in configuration file.

newrelic/core/agent_control_health.py

@@ -15,12 +15,14 @@
 import logging
 import os
 import sched
+import sys
 import threading
 import time
 import uuid
 from enum import IntEnum
 from pathlib import Path
 from urllib.parse import urlparse
+from urllib.request import url2pathname
 
 from newrelic.core.config import _environ_as_bool, _environ_as_int
 
@@ -66,43 +68,6 @@ class HealthStatus(IntEnum):
 NR_CONNECTION_ERROR_CODES = frozenset([HealthStatus.FAILED_NR_CONNECTION.value, HealthStatus.FORCED_DISCONNECT.value])
 
 
-def is_valid_file_delivery_location(file_uri):
-    # Verify whether file directory provided to agent via env var is a valid file URI to determine whether health
-    # check should run
-    try:
-        parsed_uri = urlparse(file_uri)
-        if not parsed_uri.scheme or not parsed_uri.path:
-            _logger.warning(
-                "Configured Agent Control health delivery location is not a complete file URI. Health check will not be "
-                "enabled. "
-            )
-            return False
-
-        if parsed_uri.scheme != "file":
-            _logger.warning(
-                "Configured Agent Control health delivery location does not have a valid scheme. Health check will not be "
-                "enabled."
-            )
-            return False
-
-        path = Path(parsed_uri.path)
-
-        # Check if the path exists
-        if not path.exists():
-            _logger.warning(
-                "Configured Agent Control health delivery location does not exist. Health check will not be enabled."
-            )
-            return False
-
-        return True
-
-    except Exception:
-        _logger.warning(
-            "Configured Agent Control health delivery location is not valid. Health check will not be enabled."
-        )
-        return False
-
-
 class AgentControlHealth:
     _instance_lock = threading.Lock()
     _instance = None
@@ -127,6 +92,7 @@ def __init__(self):
         self.status_message = HEALTHY_STATUS_MESSAGE
         self.start_time_unix_nano = None
         self.pid_file_id_map = {}
+        self._health_delivery_location_cache = {}
 
     @property
     def health_check_enabled(self):
@@ -135,16 +101,87 @@ def health_check_enabled(self):
         if not agent_control_enabled:
             return False
 
-        return is_valid_file_delivery_location(self.health_delivery_location)
+        return self.health_delivery_location_is_valid
 
     @property
     def health_delivery_location(self):
-        # Set a default file path if env var is not set or set to an empty string
-        health_file_location = (
+        file_uri = (
             os.environ.get("NEW_RELIC_AGENT_CONTROL_HEALTH_DELIVERY_LOCATION", "") or "file:///newrelic/apm/health"
         )
 
-        return health_file_location
+        # Return from cache if already parsed
+        if file_uri in self._health_delivery_location_cache:
+            return self._health_delivery_location_cache[file_uri]
+
+        # Parse and add to cache
+        path = self.parse_health_delivery_location(file_uri)
+        if path is not None:
+            self._health_delivery_location_cache[file_uri] = path
+
+        return path
+
+    @property
+    def health_delivery_location_is_valid(self):
+        # Verify whether file directory provided to agent via env var is a valid file URI to determine whether health
+        # check should run
+        try:
+            path = self.health_delivery_location
+            if path is None:
+                # Warning already logged in parse_health_delivery_location()
+                return False
+
+            # Check if the path exists
+            if not path.exists():
+                _logger.warning(
+                    "Configured Agent Control health delivery location does not exist. Health check will not be enabled."
+                )
+                return False
+
+            return True
+
+        except Exception:
+            _logger.warning(
+                "Configured Agent Control health delivery location is not valid. Health check will not be enabled."
+            )
+            return False
+
+    @classmethod
+    def parse_health_delivery_location(cls, file_uri):
+        """Parse the health delivery location and return it as a Path object."""
+
+        # No built in method to correctly parse file URI to a path on Python < 3.13.
+        # In the future, Path.from_uri() can be used directly.
+
+        # For now, parse with urllib.parse.urlparse and convert to a Path object.
+        parsed_uri = urlparse(file_uri)
+
+        # Ensure URI has at least a scheme and path
+        if not parsed_uri.scheme or not parsed_uri.path:
+            _logger.warning(
+                "Configured Agent Control health delivery location is not a complete file URI. Health check will not be enabled."
+            )
+            return None
+
+        # Ensure URI has a file scheme
+        if parsed_uri.scheme != "file":
+            _logger.warning(
+                "Configured Agent Control health delivery location does not have a valid scheme. Health check will not be enabled."
+            )
+            return None
+
+        # Handle Windows systems carefully due to inconsistent path handling
+        if sys.platform == "win32":
+            if parsed_uri.netloc:
+                # Matching behavior of pip where netloc is prepended with a double backslash
+                # https://github.com/pypa/pip/blob/022248f6484fe87dc0ef5aec3437f4c7971fd14b/pip/download.py#L442
+                urlpathname = url2pathname(rf"\\\\{parsed_uri.netloc}{parsed_uri.path}")
+                return Path(urlpathname)
+            else:
+                # If there's no netloc, we use url2pathname to fix leading slashes
+                return Path(url2pathname(parsed_uri.path))
+        else:
+            # On non-Windows systems we can use the parsed path directly
+            return Path(parsed_uri.path)
 
     @property
     def is_healthy(self):
@@ -185,13 +222,16 @@ def write_to_health_file(self):
         status_time_unix_nano = time.time_ns()
 
         try:
-            file_path = urlparse(self.health_delivery_location).path
+            health_dir_path = self.health_delivery_location
+            if health_dir_path is None:
+                # Allow except block to handle logging a warning
+                raise ValueError("Health delivery location is not valid.")
+
             file_id = self.get_file_id()
-            file_name = f"health-{file_id}.yml"
-            full_path = Path(file_path) / file_name
-            is_healthy = self.is_healthy
+            health_file_path = health_dir_path / f"health-{file_id}.yml"
+            is_healthy = self.is_healthy  # Cache property value to avoid multiple calls
 
-            with full_path.open("w") as f:
+            with health_file_path.open("w") as f:
                 f.write(f"healthy: {is_healthy}\n")
                 f.write(f"status: {self.status_message}\n")
                 f.write(f"start_time_unix_nano: {self.start_time_unix_nano}\n")

tests/agent_features/test_agent_control_health_check.py

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import re
+import sys
 import threading
 import time
 from pathlib import Path
@@ -21,11 +22,7 @@
 from testing_support.http_client_recorder import HttpClientRecorder
 
 from newrelic.config import _reset_configuration_done, initialize
-from newrelic.core.agent_control_health import (
-    HealthStatus,
-    agent_control_health_instance,
-    is_valid_file_delivery_location,
-)
+from newrelic.core.agent_control_health import HealthStatus, agent_control_health_instance
 from newrelic.core.agent_protocol import AgentProtocol
 from newrelic.core.application import Application
 from newrelic.core.config import finalize_application_settings, global_settings
@@ -41,8 +38,29 @@ def get_health_file_contents(tmp_path):
 
 
 @pytest.mark.parametrize("file_uri", ["", "file://", "/test/dir", "foo:/test/dir"])
-def test_invalid_file_directory_supplied(file_uri):
-    assert not is_valid_file_delivery_location(file_uri)
+def test_invalid_file_directory_supplied(monkeypatch, file_uri):
+    # Setup expected env vars to run agent control health check
+    monkeypatch.setenv("NEW_RELIC_AGENT_CONTROL_ENABLED", "True")
+    monkeypatch.setenv("NEW_RELIC_AGENT_CONTROL_HEALTH_DELIVERY_LOCATION", file_uri)
+
+    agent_control_instance = agent_control_health_instance()
+    assert not agent_control_instance.health_delivery_location_is_valid
+
+
+@pytest.mark.skipif(sys.platform != "win32", reason="Only valid for Windows")
+@pytest.mark.parametrize("leading_slash", [True, False], ids=["leading_slash", "no_leading_slash"])
+def test_inconsistent_paths_on_windows(monkeypatch, tmp_path, leading_slash):
+    file_uri = tmp_path.as_uri()
+    if not leading_slash:
+        assert file_uri.startswith("file:///")
+        file_uri.replace("file:///", "file://")
+
+    # Setup expected env vars to run agent control health check
+    monkeypatch.setenv("NEW_RELIC_AGENT_CONTROL_ENABLED", "True")
+    monkeypatch.setenv("NEW_RELIC_AGENT_CONTROL_HEALTH_DELIVERY_LOCATION", file_uri)
+
+    agent_control_instance = agent_control_health_instance()
+    assert agent_control_instance.health_delivery_location_is_valid
 
 
 def test_agent_control_not_enabled(monkeypatch, tmp_path):

tests/agent_features/test_configuration.py

@@ -17,11 +17,11 @@
 import logging
 import pathlib
 import sys
-import tempfile
 import urllib.parse as urlparse
 
 import pytest
 from testing_support.fixtures import override_generic_settings
+from testing_support.util import NamedTemporaryFile
 
 from newrelic.api.exceptions import ConfigurationError
 from newrelic.common.object_names import callable_name
@@ -637,7 +637,7 @@ def test_initialize():
 def test_initialize_raises_if_config_does_not_match_previous():
     error_message = "Configuration has already been done against differing configuration file or environment.*"
     with pytest.raises(ConfigurationError, match=error_message):
-        with tempfile.NamedTemporaryFile() as f:
+        with NamedTemporaryFile() as f:
             f.write(newrelic_ini_contents)
             f.seek(0)
 
@@ -646,7 +646,7 @@ def test_initialize_raises_if_config_does_not_match_previous():
 
 def test_initialize_via_config_file():
     _reset_configuration_done()
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -667,7 +667,7 @@ def test_initialize_config_file_does_not_exist():
 
 def test_initialize_environment():
     _reset_configuration_done()
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -676,7 +676,7 @@ def test_initialize_environment():
 
 def test_initialize_log_level():
     _reset_configuration_done()
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -685,7 +685,7 @@ def test_initialize_log_level():
 
 def test_initialize_log_file():
     _reset_configuration_done()
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -700,7 +700,7 @@ def test_initialize_config_file_feature_flag(feature_flag, expect_warning, logge
     apply_config_setting(settings, "feature_flag", feature_flag)
     _reset_configuration_done()
 
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -759,7 +759,7 @@ def test_initialize_config_file_with_traces(setting_name, setting_value, expect_
     apply_config_setting(settings, setting_name, setting_value)
     _reset_configuration_done()
 
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -951,7 +951,7 @@ def test_initialize_developer_mode(section, expect_error, logger):
     _reset_instrumentation_done()
     _reset_config_parser()
 
-    with tempfile.NamedTemporaryFile() as f:
+    with NamedTemporaryFile() as f:
         f.write(newrelic_ini_contents)
         f.write(section)
         f.seek(0)
@@ -1019,7 +1019,7 @@ def test_toml_parse_development():
     _reset_config_parser()
     _reset_instrumentation_done()
 
-    with tempfile.NamedTemporaryFile(suffix=".toml") as f:
+    with NamedTemporaryFile(suffix=".toml") as f:
         f.write(newrelic_toml_contents)
         f.seek(0)
 
@@ -1041,7 +1041,7 @@ def test_toml_parse_production():
     _reset_config_parser()
     _reset_instrumentation_done()
 
-    with tempfile.NamedTemporaryFile(suffix=".toml") as f:
+    with NamedTemporaryFile(suffix=".toml") as f:
         f.write(newrelic_toml_contents)
         f.seek(0)
 
@@ -1061,7 +1061,7 @@ def test_config_file_path_types_ini(pathtype):
     _reset_config_parser()
     _reset_instrumentation_done()
 
-    with tempfile.NamedTemporaryFile(suffix=".ini") as f:
+    with NamedTemporaryFile(suffix=".ini") as f:
         f.write(newrelic_ini_contents)
         f.seek(0)
 
@@ -1081,7 +1081,7 @@ def test_config_file_path_types_toml(pathtype):
     _reset_config_parser()
     _reset_instrumentation_done()
 
-    with tempfile.NamedTemporaryFile(suffix=".toml") as f:
+    with NamedTemporaryFile(suffix=".toml") as f:
         f.write(newrelic_toml_contents)
         f.seek(0)