Skip to content

Commit 4a35fba

Browse files
aipcc-botntkathole
aipcc-bot
authored and
ntkathole
committed
fix: Address review feedback on FIPS cipher suite configuration
# What this PR does / why we need it: Move FIPS log message below logger initialization so it is emitted at INFO level instead of being silently dropped under the default WARNING threshold. Replace importlib.reload-based import ordering test with a subprocess-based test that exercises fresh Python import from scratch, ensuring pyarrow.flight is not cached in sys.modules and the GRPC_SSL_CIPHER_SUITES ordering check is genuine. # Which issue(s) this PR fixes: Fixes RHOAIENG-78595 # Checks - [x] I've made sure the tests are passing. - [x] My PR title follows conventional commits format ## Testing Strategy - [x] Unit tests Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Jitendra Yejare <11752425+jyejare@users.noreply.github.com>
1 parent 979b82a commit 4a35fba

2 files changed

Lines changed: 56 additions & 18 deletions

File tree

‎sdk/python/feast/offline_server.py‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,17 @@ def _is_fips_enabled() -> bool:
2727
return False
2828

2929

30-
def _configure_grpc_fips() -> None:
30+
def _configure_grpc_fips() -> bool:
3131
if _is_fips_enabled() and "GRPC_SSL_CIPHER_SUITES" not in os.environ:
3232
os.environ["GRPC_SSL_CIPHER_SUITES"] = _FIPS_CIPHER_SUITES
33-
logging.getLogger(__name__).info(
34-
"FIPS mode detected, configured FIPS-compliant gRPC cipher suites."
35-
)
33+
return True
34+
return False
3635

3736

3837
# On FIPS-enabled systems (notably IBM Power ppc64le), gRPC reads
3938
# GRPC_SSL_CIPHER_SUITES during shared-library initialization. The env var
4039
# must be set before any gRPC-linked module (pyarrow.flight) is imported.
41-
_configure_grpc_fips()
40+
_fips_configured = _configure_grpc_fips()
4241

4342
import click # noqa: E402
4443
import pyarrow as pa # noqa: E402
@@ -77,6 +76,9 @@ def _configure_grpc_fips() -> None:
7776
logger = logging.getLogger(__name__)
7877
logger.setLevel(logging.INFO)
7978

79+
if _fips_configured:
80+
logger.info("FIPS mode detected, configured FIPS-compliant gRPC cipher suites.")
81+
8082

8183
class OfflineServer(fl.FlightServerBase):
8284
def __init__(

‎sdk/python/tests/unit/test_offline_server.py‎

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1-
import importlib
21
import os
2+
import subprocess
3+
import sys
4+
import textwrap
35
from unittest.mock import MagicMock, mock_open, patch
46

57
import assertpy
@@ -142,17 +144,51 @@ def test_configure_grpc_fips_noop_without_fips():
142144

143145
def test_module_level_fips_sets_env_before_pyarrow_import():
144146
"""GRPC_SSL_CIPHER_SUITES must be set at module load time,
145-
before pyarrow.flight (which bundles gRPC) is imported."""
146-
env_backup = os.environ.pop("GRPC_SSL_CIPHER_SUITES", None)
147-
try:
148-
with patch("builtins.open", mock_open(read_data="1\n")):
149-
import feast.offline_server as mod
150-
151-
importlib.reload(mod)
147+
before pyarrow.flight (which bundles gRPC) is imported.
148+
149+
Uses a subprocess so pyarrow.flight is not already cached in
150+
sys.modules, which lets us verify the true import ordering.
151+
"""
152+
script = textwrap.dedent("""\
153+
import io, os, sys
154+
155+
# Intercept only /proc/sys/crypto/fips_enabled to simulate FIPS
156+
_real_open = open
157+
def _fips_open(file, *args, **kwargs):
158+
if str(file) == "/proc/sys/crypto/fips_enabled":
159+
return io.StringIO("1\\n")
160+
return _real_open(file, *args, **kwargs)
161+
162+
import builtins
163+
builtins.open = _fips_open
164+
165+
# Track import order to verify env var is set before pyarrow.flight
166+
original_import = builtins.__import__
167+
def tracking_import(name, *args, **kwargs):
168+
if name == "pyarrow.flight":
169+
assert "GRPC_SSL_CIPHER_SUITES" in os.environ, (
170+
"GRPC_SSL_CIPHER_SUITES not set before pyarrow.flight import"
171+
)
172+
return original_import(name, *args, **kwargs)
173+
174+
builtins.__import__ = tracking_import
175+
try:
176+
import feast.offline_server
152177
assert "GRPC_SSL_CIPHER_SUITES" in os.environ
153178
assert "AES128-GCM-SHA256" in os.environ["GRPC_SSL_CIPHER_SUITES"]
154-
finally:
155-
if env_backup is not None:
156-
os.environ["GRPC_SSL_CIPHER_SUITES"] = env_backup
157-
else:
158-
os.environ.pop("GRPC_SSL_CIPHER_SUITES", None)
179+
finally:
180+
builtins.__import__ = original_import
181+
builtins.open = _real_open
182+
""")
183+
env = os.environ.copy()
184+
env.pop("GRPC_SSL_CIPHER_SUITES", None)
185+
result = subprocess.run(
186+
[sys.executable, "-c", script],
187+
capture_output=True,
188+
text=True,
189+
env=env,
190+
timeout=60,
191+
)
192+
assert result.returncode == 0, (
193+
f"Subprocess failed:\nstdout: {result.stdout}\nstderr: {result.stderr}"
194+
)

0 commit comments

Comments
 (0)