fix(ci-visibility): make ITR enable properly in EVP mode [backport #6678 to 1.18] (#6716)

romainkomorn-exdatadog · web-flow · commit f37896de63aa · 2023-08-22T17:08:32.000+01:00
Backport 54f2493 from #6684 to 1.18. Fixes an issue where ITR would not enable properly in EVP mode, due to a missing `X-Datadog-NeedsAppKey` header. This would cause requests to the endpoints that check for feature enablement to fail with "403 Forbidden" errors, disabling ITR entirely for the rest of the run. This PR also updates a couple of places to use constants rather than string literals for `dd-api-key` and `dd-app-key` headers in agentless mode. This was tested using both agentless and EVP modes, verifying both that ITR was skipping tests properly, and that subsequent modifications which altered files would properly cause tests to run, and coverage events to be uploaded. Fixes CIAPP-7443 . ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
diff --git a/ddtrace/internal/ci_visibility/constants.py b/ddtrace/internal/ci_visibility/constants.py
@@ -30,6 +30,10 @@
 EVP_PROXY_AGENT_ENDPOINT = "{}/api/v2/citestcycle".format(EVP_PROXY_AGENT_BASE_PATH)
 AGENTLESS_ENDPOINT = "api/v2/citestcycle"
 AGENTLESS_COVERAGE_ENDPOINT = "api/v2/citestcov"
+AGENTLESS_API_KEY_HEADER_NAME = "dd-api-key"
+AGENTLESS_APP_KEY_HEADER_NAME = "dd-application-key"
+EVP_NEEDS_APP_KEY_HEADER_NAME = "X-Datadog-NeedsAppKey"
+EVP_NEEDS_APP_KEY_HEADER_VALUE = "true"
 EVP_PROXY_COVERAGE_ENDPOINT = "{}/{}".format(EVP_PROXY_AGENT_BASE_PATH, AGENTLESS_COVERAGE_ENDPOINT)
 EVP_SUBDOMAIN_HEADER_API_VALUE = "api"
 EVP_SUBDOMAIN_HEADER_COVERAGE_VALUE = "citestcov-intake"
diff --git a/ddtrace/internal/ci_visibility/git_client.py b/ddtrace/internal/ci_visibility/git_client.py
@@ -23,7 +23,11 @@
 from .. import compat
 from ..utils.http import Response
 from ..utils.http import get_connection
+from .constants import AGENTLESS_API_KEY_HEADER_NAME
+from .constants import AGENTLESS_APP_KEY_HEADER_NAME
 from .constants import AGENTLESS_DEFAULT_SITE
+from .constants import EVP_NEEDS_APP_KEY_HEADER_NAME
+from .constants import EVP_NEEDS_APP_KEY_HEADER_VALUE
 from .constants import EVP_PROXY_AGENT_BASE_PATH
 from .constants import EVP_SUBDOMAIN_HEADER_API_VALUE
 from .constants import EVP_SUBDOMAIN_HEADER_NAME
@@ -134,9 +138,15 @@ def _search_commits(cls, requests_mode, base_url, repo_url, latest_commits, seri
     def _do_request(cls, requests_mode, base_url, endpoint, payload, serializer, headers=None):
         # type: (int, str, str, str, CIVisibilityGitClientSerializerV1, Optional[dict]) -> Response
         url = "{}/repository{}".format(base_url, endpoint)
-        _headers = {"dd-api-key": serializer.api_key, "dd-application-key": serializer.app_key}
+        _headers = {
+            AGENTLESS_API_KEY_HEADER_NAME: serializer.api_key,
+            AGENTLESS_APP_KEY_HEADER_NAME: serializer.app_key,
+        }
         if requests_mode == REQUESTS_MODE.EVP_PROXY_EVENTS:
-            _headers = {EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE}
+            _headers = {
+                EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE,
+                EVP_NEEDS_APP_KEY_HEADER_NAME: EVP_NEEDS_APP_KEY_HEADER_VALUE,
+            }
         if headers is not None:
             _headers.update(headers)
         try:
diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py
@@ -25,7 +25,11 @@
 from ddtrace.provider import CIContextProvider
 
 from .. import agent
+from .constants import AGENTLESS_API_KEY_HEADER_NAME
+from .constants import AGENTLESS_APP_KEY_HEADER_NAME
 from .constants import AGENTLESS_DEFAULT_SITE
+from .constants import EVP_NEEDS_APP_KEY_HEADER_NAME
+from .constants import EVP_NEEDS_APP_KEY_HEADER_VALUE
 from .constants import EVP_PROXY_AGENT_BASE_PATH
 from .constants import EVP_SUBDOMAIN_HEADER_API_VALUE
 from .constants import EVP_SUBDOMAIN_HEADER_EVENT_VALUE
@@ -168,21 +172,27 @@ def _should_collect_coverage(coverage_enabled_by_api):
 
     def _check_enabled_features(self):
         # type: () -> Tuple[bool, bool]
-        if not self._app_key:
-            log.debug("Cannot make request to setting endpoint if application key is not set")
+        # DEV: Remove this ``if`` once ITR is in GA
+        if not ddconfig._ci_visibility_intelligent_testrunner_enabled:
             return False, False
 
-        _headers = {
-            "dd-api-key": self._api_key,
-            "dd-application-key": self._app_key,
-            "Content-Type": "application/json",
-        }
-
         if self._requests_mode == REQUESTS_MODE.EVP_PROXY_EVENTS:
             url = get_trace_url() + EVP_PROXY_AGENT_BASE_PATH + SETTING_ENDPOINT
-            _headers = {EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE}
+            _headers = {
+                EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE,
+                EVP_NEEDS_APP_KEY_HEADER_NAME: EVP_NEEDS_APP_KEY_HEADER_VALUE,
+            }
+            log.debug("Making EVP request to agent: url=%s, headers=%s", url, _headers)
         elif self._requests_mode == REQUESTS_MODE.AGENTLESS_EVENTS:
+            if not self._app_key or not self._api_key:
+                log.debug("Cannot make request to setting endpoint if application key is not set")
+                return False, False
             url = "https://api." + self._dd_site + SETTING_ENDPOINT
+            _headers = {
+                AGENTLESS_API_KEY_HEADER_NAME: self._api_key,
+                AGENTLESS_APP_KEY_HEADER_NAME: self._app_key,
+                "Content-Type": "application/json",
+            }
         else:
             log.warning("Cannot make requests to setting endpoint if mode is not agentless or evp proxy")
             return False, False
@@ -314,8 +324,11 @@ def _fetch_tests_to_skip(self, skipping_mode):
             "Content-Type": "application/json",
         }
         if self._requests_mode == REQUESTS_MODE.EVP_PROXY_EVENTS:
-            url = EVP_PROXY_AGENT_BASE_PATH + SKIPPABLE_ENDPOINT
-            _headers = {EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE}
+            url = get_trace_url() + EVP_PROXY_AGENT_BASE_PATH + SKIPPABLE_ENDPOINT
+            _headers = {
+                EVP_SUBDOMAIN_HEADER_NAME: EVP_SUBDOMAIN_HEADER_API_VALUE,
+                EVP_NEEDS_APP_KEY_HEADER_NAME: EVP_NEEDS_APP_KEY_HEADER_VALUE,
+            }
         elif self._requests_mode == REQUESTS_MODE.AGENTLESS_EVENTS:
             url = "https://api." + self._dd_site + SKIPPABLE_ENDPOINT
         else:
diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt
@@ -142,6 +142,7 @@ mysql
 mysqlclient
 mysqldb
 namespace
+NeedsAppKey
 obfuscator
 openai
 opensearch
diff --git a/releasenotes/notes/fix-itr-in-evp-mode-e66fa0ef8a6ffe72.yaml b/releasenotes/notes/fix-itr-in-evp-mode-e66fa0ef8a6ffe72.yaml
@@ -0,0 +1,4 @@
+fixes:
+  - |
+    CI Visibility: fixes an issue where the Intelligent Test Runner would not work
+    when in EVP proxy mode due to missing ``X-Datadog-NeedsAppKey`` header.
diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py
@@ -1,3 +1,4 @@
+import json
 import os
 import time
 
@@ -342,7 +343,10 @@ def test_get_client_do_request_evp_proxy_headers():
         )
 
     _request.assert_called_once_with(
-        "POST", "http://base_url/repository/endpoint", "payload", {"X-Datadog-EVP-Subdomain": "api"}
+        "POST",
+        "http://base_url/repository/endpoint",
+        "payload",
+        {"X-Datadog-EVP-Subdomain": "api", "X-Datadog-NeedsAppKey": "true"},
     )
 
 
