logs: allow without strict logging (PROJQUAY-7116) (quay#2846)

* allow to disable strict logging in general Signed-off-by: dmesser <dmesser@redhat.com> * add strict logging exceptions for log kinds during reads Signed-off-by: dmesser <dmesser@redhat.com> * add strict logging exceptions for log kinds during reads Signed-off-by: dmesser <dmesser@redhat.com> * formatting Signed-off-by: dmesser <dmesser@redhat.com> --------- Signed-off-by: dmesser <dmesser@redhat.com>
Marcusk19 · Jun 17, 2024 · 99d571a · 99d571a
1 parent e0573f9
commit 99d571a
Show file tree

Hide file tree

Showing 10 changed files with 214 additions and 43 deletions.
diff --git a/config-tool/utils/scripts/dumpschema.py b/config-tool/utils/scripts/dumpschema.py
@@ -182,6 +182,13 @@
             + "and it is desired for pulls to continue during that time. Defaults to False.",
             "x-example": True,
         },
+        "ALLOW_WITHOUT_STRICT_LOGGING": {
+            "type": "boolean",
+            "description": "If true, any action in which the audit log entry cannot be written will "
+            + "still succeed. Useful if using an external logging service that may be down "
+            + "intermittently and the registry should continue to work. Defaults to False.",
+            "x-example": False,
+        },
         # Storage.
         "FEATURE_STORAGE_REPLICATION": {
             "type": "boolean",

diff --git a/config.py b/config.py
@@ -456,6 +456,9 @@ def create_transaction(db):
     # Allow registry pulls when unable to write to the audit log
     ALLOW_PULLS_WITHOUT_STRICT_LOGGING = False
 
+    # Allow any registry action when unable to write to the audit log
+    ALLOW_WITHOUT_STRICT_LOGGING = False
+
     # Temporary tag expiration in seconds, this may actually be longer based on GC policy
     PUSH_TEMP_TAG_EXPIRATION_SEC = 60 * 60  # One hour per layer
 

diff --git a/data/logs_model/document_logs_model.py b/data/logs_model/document_logs_model.py
@@ -511,9 +511,15 @@ def log_action(
         try:
             self._logs_producer.send(log)
         except LogSendException as lse:
-            strict_logging_disabled = config.app_config.get("ALLOW_PULLS_WITHOUT_STRICT_LOGGING")
-            logger.exception("log_action failed", extra=({"exception": lse}).update(log.to_dict()))
-            if not (strict_logging_disabled and kind_name in ACTIONS_ALLOWED_WITHOUT_AUDIT_LOGGING):
+            strict_logging_disabled = config.app_config.get("ALLOW_WITHOUT_STRICT_LOGGING") or (
+                config.app_config.get("ALLOW_PULLS_WITHOUT_STRICT_LOGGING")
+                and kind_name in ACTIONS_ALLOWED_WITHOUT_AUDIT_LOGGING
+            )
+            if strict_logging_disabled:
+                logger.exception(
+                    "log_action failed", extra=({"exception": lse}).update(log.to_dict())
+                )
+            else:
                 raise
 
     def yield_logs_for_export(

diff --git a/data/logs_model/splunk_logs_model.py b/data/logs_model/splunk_logs_model.py
@@ -94,9 +94,13 @@ def log_action(
         try:
             self._logs_producer.send(log_data)
         except LogSendException as lse:
-            strict_logging_disabled = config.app_config.get("ALLOW_PULLS_WITHOUT_STRICT_LOGGING")
-            logger.exception("log_action failed", extra=({"exception": lse}).update(log_data))
-            if not (strict_logging_disabled and kind_name in ACTIONS_ALLOWED_WITHOUT_AUDIT_LOGGING):
+            strict_logging_disabled = config.app_config.get("ALLOW_WITHOUT_STRICT_LOGGING") or (
+                config.app_config.get("ALLOW_PULLS_WITHOUT_STRICT_LOGGING")
+                and kind_name in ACTIONS_ALLOWED_WITHOUT_AUDIT_LOGGING
+            )
+            if strict_logging_disabled:
+                logger.exception("log_action failed", extra=({"exception": lse}).update(log_data))
+            else:
                 raise
 
     def lookup_logs(

diff --git a/data/logs_model/test/test_elasticsearch.py b/data/logs_model/test/test_elasticsearch.py
@@ -10,6 +10,7 @@
 from httmock import HTTMock, urlmatch
 from mock import Mock, patch
 
+from .mock_elasticsearch import *
 from data.logs_model import LogsModelProxy, configure
 from data.logs_model.elastic_logs import (
     INDEX_DATE_FORMAT,
@@ -18,8 +19,6 @@
 )
 from data.model.log import _json_serialize
 
-from .mock_elasticsearch import *
-
 FAKE_ES_HOST = "fakees"
 FAKE_ES_HOST_PATTERN = r"fakees.*"
 FAKE_ES_PORT = 443
@@ -210,16 +209,27 @@ def search(url, req):
 
 @pytest.mark.parametrize(
     """
-  unlogged_pulls_ok, kind_name, namespace_name, repository, repository_name,
+  unlogged_ok,unlogged_pulls_ok, kind_name, namespace_name, repository, repository_name,
   timestamp,
   index_response, expected_request, throws
   """,
     [
         # Invalid inputs
         pytest.param(
-            False, "non-existing", None, None, None, None, None, None, True, id="Invalid Kind"
+            False,
+            False,
+            "non-existing",
+            None,
+            None,
+            None,
+            None,
+            None,
+            None,
+            True,
+            id="Invalid Kind",
         ),
         pytest.param(
+            False,
             False,
             "pull_repo",
             "user1",
@@ -233,6 +243,7 @@ def search(url, req):
         ),
         # Remote exceptions
         pytest.param(
+            False,
             False,
             "pull_repo",
             "user1",
@@ -244,8 +255,22 @@ def search(url, req):
             True,
             id="Throw on pull log failure",
         ),
+        pytest.param(
+            False,
+            True,
+            "pull_repo",
+            "user1",
+            Mock(id=1),
+            None,
+            parse("2017-03-08T03:30"),
+            FAILURE_400,
+            INDEX_REQUEST_2017_03_08,
+            False,
+            id="Ok on pull log failure",
+        ),
         pytest.param(
             True,
+            False,
             "pull_repo",
             "user1",
             Mock(id=1),
@@ -256,8 +281,22 @@ def search(url, req):
             False,
             id="Ok on pull log failure",
         ),
+        pytest.param(
+            True,
+            False,
+            "push_repo",
+            "user1",
+            None,
+            "repo1",
+            parse("2019-01-01T03:30"),
+            FAILURE_400,
+            INDEX_REQUEST_2019_01_01,
+            False,
+            id="Ok on push log failure",
+        ),
         # Success executions
         pytest.param(
+            False,
             False,
             "pull_repo",
             "user1",
@@ -270,6 +309,7 @@ def search(url, req):
             id="Log with namespace name and repository",
         ),
         pytest.param(
+            False,
             False,
             "push_repo",
             "user1",
@@ -284,6 +324,7 @@ def search(url, req):
     ],
 )
 def test_log_action(
+    unlogged_ok,
     unlogged_pulls_ok,
     kind_name,
     namespace_name,
@@ -302,6 +343,7 @@ def test_log_action(
     mock_elasticsearch.template = Mock(return_value=DEFAULT_TEMPLATE_RESPONSE)
     mock_elasticsearch.index = Mock(return_value=index_response)
     app_config["ALLOW_PULLS_WITHOUT_STRICT_LOGGING"] = unlogged_pulls_ok
+    app_config["ALLOW_WITHOUT_STRICT_LOGGING"] = unlogged_ok
     configure(app_config)
 
     performer = Mock(id=1)

diff --git a/data/logs_model/test/test_splunk.py b/data/logs_model/test/test_splunk.py
@@ -15,6 +15,8 @@
 from ..logs_producer.splunk_logs_producer import SplunkLogsProducer
 from .test_elasticsearch import logs_model, mock_db_model
 from data.logs_model import configure
+from data.logs_model.logs_producer import LogSendException
+from data.model import config as _config
 from test.fixtures import *
 
 FAKE_SPLUNK_HOST = "fakesplunk"
@@ -48,6 +50,12 @@
 }
 
 
+@pytest.fixture(scope="function")
+def app_config():
+    with patch.dict(_config.app_config, {}, clear=True):
+        yield _config.app_config
+
+
 @pytest.fixture()
 def splunk_logs_model_config():
     conf = {
@@ -105,11 +113,29 @@ def cert_file_path():
 
 @pytest.mark.parametrize(
     """
-    kind_name, namespace_name,
-    performer, ip, metadata, repository, repository_name, timestamp, throws
+    unlogged_ok, unlogged_pulls_ok, kind_name, namespace_name, performer, ip,
+    metadata, repository, repository_name, timestamp, throws, send_exception
     """,
     [
+        # logs a push_repo action
+        pytest.param(
+            False,
+            False,
+            "push_repo",
+            "devtable",
+            FAKE_PERFORMER["user1"],
+            "192.168.1.1",
+            {"key": "value"},
+            None,
+            "repo1",
+            parse("2019-01-01T03:30"),
+            False,
+            None,
+        ),
+        # doesn't raise a failed push_repo action
         pytest.param(
+            True,
+            False,
             "push_repo",
             "devtable",
             FAKE_PERFORMER["user1"],
@@ -119,9 +145,42 @@ def cert_file_path():
             "repo1",
             parse("2019-01-01T03:30"),
             False,
+            LogSendException("Failed to send log data"),
+        ),
+        # doesn't raise a failed pull_repo action
+        pytest.param(
+            False,
+            True,
+            "pull_repo",
+            "devtable",
+            FAKE_PERFORMER["user1"],
+            "192.168.1.1",
+            {"key": "value"},
+            None,
+            "repo1",
+            parse("2019-01-01T03:30"),
+            False,
+            LogSendException("Failed to send log data"),
+        ),
+        # raise a failed pull_repo action
+        pytest.param(
+            False,
+            False,
+            "pull_repo",
+            "devtable",
+            FAKE_PERFORMER["user1"],
+            "192.168.1.1",
+            {"key": "value"},
+            None,
+            "repo1",
+            parse("2019-01-01T03:30"),
+            True,
+            LogSendException("Failed to send log data"),
         ),
         # raises ValueError when repository_name is not None and repository is not None
         pytest.param(
+            False,
+            False,
             "pull_repo",
             "devtable",
             FAKE_PERFORMER["user1"],
@@ -131,9 +190,12 @@ def cert_file_path():
             "repo1",
             parse("2019-01-01T03:30"),
             True,
+            None,
         ),
         # raises exception when no namespace is given
         pytest.param(
+            False,
+            False,
             "pull_repo",
             None,
             FAKE_PERFORMER["user1"],
@@ -143,10 +205,13 @@ def cert_file_path():
             "user1/repo1",
             parse("2019-01-01T03:30"),
             True,
+            None,
         ),
     ],
 )
-def test_splunk_logs_producer(
+def test_splunk_logs_producers(
+    unlogged_ok,
+    unlogged_pulls_ok,
     kind_name,
     namespace_name,
     performer,
@@ -156,12 +221,17 @@ def test_splunk_logs_producer(
     repository_name,
     timestamp,
     throws,
+    send_exception,
     logs_model,
     splunk_logs_model_config,
     mock_db_model,
     initialized_db,
     cert_file_path,
+    app_config,
 ):
+    app_config["ALLOW_PULLS_WITHOUT_STRICT_LOGGING"] = unlogged_pulls_ok
+    app_config["ALLOW_WITHOUT_STRICT_LOGGING"] = unlogged_ok
+
     with (
         patch(
             "data.logs_model.logs_producer.splunk_logs_producer.SplunkLogsProducer.send"
@@ -170,23 +240,40 @@ def test_splunk_logs_producer(
     ):
         with patch("ssl.SSLContext.load_verify_locations"):
             configure(splunk_logs_model_config)
+
+            if send_exception:
+                mock_send.side_effect = send_exception
+
             if throws:
-                with pytest.raises(
-                    ValueError,
-                    match=r"Incorrect argument provided when logging action logs, "
-                    r"namespace name should not be empty",
-                ):
-                    logs_model.log_action(
-                        kind_name,
-                        namespace_name,
-                        performer,
-                        ip,
-                        metadata,
-                        repository,
-                        repository_name,
-                        timestamp,
-                    )
-                mock_send.assert_not_called()
+                if not send_exception:
+                    with pytest.raises(
+                        ValueError,
+                        match=r"Incorrect argument provided when logging action logs, "
+                        r"namespace name should not be empty",
+                    ):
+                        logs_model.log_action(
+                            kind_name,
+                            namespace_name,
+                            performer,
+                            ip,
+                            metadata,
+                            repository,
+                            repository_name,
+                            timestamp,
+                        )
+                    mock_send.assert_not_called()
+                else:
+                    with pytest.raises(LogSendException):
+                        logs_model.log_action(
+                            kind_name,
+                            namespace_name,
+                            performer,
+                            ip,
+                            metadata,
+                            repository,
+                            repository_name,
+                            timestamp,
+                        )
             else:
                 logs_model.log_action(
                     kind_name,
@@ -203,7 +290,7 @@ def test_splunk_logs_producer(
                     "account": "devtable",
                     "datetime": datetime(2019, 1, 1, 3, 30),
                     "ip": "192.168.1.1",
-                    "kind": "push_repo",
+                    "kind": kind_name,
                     "metadata_json": {"key": "value"},
                     "performer": "fake_username",
                     "repository": None,