feat: warn if default ingress_settings is used in remote_functions (#1419)

* feat: warn if default ingress_settings is used in remote_functions

* Apply suggestions from code review

Co-authored-by: Tim Sweña (Swast) <swast@google.com>

* add tests to verify warning

* Update bigframes/functions/_function_session.py

Co-authored-by: Shobhit Singh <shobs@google.com>

---------

Co-authored-by: Tim Sweña (Swast) <swast@google.com>
Co-authored-by: Shobhit Singh <shobs@google.com>

Author: 3 people

bigframes/functions/_function_session.py

@@ -121,9 +121,9 @@ def remote_function(
         cloud_function_max_instances: Optional[int] = None,
         cloud_function_vpc_connector: Optional[str] = None,
         cloud_function_memory_mib: Optional[int] = 1024,
-        cloud_function_ingress_settings: Literal[
-            "all", "internal-only", "internal-and-gclb"
-        ] = "all",
+        cloud_function_ingress_settings: Optional[
+            Literal["all", "internal-only", "internal-and-gclb"]
+        ] = None,
     ):
         """Decorator to turn a user defined function into a BigQuery remote function.
 
@@ -311,8 +311,9 @@ def remote_function(
                 https://cloud.google.com/functions/docs/configuring/memory.
             cloud_function_ingress_settings (str, Optional):
                 Ingress settings controls dictating what traffic can reach the
-                function. By default `all` will be used. It must be one of:
-                `all`, `internal-only`, `internal-and-gclb`. See for more details
+                function. Options are: `all`, `internal-only`, or `internal-and-gclb`.
+                If no setting is provided, `all` will be used by default and a warning
+                will be issued. See for more details
                 https://cloud.google.com/functions/docs/networking/network-settings#ingress_settings.
         """
         # Some defaults may be used from the session if not provided otherwise
@@ -429,6 +430,16 @@ def remote_function(
                 " For more details see https://cloud.google.com/functions/docs/securing/cmek#before_you_begin"
             )
 
+        if cloud_function_ingress_settings is None:
+            cloud_function_ingress_settings = "all"
+            msg = (
+                "The `cloud_function_ingress_settings` are set to 'all' by default, "
+                "which will change to 'internal-only' for enhanced security in future version 2.0 onwards. "
+                "However, you will be able to explicitly pass cloud_function_ingress_settings='all' if you need. "
+                "See https://cloud.google.com/functions/docs/networking/network-settings#ingress_settings for details."
+            )
+            warnings.warn(msg, category=FutureWarning, stacklevel=2)
+
         bq_connection_manager = session.bqconnectionmanager
 
         def wrapper(func):

bigframes/pandas/__init__.py

@@ -80,9 +80,9 @@ def remote_function(
     cloud_function_max_instances: Optional[int] = None,
     cloud_function_vpc_connector: Optional[str] = None,
     cloud_function_memory_mib: Optional[int] = 1024,
-    cloud_function_ingress_settings: Literal[
-        "all", "internal-only", "internal-and-gclb"
-    ] = "all",
+    cloud_function_ingress_settings: Optional[
+        Literal["all", "internal-only", "internal-and-gclb"]
+    ] = None,
 ):
     return global_session.with_default_session(
         bigframes.session.Session.remote_function,

bigframes/session/__init__.py

@@ -1199,9 +1199,9 @@ def remote_function(
         cloud_function_max_instances: Optional[int] = None,
         cloud_function_vpc_connector: Optional[str] = None,
         cloud_function_memory_mib: Optional[int] = 1024,
-        cloud_function_ingress_settings: Literal[
-            "all", "internal-only", "internal-and-gclb"
-        ] = "all",
+        cloud_function_ingress_settings: Optional[
+            Literal["all", "internal-only", "internal-and-gclb"]
+        ] = None,
     ):
         """Decorator to turn a user defined function into a BigQuery remote function. Check out
         the code samples at: https://cloud.google.com/bigquery/docs/remote-functions#bigquery-dataframes.
@@ -1373,8 +1373,9 @@ def remote_function(
                 https://cloud.google.com/functions/docs/configuring/memory.
             cloud_function_ingress_settings (str, Optional):
                 Ingress settings controls dictating what traffic can reach the
-                function. By default `all` will be used. It must be one of:
-                `all`, `internal-only`, `internal-and-gclb`. See for more details
+                function. Options are: `all`, `internal-only`, or `internal-and-gclb`.
+                If no setting is provided, `all` will be used by default and a warning
+                will be issued. See for more details
                 https://cloud.google.com/functions/docs/networking/network-settings#ingress_settings.
         Returns:
             collections.abc.Callable:

tests/system/large/functions/test_remote_function.py

@@ -21,6 +21,7 @@
 import sys
 import tempfile
 import textwrap
+import warnings
 
 import google.api_core.exceptions
 from google.cloud import bigquery, functions_v2, storage
@@ -2363,40 +2364,64 @@ def generate_stats(row: pandas.Series) -> list[int]:
 
 
 @pytest.mark.parametrize(
-    ("ingress_settings_args", "effective_ingress_settings"),
+    ("ingress_settings_args", "effective_ingress_settings", "expected_warning"),
     [
         pytest.param(
-            {}, functions_v2.ServiceConfig.IngressSettings.ALLOW_ALL, id="no-set"
+            {},
+            functions_v2.ServiceConfig.IngressSettings.ALLOW_ALL,
+            FutureWarning,
+            id="no-set",
+        ),
+        pytest.param(
+            {"cloud_function_ingress_settings": None},
+            functions_v2.ServiceConfig.IngressSettings.ALLOW_ALL,
+            FutureWarning,
+            id="set-none",
         ),
         pytest.param(
             {"cloud_function_ingress_settings": "all"},
             functions_v2.ServiceConfig.IngressSettings.ALLOW_ALL,
+            None,
             id="set-all",
         ),
         pytest.param(
             {"cloud_function_ingress_settings": "internal-only"},
             functions_v2.ServiceConfig.IngressSettings.ALLOW_INTERNAL_ONLY,
+            None,
             id="set-internal-only",
         ),
         pytest.param(
             {"cloud_function_ingress_settings": "internal-and-gclb"},
             functions_v2.ServiceConfig.IngressSettings.ALLOW_INTERNAL_AND_GCLB,
+            None,
             id="set-internal-and-gclb",
         ),
     ],
 )
 @pytest.mark.flaky(retries=2, delay=120)
 def test_remote_function_ingress_settings(
-    session, scalars_dfs, ingress_settings_args, effective_ingress_settings
+    session,
+    scalars_dfs,
+    ingress_settings_args,
+    effective_ingress_settings,
+    expected_warning,
 ):
     try:
+        # Verify the function raises the expected security warning message.
+        with warnings.catch_warnings(record=True) as w:
 
-        def square(x: int) -> int:
-            return x * x
+            def square(x: int) -> int:
+                return x * x
 
-        square_remote = session.remote_function(reuse=False, **ingress_settings_args)(
-            square
-        )
+            square_remote = session.remote_function(
+                reuse=False, **ingress_settings_args
+            )(square)
+
+            if expected_warning is not None:
+                assert issubclass(w[0].category, FutureWarning)
+                assert "Consider using 'internal-only' for enhanced security." in str(
+                    w[0].message
+                )
 
         # Assert that the GCF is created with the intended maximum timeout
         gcf = session.cloudfunctionsclient.get_function(