@@ -417,6 +421,74 @@ def test_git_client_upload_packfiles(git_repo):
             assert call_kwargs["headers"] == {"Content-Type": "multipart/form-data; boundary=----------boundary------"}
 
 
+def test_git_do_request_agentless(git_repo):
+    mock_serializer = CIVisibilityGitClientSerializerV1("fakeapikey", "fakeappkey")
+    response = mock.MagicMock()
+    setattr(response, "status", 200)
+
+    with mock.patch("ddtrace.internal.ci_visibility.git_client.get_connection") as mock_get_connection:
+        with mock.patch("ddtrace.internal.compat.get_connection_response", return_value=response):
+            mock_http_connection = mock.Mock()
+            mock_http_connection.request = mock.Mock()
+            mock_http_connection.close = mock.Mock()
+            mock_get_connection.return_value = mock_http_connection
+
+            CIVisibilityGitClient._do_request(
+                REQUESTS_MODE.AGENTLESS_EVENTS,
+                "base_url",
+                "endpoint",
+                '{"payload": "payload"}',
+                mock_serializer,
+                {"mock_header_name": "mock_header_value"},
+            )
+
+            mock_get_connection.assert_called_once_with("base_url/repositoryendpoint")
+            mock_http_connection.request.assert_called_once_with(
+                "POST",
+                "base_url/repositoryendpoint",
+                '{"payload": "payload"}',
+                {
+                    "dd-api-key": "fakeapikey",
+                    "dd-application-key": "fakeappkey",
+                    "mock_header_name": "mock_header_value",
+                },
+            )
+
+
+def test_git_do_request_evp(git_repo):
+    mock_serializer = CIVisibilityGitClientSerializerV1("foo", "bar")
+    response = mock.MagicMock()
+    setattr(response, "status", 200)
+
+    with mock.patch("ddtrace.internal.ci_visibility.git_client.get_connection") as mock_get_connection:
+        with mock.patch("ddtrace.internal.compat.get_connection_response", return_value=response):
+            mock_http_connection = mock.Mock()
+            mock_http_connection.request = mock.Mock()
+            mock_http_connection.close = mock.Mock()
+            mock_get_connection.return_value = mock_http_connection
+
+            CIVisibilityGitClient._do_request(
+                REQUESTS_MODE.EVP_PROXY_EVENTS,
+                "base_url",
+                "endpoint",
+                '{"payload": "payload"}',
+                mock_serializer,
+                {"mock_header_name": "mock_header_value"},
+            )
+
+            mock_get_connection.assert_called_once_with("base_url/repositoryendpoint")
+            mock_http_connection.request.assert_called_once_with(
+                "POST",
+                "base_url/repositoryendpoint",
+                '{"payload": "payload"}',
+                {
+                    "X-Datadog-NeedsAppKey": "true",
+                    "X-Datadog-EVP-Subdomain": "api",
+                    "mock_header_name": "mock_header_value",
+                },
+            )
+
+
 def test_civisibilitywriter_agentless_url():
     with override_env(dict(DD_API_KEY="foobar.baz")):
         with override_global_config({"_ci_visibility_agentless_url": "https://foo.bar"}):
@@ -521,6 +593,106 @@ def test_civisibilitywriter_only_traces(_check_enabled_features):
         CIVisibility.disable()
 
 
+def test_civisibility_check_enabled_features_agentless_do_request_called_correctly():
+    with mock.patch("ddtrace.internal.ci_visibility.recorder.uuid4", return_value="checkoutmyuuid4"):
+        with mock.patch("ddtrace.internal.ci_visibility.recorder._do_request") as mock_do_request:
+            mock_do_request.return_value = Response(
+                status=200,
+                body='{"data":{"id":"1234","type":"ci_app_tracers_test_service_settings","attributes":'
+                '{"code_coverage":true,"tests_skipping":true}}}',
+            )
+            with mock.patch.object(CIVisibility, "__init__", return_value=None):
+                mock_civisibilty = CIVisibility()
+
+                ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
+                mock_civisibilty._requests_mode = REQUESTS_MODE.AGENTLESS_EVENTS
+                mock_civisibilty._service = "service"
+                mock_civisibilty._api_key = "myfakeapikey"
+                mock_civisibilty._app_key = "myfakeappkey"
+                mock_civisibilty._dd_site = "datad0g.com"
+                mock_civisibilty._tags = {
+                    ci.git.REPOSITORY_URL: "my_repo_url",
+                    ci.git.COMMIT_SHA: "mycommitshaaaaaaalalala",
+                    ci.git.BRANCH: "notmain",
+                }
+
+                enabled_features = mock_civisibilty._check_enabled_features()
+
+                mock_do_request.assert_called_once()
+                do_request_call_args = mock_do_request.call_args[0]
+                do_request_payload = json.loads(do_request_call_args[2])
+
+                assert do_request_call_args[0] == "POST"
+                assert do_request_call_args[1] == "https://api.datad0g.com/api/v2/libraries/tests/services/setting"
+                assert do_request_call_args[3] == {
+                    "dd-api-key": "myfakeapikey",
+                    "dd-application-key": "myfakeappkey",
+                    "Content-Type": "application/json",
+                }
+                assert do_request_payload == {
+                    "data": {
+                        "id": "checkoutmyuuid4",
+                        "type": "ci_app_test_service_libraries_settings",
+                        "attributes": {
+                            "service": "service",
+                            "env": None,
+                            "repository_url": "my_repo_url",
+                            "sha": "mycommitshaaaaaaalalala",
+                            "branch": "notmain",
+                        },
+                    }
+                }
+                assert enabled_features == (True, True)
+
+
+def test_civisibility_check_enabled_features_evp_do_request_called_correctly():
+    with mock.patch("ddtrace.internal.ci_visibility.recorder.uuid4", return_value="checkoutmyuuid4"):
+        with mock.patch("ddtrace.internal.ci_visibility.recorder._do_request") as mock_do_request:
+            mock_do_request.return_value = Response(
+                status=200,
+                body='{"data":{"id":"1234","type":"ci_app_tracers_test_service_settings","attributes":'
+                '{"code_coverage":true,"tests_skipping":true}}}',
+            )
+            with mock.patch.object(CIVisibility, "__init__", return_value=None):
+                mock_civisibilty = CIVisibility()
+
+                ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
+                mock_civisibilty._requests_mode = REQUESTS_MODE.EVP_PROXY_EVENTS
+                mock_civisibilty._service = "service"
+                mock_civisibilty._tags = {
+                    ci.git.REPOSITORY_URL: "my_repo_url",
+                    ci.git.COMMIT_SHA: "mycommitshaaaaaaalalala",
+                    ci.git.BRANCH: "notmain",
+                }
+
+                enabled_features = mock_civisibilty._check_enabled_features()
+
+                mock_do_request.assert_called_once()
+                do_request_call_args = mock_do_request.call_args[0]
+                do_request_payload = json.loads(do_request_call_args[2])
+
+                assert do_request_call_args[0] == "POST"
+                assert (
+                    do_request_call_args[1]
+                    == "http://localhost:8126/evp_proxy/v2/api/v2/libraries/tests/services/setting"
+                )
+                assert do_request_call_args[3] == {"X-Datadog-EVP-Subdomain": "api", "X-Datadog-NeedsAppKey": "true"}
+                assert do_request_payload == {
+                    "data": {
+                        "id": "checkoutmyuuid4",
+                        "type": "ci_app_test_service_libraries_settings",
+                        "attributes": {
+                            "service": "service",
+                            "env": None,
+                            "repository_url": "my_repo_url",
+                            "sha": "mycommitshaaaaaaalalala",
+                            "branch": "notmain",
+                        },
+                    }
+                }
+                assert enabled_features == (True, True)
+
+
 @mock.patch("ddtrace.internal.ci_visibility.recorder._do_request")
 def test_civisibility_check_enabled_features_no_app_key_request_not_called(_do_request):
     with override_env(

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +fixes:
 +  - |
 +    CI Visibility: fixes an issue where the Intelligent Test Runner would not work
 +    when in EVP proxy mode due to missing ``X-Datadog-NeedsAppKey`` header.