From dd4a8d6a838fd13ef74be85130739837e6a828ed Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:55:16 -0400 Subject: [PATCH 1/9] Adding ssl support for registry server. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- .../feature-servers/python-feature-server.md | 2 +- sdk/python/feast/cli.py | 29 ++++++++++++++++-- sdk/python/feast/feature_store.py | 8 +++-- sdk/python/feast/infra/registry/remote.py | 15 +++++++++- sdk/python/feast/registry_server.py | 30 +++++++++++++++++-- 5 files changed, 76 insertions(+), 8 deletions(-) diff --git a/docs/reference/feature-servers/python-feature-server.md b/docs/reference/feature-servers/python-feature-server.md index bdba3678337..0400a5fe229 100644 --- a/docs/reference/feature-servers/python-feature-server.md +++ b/docs/reference/feature-servers/python-feature-server.md @@ -219,7 +219,7 @@ The above command will generate two files To start the feature server in SSL mode, you need to provide the private and public keys using the `--ssl-key-path` and `--ssl-cert-path` arguments with the `feast serve` command. ```shell -feast serve --ssl-key-path key.pem --ssl-cert-path cert.pem +feast serve --ssl-key-path /path/to/key.pem --ssl-cert-path /path/to/cert.pem ``` # Online Feature Server Permissions and Access Control diff --git a/sdk/python/feast/cli.py b/sdk/python/feast/cli.py index ecb307b6069..4ba6acc3ee5 100644 --- a/sdk/python/feast/cli.py +++ b/sdk/python/feast/cli.py @@ -1035,12 +1035,37 @@ def serve_transformations_command(ctx: click.Context, port: int): default=DEFAULT_REGISTRY_SERVER_PORT, help="Specify a port for the server", ) +@click.option( + "--ssl-key-path", + "-k", + type=click.STRING, + default="", + show_default=False, + help="path to SSL certificate private key. You need to pass ssl-cert-path as well to start server in SSL mode", +) +@click.option( + "--ssl-cert-path", + "-c", + type=click.STRING, + default="", + show_default=False, + help="path to SSL certificate public key. You need to pass ssl-key-path as well to start server in SSL mode", +) @click.pass_context -def serve_registry_command(ctx: click.Context, port: int): +def serve_registry_command( + ctx: click.Context, + port: int, + ssl_key_path: str, + ssl_cert_path: str, +): """Start a registry server locally on a given port.""" + if (ssl_key_path and not ssl_cert_path) or (not ssl_key_path and ssl_cert_path): + raise click.BadParameter( + "Please configure ssl-cert-path and ssl-key-path args to start the registry server in SSL mode." + ) store = create_feature_store(ctx) - store.serve_registry(port) + store.serve_registry(port, ssl_key_path, ssl_cert_path) @cli.command("serve_offline") diff --git a/sdk/python/feast/feature_store.py b/sdk/python/feast/feature_store.py index 876345c8bbb..06e860cfb81 100644 --- a/sdk/python/feast/feature_store.py +++ b/sdk/python/feast/feature_store.py @@ -1949,11 +1949,15 @@ def serve_ui( root_path=root_path, ) - def serve_registry(self, port: int) -> None: + def serve_registry( + self, port: int, ssl_key_path: str = "", ssl_cert_path: str = "" + ) -> None: """Start registry server locally on a given port.""" from feast import registry_server - registry_server.start_server(self, port) + registry_server.start_server( + self, port=port, ssl_key_path=ssl_key_path, ssl_cert_path=ssl_cert_path + ) def serve_offline(self, host: str, port: int) -> None: """Start offline server locally on a given port.""" diff --git a/sdk/python/feast/infra/registry/remote.py b/sdk/python/feast/infra/registry/remote.py index 424c59c57d3..eb8f54a6372 100644 --- a/sdk/python/feast/infra/registry/remote.py +++ b/sdk/python/feast/infra/registry/remote.py @@ -55,6 +55,10 @@ class RemoteRegistryConfig(RegistryConfig): """ str: Path to metadata store. If registry_type is 'remote', then this is a URL for registry server """ + ssl_cert_path: StrictStr = "" + """ str: Path to the public certificate when the registry server starts in SSL mode. This may be needed if the registry server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. + If registry_type is 'remote', then this configuration is needed to connect to remote registry server in SSL mode. """ + class RemoteRegistry(BaseRegistry): def __init__( @@ -65,7 +69,16 @@ def __init__( auth_config: AuthConfig = NoAuthConfig(), ): self.auth_config = auth_config - self.channel = grpc.insecure_channel(registry_config.path) + if registry_config.ssl_cert_path: + with open("server.crt", "rb") as cert_file: + trusted_certs = cert_file.read() + ssl_credentials = grpc.ssl_channel_credentials( + root_certificates=trusted_certs + ) + self.channel = grpc.secure_channel(registry_config.path, ssl_credentials) + else: + self.channel = grpc.insecure_channel(registry_config.path) + auth_header_interceptor = GrpcClientAuthHeaderInterceptor(auth_config) self.channel = grpc.intercept_channel(self.channel, auth_header_interceptor) self.stub = RegistryServer_pb2_grpc.RegistryServerStub(self.channel) diff --git a/sdk/python/feast/registry_server.py b/sdk/python/feast/registry_server.py index c2f4a688d3b..4d77d687378 100644 --- a/sdk/python/feast/registry_server.py +++ b/sdk/python/feast/registry_server.py @@ -1,3 +1,4 @@ +import logging from concurrent import futures from datetime import datetime, timezone from typing import Optional, Union, cast @@ -38,6 +39,9 @@ from feast.saved_dataset import SavedDataset, ValidationReference from feast.stream_feature_view import StreamFeatureView +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + def _build_any_feature_view_proto(feature_view: BaseFeatureView): if isinstance(feature_view, StreamFeatureView): @@ -753,7 +757,13 @@ def Proto(self, request, context): return self.proxied_registry.proto() -def start_server(store: FeatureStore, port: int, wait_for_termination: bool = True): +def start_server( + store: FeatureStore, + port: int, + wait_for_termination: bool = True, + ssl_key_path: str = "", + ssl_cert_path: str = "", +): auth_manager_type = str_to_auth_manager_type(store.config.auth_config.type) init_security_manager(auth_type=auth_manager_type, fs=store) init_auth_manager( @@ -781,9 +791,25 @@ def start_server(store: FeatureStore, port: int, wait_for_termination: bool = Tr ) reflection.enable_server_reflection(service_names_available_for_reflection, server) - server.add_insecure_port(f"[::]:{port}") + if ssl_cert_path and ssl_key_path: + with open(ssl_cert_path, "rb") as cert_file, open( + ssl_key_path, "rb" + ) as key_file: + certificate_chain = cert_file.read() + private_key = key_file.read() + server_credentials = grpc.ssl_server_credentials( + ((private_key, certificate_chain),) + ) + logger.info("Starting grpc registry server in SSL mode") + server.add_secure_port(f"[::]:{port}", server_credentials) + else: + logger.info("Starting grpc registry server in non-SSL mode") + server.add_insecure_port(f"[::]:{port}") server.start() if wait_for_termination: + logger.info( + f"Grpc server started at {'https' if ssl_cert_path and ssl_key_path else 'http'}://localhost:{port}" + ) server.wait_for_termination() else: return server From b1672212a176d71421c5f09d4cef5c4bb92fbed1 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:55:16 -0400 Subject: [PATCH 2/9] Adding ssl support for registry server. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- sdk/python/feast/infra/registry/remote.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/python/feast/infra/registry/remote.py b/sdk/python/feast/infra/registry/remote.py index eb8f54a6372..579f371ae10 100644 --- a/sdk/python/feast/infra/registry/remote.py +++ b/sdk/python/feast/infra/registry/remote.py @@ -57,7 +57,7 @@ class RemoteRegistryConfig(RegistryConfig): ssl_cert_path: StrictStr = "" """ str: Path to the public certificate when the registry server starts in SSL mode. This may be needed if the registry server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. - If registry_type is 'remote', then this configuration is needed to connect to remote registry server in SSL mode. """ + If registry_type is 'remote', then this configuration is needed to connect to remote registry server in SSL mode. If the remote registry started in non-ssl mode then this configuration is not needed.""" class RemoteRegistry(BaseRegistry): @@ -69,6 +69,7 @@ def __init__( auth_config: AuthConfig = NoAuthConfig(), ): self.auth_config = auth_config + assert isinstance(registry_config, RemoteRegistryConfig) if registry_config.ssl_cert_path: with open("server.crt", "rb") as cert_file: trusted_certs = cert_file.read() From d5a0a7af642d2d02d0bc7b08e733014d0f281766 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:00:42 -0400 Subject: [PATCH 3/9] * Adding tests for the SSL changes related to registry server. * Refactored the ssl certificate generation logic to its own fixture so that we can reuse it. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- sdk/python/feast/infra/registry/remote.py | 2 +- sdk/python/tests/conftest.py | 17 ++++++++++ .../online_store/test_remote_online_store.py | 24 +++++-------- .../auth/server/test_auth_registry_server.py | 34 ++++++++++++------- .../tests/utils/auth_permissions_util.py | 16 ++++++--- 5 files changed, 61 insertions(+), 32 deletions(-) diff --git a/sdk/python/feast/infra/registry/remote.py b/sdk/python/feast/infra/registry/remote.py index 579f371ae10..06a88230c9e 100644 --- a/sdk/python/feast/infra/registry/remote.py +++ b/sdk/python/feast/infra/registry/remote.py @@ -71,7 +71,7 @@ def __init__( self.auth_config = auth_config assert isinstance(registry_config, RemoteRegistryConfig) if registry_config.ssl_cert_path: - with open("server.crt", "rb") as cert_file: + with open(registry_config.ssl_cert_path, "rb") as cert_file: trusted_certs = cert_file.read() ssl_credentials = grpc.ssl_channel_credentials( root_certificates=trusted_certs diff --git a/sdk/python/tests/conftest.py b/sdk/python/tests/conftest.py index 3b706d3c274..b724dd7cf17 100644 --- a/sdk/python/tests/conftest.py +++ b/sdk/python/tests/conftest.py @@ -57,6 +57,7 @@ location, ) from tests.utils.auth_permissions_util import default_store +from tests.utils.generate_self_signed_certifcate_util import generate_self_signed_cert from tests.utils.http_server import check_port_open, free_port # noqa: E402 logger = logging.getLogger(__name__) @@ -511,3 +512,19 @@ def auth_config(request, is_integration_test): return auth_configuration.replace("KEYCLOAK_URL_PLACE_HOLDER", keycloak_url) return auth_configuration + + +@pytest.fixture(params=[True, False], scope="module") +def ssl_mode(request): + is_ssl_mode = request.param + + if is_ssl_mode: + certificates_path = tempfile.mkdtemp() + ssl_key_path = os.path.join(certificates_path, "key.pem") + ssl_cert_path = os.path.join(certificates_path, "cert.pem") + generate_self_signed_cert(cert_path=ssl_cert_path, key_path=ssl_key_path) + else: + ssl_key_path = "" + ssl_cert_path = "" + + return is_ssl_mode, ssl_key_path, ssl_cert_path diff --git a/sdk/python/tests/integration/online_store/test_remote_online_store.py b/sdk/python/tests/integration/online_store/test_remote_online_store.py index 0c7894d1127..72d5006ddd7 100644 --- a/sdk/python/tests/integration/online_store/test_remote_online_store.py +++ b/sdk/python/tests/integration/online_store/test_remote_online_store.py @@ -1,3 +1,4 @@ +import logging import os import tempfile from textwrap import dedent @@ -15,11 +16,11 @@ start_feature_server, ) from tests.utils.cli_repo_creator import CliRunner -from tests.utils.generate_self_signed_certifcate_util import generate_self_signed_cert from tests.utils.http_server import free_port +logger = logging.getLogger(__name__) + -@pytest.mark.parametrize("ssl_mode", [True, False]) @pytest.mark.integration def test_remote_online_store_read(auth_config, ssl_mode): with tempfile.TemporaryDirectory() as remote_server_tmp_dir, tempfile.TemporaryDirectory() as remote_client_tmp_dir: @@ -43,7 +44,7 @@ def test_remote_online_store_read(auth_config, ssl_mode): actions=[AuthzedAction.READ_ONLINE], ), ] - server_store, server_url, registry_path, ssl_cert_path = ( + server_store, server_url, registry_path = ( _create_server_store_spin_feature_server( temp_dir=remote_server_tmp_dir, auth_config=auth_config, @@ -52,6 +53,7 @@ def test_remote_online_store_read(auth_config, ssl_mode): ) ) assert None not in (server_store, server_url, registry_path) + _, _, ssl_cert_path = ssl_mode client_store = _create_remote_client_feature_store( temp_dir=remote_client_tmp_dir, server_registry_path=str(registry_path), @@ -167,14 +169,7 @@ def _create_server_store_spin_feature_server( ): store = default_store(str(temp_dir), auth_config, permissions_list) feast_server_port = free_port() - if ssl_mode: - certificates_path = tempfile.mkdtemp() - ssl_key_path = os.path.join(certificates_path, "key.pem") - ssl_cert_path = os.path.join(certificates_path, "cert.pem") - generate_self_signed_cert(cert_path=ssl_cert_path, key_path=ssl_key_path) - else: - ssl_key_path = "" - ssl_cert_path = "" + is_ssl_mode, ssl_key_path, ssl_cert_path = ssl_mode server_url = next( start_feature_server( @@ -184,16 +179,15 @@ def _create_server_store_spin_feature_server( ssl_cert_path=ssl_cert_path, ) ) - if ssl_cert_path and ssl_key_path: - print(f"Online Server started successfully in SSL mode, {server_url}") + if is_ssl_mode: + logger.info(f"Online Server started successfully in SSL mode, {server_url}") else: - print(f"Server started successfully, {server_url}") + logger.info(f"Online Server started successfully in Non-SSL mode, {server_url}") return ( store, server_url, os.path.join(store.repo_path, "data", "registry.db"), - ssl_cert_path, ) diff --git a/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py b/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py index c72b1aa1e25..1a91f437e4a 100644 --- a/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py +++ b/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py @@ -30,11 +30,7 @@ @pytest.fixture def start_registry_server( - request, - auth_config, - server_port, - feature_store, - monkeypatch, + request, auth_config, server_port, feature_store, monkeypatch, ssl_mode ): if "kubernetes" in auth_config: mock_utils.mock_kubernetes(request=request, monkeypatch=monkeypatch) @@ -48,12 +44,23 @@ def start_registry_server( assertpy.assert_that(server_port).is_not_equal_to(0) - print(f"Starting Registry at {server_port}") - server = start_server( - feature_store, - server_port, - wait_for_termination=False, - ) + is_ssl_mode, ssl_key_path, ssl_cert_path = ssl_mode + if is_ssl_mode: + print(f"Starting Registry in SSL mode at {server_port}") + server = start_server( + store=feature_store, + port=server_port, + wait_for_termination=False, + ssl_key_path=ssl_key_path, + ssl_cert_path=ssl_cert_path, + ) + else: + print(f"Starting Registry in Non-SSL mode at {server_port}") + server = start_server( + feature_store, + server_port, + wait_for_termination=False, + ) print("Waiting server availability") wait_retry_backoff( lambda: (None, check_port_open("localhost", server_port)), @@ -69,6 +76,7 @@ def start_registry_server( def test_registry_apis( auth_config, + ssl_mode, temp_dir, server_port, start_registry_server, @@ -76,7 +84,9 @@ def test_registry_apis( applied_permissions, ): print(f"Running for\n:{auth_config}") - remote_feature_store = get_remote_registry_store(server_port, feature_store) + remote_feature_store = get_remote_registry_store( + server_port, feature_store, ssl_mode + ) permissions = _test_list_permissions(remote_feature_store, applied_permissions) _test_get_entity(remote_feature_store, applied_permissions) _test_list_entities(remote_feature_store, applied_permissions) diff --git a/sdk/python/tests/utils/auth_permissions_util.py b/sdk/python/tests/utils/auth_permissions_util.py index 1147e66a0d1..3b3f63cabca 100644 --- a/sdk/python/tests/utils/auth_permissions_util.py +++ b/sdk/python/tests/utils/auth_permissions_util.py @@ -126,10 +126,18 @@ def start_feature_server( ) -def get_remote_registry_store(server_port, feature_store): - registry_config = RemoteRegistryConfig( - registry_type="remote", path=f"localhost:{server_port}" - ) +def get_remote_registry_store(server_port, feature_store, ssl_mode): + is_ssl_mode, _, ssl_cert_path = ssl_mode + if is_ssl_mode: + registry_config = RemoteRegistryConfig( + registry_type="remote", + path=f"localhost:{server_port}", + ssl_cert_path=ssl_cert_path, + ) + else: + registry_config = RemoteRegistryConfig( + registry_type="remote", path=f"localhost:{server_port}" + ) store = FeatureStore( config=RepoConfig( From 2a9fd71a4d0ec5cd6322276ecc8c4d114dd443fa Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:56:58 -0400 Subject: [PATCH 4/9] Incorporating code review comments. refactored the args from "ssl_cert_path" and "ssl_key_path" to just --cert and --key. This change applied to registry and online server. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- .../feature-servers/offline-feature-server.md | 2 +- .../feature-servers/python-feature-server.md | 4 ++-- sdk/python/feast/cli.py | 24 +++++++++++-------- .../tests/utils/auth_permissions_util.py | 4 ++-- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/docs/reference/feature-servers/offline-feature-server.md b/docs/reference/feature-servers/offline-feature-server.md index 1db5adacd8a..4d6879624f2 100644 --- a/docs/reference/feature-servers/offline-feature-server.md +++ b/docs/reference/feature-servers/offline-feature-server.md @@ -23,7 +23,7 @@ helm install feast-offline-server feast-charts/feast-feature-server --set feast_ ## Server Example -The complete example can be find under [remote-offline-store-example](../../../examples/remote-offline-store) +The complete example can be found under [remote-offline-store-example](../../../examples/remote-offline-store) ## How to configure the client diff --git a/docs/reference/feature-servers/python-feature-server.md b/docs/reference/feature-servers/python-feature-server.md index 0400a5fe229..afad0ad950c 100644 --- a/docs/reference/feature-servers/python-feature-server.md +++ b/docs/reference/feature-servers/python-feature-server.md @@ -216,10 +216,10 @@ The above command will generate two files * `cert.pem`: certificate public key ### Starting the Online Server in SSL Mode -To start the feature server in SSL mode, you need to provide the private and public keys using the `--ssl-key-path` and `--ssl-cert-path` arguments with the `feast serve` command. +To start the feature server in SSL mode, you need to provide the private and public keys using the `--key` and `--cert` arguments with the `feast serve` command. ```shell -feast serve --ssl-key-path /path/to/key.pem --ssl-cert-path /path/to/cert.pem +feast serve --key /path/to/key.pem --cert /path/to/cert.pem ``` # Online Feature Server Permissions and Access Control diff --git a/sdk/python/feast/cli.py b/sdk/python/feast/cli.py index 4ba6acc3ee5..bf67156f61e 100644 --- a/sdk/python/feast/cli.py +++ b/sdk/python/feast/cli.py @@ -912,20 +912,22 @@ def init_command(project_directory, minimal: bool, template: str): show_default=True, ) @click.option( - "--ssl-key-path", + "--key", "-k", + "ssl_key_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate private key. You need to pass ssl-cert-path as well to start server in SSL mode", + help="path to SSL certificate private key. You need to pass --cert as well to start server in SSL mode", ) @click.option( - "--ssl-cert-path", + "--cert", "-c", + "ssl_cert_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate public key. You need to pass ssl-key-path as well to start server in SSL mode", + help="path to SSL certificate public key. You need to pass --key as well to start server in SSL mode", ) @click.option( "--metrics", @@ -951,7 +953,7 @@ def serve_command( """Start a feature server locally on a given port.""" if (ssl_key_path and not ssl_cert_path) or (not ssl_key_path and ssl_cert_path): raise click.BadParameter( - "Please configure ssl-cert-path and ssl-key-path args to start the feature server in SSL mode." + "Please pass --cert and --key args to start the feature server in SSL mode." ) store = create_feature_store(ctx) @@ -1036,20 +1038,22 @@ def serve_transformations_command(ctx: click.Context, port: int): help="Specify a port for the server", ) @click.option( - "--ssl-key-path", + "--key", "-k", + "ssl_key_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate private key. You need to pass ssl-cert-path as well to start server in SSL mode", + help="path to SSL certificate private key. You need to pass --cert as well to start server in SSL mode", ) @click.option( - "--ssl-cert-path", + "--cert", "-c", + "ssl_cert_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate public key. You need to pass ssl-key-path as well to start server in SSL mode", + help="path to SSL certificate public key. You need to pass --key as well to start server in SSL mode", ) @click.pass_context def serve_registry_command( @@ -1061,7 +1065,7 @@ def serve_registry_command( """Start a registry server locally on a given port.""" if (ssl_key_path and not ssl_cert_path) or (not ssl_key_path and ssl_cert_path): raise click.BadParameter( - "Please configure ssl-cert-path and ssl-key-path args to start the registry server in SSL mode." + "Please pass --cert and --key args to start the registry server in SSL mode." ) store = create_feature_store(ctx) diff --git a/sdk/python/tests/utils/auth_permissions_util.py b/sdk/python/tests/utils/auth_permissions_util.py index 3b3f63cabca..a5a859bd03e 100644 --- a/sdk/python/tests/utils/auth_permissions_util.py +++ b/sdk/python/tests/utils/auth_permissions_util.py @@ -73,9 +73,9 @@ def start_feature_server( ] if ssl_cert_path and ssl_cert_path: - cmd.append("--ssl-key-path") + cmd.append("--key") cmd.append(ssl_key_path) - cmd.append("--ssl-cert-path") + cmd.append("--cert") cmd.append(ssl_cert_path) feast_server_process = subprocess.Popen( From 5a2620bd0279094f31dabb25eb2d4010347f575c Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:56:58 -0400 Subject: [PATCH 5/9] Incorporating code review comments. refactored the args from "ssl_cert_path" and "ssl_key_path" to just --cert and --key. This change applied to registry and online server. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- sdk/python/feast/infra/online_stores/remote.py | 6 +++--- sdk/python/feast/infra/registry/remote.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/python/feast/infra/online_stores/remote.py b/sdk/python/feast/infra/online_stores/remote.py index 70edf93eb33..efee13ef2e1 100644 --- a/sdk/python/feast/infra/online_stores/remote.py +++ b/sdk/python/feast/infra/online_stores/remote.py @@ -41,7 +41,7 @@ class RemoteOnlineStoreConfig(FeastConfigBaseModel): """ str: Path to metadata store. If type is 'remote', then this is a URL for registry server """ - ssl_cert_path: StrictStr = "" + cert: StrictStr = "" """ str: Path to the public certificate when the online server starts in SSL mode. This may be needed if the online server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. If type is 'remote', then this configuration is needed to connect to remote online server in SSL mode. """ @@ -174,11 +174,11 @@ def teardown( def get_remote_online_features( session: requests.Session, config: RepoConfig, req_body: str ) -> requests.Response: - if config.online_store.ssl_cert_path: + if config.online_store.cert: return session.post( f"{config.online_store.path}/get-online-features", data=req_body, - verify=config.online_store.ssl_cert_path, + verify=config.online_store.cert, ) else: return session.post( diff --git a/sdk/python/feast/infra/registry/remote.py b/sdk/python/feast/infra/registry/remote.py index 06a88230c9e..ee2054748f9 100644 --- a/sdk/python/feast/infra/registry/remote.py +++ b/sdk/python/feast/infra/registry/remote.py @@ -55,7 +55,7 @@ class RemoteRegistryConfig(RegistryConfig): """ str: Path to metadata store. If registry_type is 'remote', then this is a URL for registry server """ - ssl_cert_path: StrictStr = "" + cert: StrictStr = "" """ str: Path to the public certificate when the registry server starts in SSL mode. This may be needed if the registry server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. If registry_type is 'remote', then this configuration is needed to connect to remote registry server in SSL mode. If the remote registry started in non-ssl mode then this configuration is not needed.""" @@ -70,8 +70,8 @@ def __init__( ): self.auth_config = auth_config assert isinstance(registry_config, RemoteRegistryConfig) - if registry_config.ssl_cert_path: - with open(registry_config.ssl_cert_path, "rb") as cert_file: + if registry_config.cert: + with open(registry_config.cert, "rb") as cert_file: trusted_certs = cert_file.read() ssl_credentials = grpc.ssl_channel_credentials( root_certificates=trusted_certs From 67a7197dbcb309b363f87a3881538e5385c38653 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:56:58 -0400 Subject: [PATCH 6/9] Incorporating code review comments. refactored the args from "ssl_cert_path" and "ssl_key_path" to just --cert and --key. This change applied to registry and online server. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- docs/reference/online-stores/remote.md | 4 ++-- .../integration/online_store/test_remote_online_store.py | 2 +- sdk/python/tests/utils/auth_permissions_util.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/reference/online-stores/remote.md b/docs/reference/online-stores/remote.md index 61bb50793d0..4927daf8dc6 100644 --- a/docs/reference/online-stores/remote.md +++ b/docs/reference/online-stores/remote.md @@ -16,14 +16,14 @@ provider: local online_store: path: http://localhost:6566 type: remote - ssl_cert_path: /path/to/cert.pem + cert: /path/to/cert.pem entity_key_serialization_version: 2 auth: type: no_auth ``` {% endcode %} -`ssl_cert_path` is an optional configuration to the public certificate path when the online server starts in SSL mode. This may be needed if the online server is started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. +`cert` is an optional configuration to the public certificate path when the online server starts in SSL mode. This may be needed if the online server is started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. ## How to configure Authentication and Authorization Please refer the [page](./../../../docs/getting-started/concepts/permission.md) for more details on how to configure authentication and authorization. diff --git a/sdk/python/tests/integration/online_store/test_remote_online_store.py b/sdk/python/tests/integration/online_store/test_remote_online_store.py index 72d5006ddd7..5caafbc2134 100644 --- a/sdk/python/tests/integration/online_store/test_remote_online_store.py +++ b/sdk/python/tests/integration/online_store/test_remote_online_store.py @@ -236,7 +236,7 @@ def _overwrite_remote_client_feature_store_yaml( ) if ssl_cert_path: - config_content += f" ssl_cert_path: {ssl_cert_path}\n" + config_content += f" cert: {ssl_cert_path}\n" with open(repo_config, "w") as repo_config_file: repo_config_file.write(config_content) diff --git a/sdk/python/tests/utils/auth_permissions_util.py b/sdk/python/tests/utils/auth_permissions_util.py index a5a859bd03e..7008158b5df 100644 --- a/sdk/python/tests/utils/auth_permissions_util.py +++ b/sdk/python/tests/utils/auth_permissions_util.py @@ -132,7 +132,7 @@ def get_remote_registry_store(server_port, feature_store, ssl_mode): registry_config = RemoteRegistryConfig( registry_type="remote", path=f"localhost:{server_port}", - ssl_cert_path=ssl_cert_path, + cert=ssl_cert_path, ) else: registry_config = RemoteRegistryConfig( From ed2809c5f109d30ec8ff085cb4fc20fb2df83a75 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Thu, 31 Oct 2024 16:38:08 -0400 Subject: [PATCH 7/9] Incorporating code review comments. renamed SSL to TLS. some places kept both intentionally. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- .../feature-servers/python-feature-server.md | 12 +++--- docs/reference/online-stores/remote.md | 2 +- sdk/python/feast/cli.py | 38 +++++++++---------- sdk/python/feast/feature_server.py | 16 ++++---- sdk/python/feast/feature_store.py | 12 +++--- sdk/python/feast/infra/registry/remote.py | 8 ++-- sdk/python/feast/registry_server.py | 16 ++++---- sdk/python/tests/conftest.py | 18 ++++----- .../online_store/test_remote_online_store.py | 36 ++++++++++-------- .../auth/server/test_auth_registry_server.py | 18 ++++----- .../tests/utils/auth_permissions_util.py | 20 +++++----- 11 files changed, 100 insertions(+), 96 deletions(-) diff --git a/docs/reference/feature-servers/python-feature-server.md b/docs/reference/feature-servers/python-feature-server.md index afad0ad950c..d7374852495 100644 --- a/docs/reference/feature-servers/python-feature-server.md +++ b/docs/reference/feature-servers/python-feature-server.md @@ -200,12 +200,12 @@ requests.post( data=json.dumps(push_data)) ``` -## Starting the feature server in SSL mode +## Starting the feature server in TLS(SSL) mode -Enabling SSL mode ensures that data between the Feast client and server is transmitted securely. For an ideal production environment, it is recommended to start the feature server in SSL mode. +Enabling TLS mode ensures that data between the Feast client and server is transmitted securely. For an ideal production environment, it is recommended to start the feature server in TLS mode. -### Obtaining a self-signed SSL certificate and key -In development mode we can generate a self-signed certificate for testing. In an actual production environment it is always recommended to get it from a trusted SSL certificate provider. +### Obtaining a self-signed TLS certificate and key +In development mode we can generate a self-signed certificate for testing. In an actual production environment it is always recommended to get it from a trusted TLS certificate provider. ```shell openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 365 -nodes @@ -215,8 +215,8 @@ The above command will generate two files * `key.pem` : certificate private key * `cert.pem`: certificate public key -### Starting the Online Server in SSL Mode -To start the feature server in SSL mode, you need to provide the private and public keys using the `--key` and `--cert` arguments with the `feast serve` command. +### Starting the Online Server in TLS(SSL) Mode +To start the feature server in TLS mode, you need to provide the private and public keys using the `--key` and `--cert` arguments with the `feast serve` command. ```shell feast serve --key /path/to/key.pem --cert /path/to/cert.pem diff --git a/docs/reference/online-stores/remote.md b/docs/reference/online-stores/remote.md index 4927daf8dc6..aa97a495baa 100644 --- a/docs/reference/online-stores/remote.md +++ b/docs/reference/online-stores/remote.md @@ -23,7 +23,7 @@ auth: ``` {% endcode %} -`cert` is an optional configuration to the public certificate path when the online server starts in SSL mode. This may be needed if the online server is started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. +`cert` is an optional configuration to the public certificate path when the online server starts in TLS(SSL) mode. This may be needed if the online server is started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. ## How to configure Authentication and Authorization Please refer the [page](./../../../docs/getting-started/concepts/permission.md) for more details on how to configure authentication and authorization. diff --git a/sdk/python/feast/cli.py b/sdk/python/feast/cli.py index bf67156f61e..3cd5530bee3 100644 --- a/sdk/python/feast/cli.py +++ b/sdk/python/feast/cli.py @@ -914,20 +914,20 @@ def init_command(project_directory, minimal: bool, template: str): @click.option( "--key", "-k", - "ssl_key_path", + "tls_key_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate private key. You need to pass --cert as well to start server in SSL mode", + help="path to TLS certificate private key. You need to pass --cert as well to start server in TLS mode", ) @click.option( "--cert", "-c", - "ssl_cert_path", + "tls_cert_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate public key. You need to pass --key as well to start server in SSL mode", + help="path to TLS certificate public key. You need to pass --key as well to start server in TLS mode", ) @click.option( "--metrics", @@ -946,14 +946,14 @@ def serve_command( workers: int, metrics: bool, keep_alive_timeout: int, - ssl_key_path: str, - ssl_cert_path: str, + tls_key_path: str, + tls_cert_path: str, registry_ttl_sec: int = 5, ): """Start a feature server locally on a given port.""" - if (ssl_key_path and not ssl_cert_path) or (not ssl_key_path and ssl_cert_path): + if (tls_key_path and not tls_cert_path) or (not tls_key_path and tls_cert_path): raise click.BadParameter( - "Please pass --cert and --key args to start the feature server in SSL mode." + "Please pass --cert and --key args to start the feature server in TLS mode." ) store = create_feature_store(ctx) @@ -966,8 +966,8 @@ def serve_command( workers=workers, metrics=metrics, keep_alive_timeout=keep_alive_timeout, - ssl_key_path=ssl_key_path, - ssl_cert_path=ssl_cert_path, + tls_key_path=tls_key_path, + tls_cert_path=tls_cert_path, registry_ttl_sec=registry_ttl_sec, ) @@ -1040,36 +1040,36 @@ def serve_transformations_command(ctx: click.Context, port: int): @click.option( "--key", "-k", - "ssl_key_path", + "tls_key_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate private key. You need to pass --cert as well to start server in SSL mode", + help="path to TLS certificate private key. You need to pass --cert as well to start server in TLS mode", ) @click.option( "--cert", "-c", - "ssl_cert_path", + "tls_cert_path", type=click.STRING, default="", show_default=False, - help="path to SSL certificate public key. You need to pass --key as well to start server in SSL mode", + help="path to TLS certificate public key. You need to pass --key as well to start server in TLS mode", ) @click.pass_context def serve_registry_command( ctx: click.Context, port: int, - ssl_key_path: str, - ssl_cert_path: str, + tls_key_path: str, + tls_cert_path: str, ): """Start a registry server locally on a given port.""" - if (ssl_key_path and not ssl_cert_path) or (not ssl_key_path and ssl_cert_path): + if (tls_key_path and not tls_cert_path) or (not tls_key_path and tls_cert_path): raise click.BadParameter( - "Please pass --cert and --key args to start the registry server in SSL mode." + "Please pass --cert and --key args to start the registry server in TLS mode." ) store = create_feature_store(ctx) - store.serve_registry(port, ssl_key_path, ssl_cert_path) + store.serve_registry(port, tls_key_path, tls_cert_path) @cli.command("serve_offline") diff --git a/sdk/python/feast/feature_server.py b/sdk/python/feast/feature_server.py index 0502c2a85d5..3c98754cc36 100644 --- a/sdk/python/feast/feature_server.py +++ b/sdk/python/feast/feature_server.py @@ -339,8 +339,8 @@ def start_server( workers: int, keep_alive_timeout: int, registry_ttl_sec: int, - ssl_key_path: str, - ssl_cert_path: str, + tls_key_path: str, + tls_cert_path: str, metrics: bool, ): if metrics: @@ -375,22 +375,22 @@ def start_server( } # Add SSL options if the paths exist - if ssl_key_path and ssl_cert_path: - options["keyfile"] = ssl_key_path - options["certfile"] = ssl_cert_path + if tls_key_path and tls_cert_path: + options["keyfile"] = tls_key_path + options["certfile"] = tls_cert_path FeastServeApplication(store=store, **options).run() else: import uvicorn app = get_app(store, registry_ttl_sec) - if ssl_key_path and ssl_cert_path: + if tls_key_path and tls_cert_path: uvicorn.run( app, host=host, port=port, access_log=(not no_access_log), - ssl_keyfile=ssl_key_path, - ssl_certfile=ssl_cert_path, + ssl_keyfile=tls_key_path, + ssl_certfile=tls_cert_path, ) else: uvicorn.run(app, host=host, port=port, access_log=(not no_access_log)) diff --git a/sdk/python/feast/feature_store.py b/sdk/python/feast/feature_store.py index 06e860cfb81..b4f11a73c78 100644 --- a/sdk/python/feast/feature_store.py +++ b/sdk/python/feast/feature_store.py @@ -1896,8 +1896,8 @@ def serve( workers: int = 1, metrics: bool = False, keep_alive_timeout: int = 30, - ssl_key_path: str = "", - ssl_cert_path: str = "", + tls_key_path: str = "", + tls_cert_path: str = "", registry_ttl_sec: int = 2, ) -> None: """Start the feature consumption server locally on a given port.""" @@ -1915,8 +1915,8 @@ def serve( workers=workers, metrics=metrics, keep_alive_timeout=keep_alive_timeout, - ssl_key_path=ssl_key_path, - ssl_cert_path=ssl_cert_path, + tls_key_path=tls_key_path, + tls_cert_path=tls_cert_path, registry_ttl_sec=registry_ttl_sec, ) @@ -1950,13 +1950,13 @@ def serve_ui( ) def serve_registry( - self, port: int, ssl_key_path: str = "", ssl_cert_path: str = "" + self, port: int, tls_key_path: str = "", tls_cert_path: str = "" ) -> None: """Start registry server locally on a given port.""" from feast import registry_server registry_server.start_server( - self, port=port, ssl_key_path=ssl_key_path, ssl_cert_path=ssl_cert_path + self, port=port, tls_key_path=tls_key_path, tls_cert_path=tls_cert_path ) def serve_offline(self, host: str, port: int) -> None: diff --git a/sdk/python/feast/infra/registry/remote.py b/sdk/python/feast/infra/registry/remote.py index ee2054748f9..6cc80d5dad1 100644 --- a/sdk/python/feast/infra/registry/remote.py +++ b/sdk/python/feast/infra/registry/remote.py @@ -56,8 +56,8 @@ class RemoteRegistryConfig(RegistryConfig): If registry_type is 'remote', then this is a URL for registry server """ cert: StrictStr = "" - """ str: Path to the public certificate when the registry server starts in SSL mode. This may be needed if the registry server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. - If registry_type is 'remote', then this configuration is needed to connect to remote registry server in SSL mode. If the remote registry started in non-ssl mode then this configuration is not needed.""" + """ str: Path to the public certificate when the registry server starts in TLS(SSL) mode. This may be needed if the registry server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. + If registry_type is 'remote', then this configuration is needed to connect to remote registry server in TLS mode. If the remote registry started in non-tls mode then this configuration is not needed.""" class RemoteRegistry(BaseRegistry): @@ -73,10 +73,10 @@ def __init__( if registry_config.cert: with open(registry_config.cert, "rb") as cert_file: trusted_certs = cert_file.read() - ssl_credentials = grpc.ssl_channel_credentials( + tls_credentials = grpc.ssl_channel_credentials( root_certificates=trusted_certs ) - self.channel = grpc.secure_channel(registry_config.path, ssl_credentials) + self.channel = grpc.secure_channel(registry_config.path, tls_credentials) else: self.channel = grpc.insecure_channel(registry_config.path) diff --git a/sdk/python/feast/registry_server.py b/sdk/python/feast/registry_server.py index 4d77d687378..181dc79656e 100644 --- a/sdk/python/feast/registry_server.py +++ b/sdk/python/feast/registry_server.py @@ -761,8 +761,8 @@ def start_server( store: FeatureStore, port: int, wait_for_termination: bool = True, - ssl_key_path: str = "", - ssl_cert_path: str = "", + tls_key_path: str = "", + tls_cert_path: str = "", ): auth_manager_type = str_to_auth_manager_type(store.config.auth_config.type) init_security_manager(auth_type=auth_manager_type, fs=store) @@ -791,24 +791,24 @@ def start_server( ) reflection.enable_server_reflection(service_names_available_for_reflection, server) - if ssl_cert_path and ssl_key_path: - with open(ssl_cert_path, "rb") as cert_file, open( - ssl_key_path, "rb" + if tls_cert_path and tls_key_path: + with open(tls_cert_path, "rb") as cert_file, open( + tls_key_path, "rb" ) as key_file: certificate_chain = cert_file.read() private_key = key_file.read() server_credentials = grpc.ssl_server_credentials( ((private_key, certificate_chain),) ) - logger.info("Starting grpc registry server in SSL mode") + logger.info("Starting grpc registry server in TLS(SSL) mode") server.add_secure_port(f"[::]:{port}", server_credentials) else: - logger.info("Starting grpc registry server in non-SSL mode") + logger.info("Starting grpc registry server in non-TLS(SSL) mode") server.add_insecure_port(f"[::]:{port}") server.start() if wait_for_termination: logger.info( - f"Grpc server started at {'https' if ssl_cert_path and ssl_key_path else 'http'}://localhost:{port}" + f"Grpc server started at {'https' if tls_cert_path and tls_key_path else 'http'}://localhost:{port}" ) server.wait_for_termination() else: diff --git a/sdk/python/tests/conftest.py b/sdk/python/tests/conftest.py index b724dd7cf17..24c8f40f742 100644 --- a/sdk/python/tests/conftest.py +++ b/sdk/python/tests/conftest.py @@ -515,16 +515,16 @@ def auth_config(request, is_integration_test): @pytest.fixture(params=[True, False], scope="module") -def ssl_mode(request): - is_ssl_mode = request.param +def tls_mode(request): + is_tls_mode = request.param - if is_ssl_mode: + if is_tls_mode: certificates_path = tempfile.mkdtemp() - ssl_key_path = os.path.join(certificates_path, "key.pem") - ssl_cert_path = os.path.join(certificates_path, "cert.pem") - generate_self_signed_cert(cert_path=ssl_cert_path, key_path=ssl_key_path) + tls_key_path = os.path.join(certificates_path, "key.pem") + tls_cert_path = os.path.join(certificates_path, "cert.pem") + generate_self_signed_cert(cert_path=tls_cert_path, key_path=tls_key_path) else: - ssl_key_path = "" - ssl_cert_path = "" + tls_key_path = "" + tls_cert_path = "" - return is_ssl_mode, ssl_key_path, ssl_cert_path + return is_tls_mode, tls_key_path, tls_cert_path diff --git a/sdk/python/tests/integration/online_store/test_remote_online_store.py b/sdk/python/tests/integration/online_store/test_remote_online_store.py index 5caafbc2134..2519d3d9bef 100644 --- a/sdk/python/tests/integration/online_store/test_remote_online_store.py +++ b/sdk/python/tests/integration/online_store/test_remote_online_store.py @@ -22,7 +22,7 @@ @pytest.mark.integration -def test_remote_online_store_read(auth_config, ssl_mode): +def test_remote_online_store_read(auth_config, tls_mode): with tempfile.TemporaryDirectory() as remote_server_tmp_dir, tempfile.TemporaryDirectory() as remote_client_tmp_dir: permissions_list = [ Permission( @@ -49,17 +49,17 @@ def test_remote_online_store_read(auth_config, ssl_mode): temp_dir=remote_server_tmp_dir, auth_config=auth_config, permissions_list=permissions_list, - ssl_mode=ssl_mode, + tls_mode=tls_mode, ) ) assert None not in (server_store, server_url, registry_path) - _, _, ssl_cert_path = ssl_mode + _, _, tls_cert_path = tls_mode client_store = _create_remote_client_feature_store( temp_dir=remote_client_tmp_dir, server_registry_path=str(registry_path), feature_server_url=server_url, auth_config=auth_config, - ssl_cert_path=ssl_cert_path, + tls_cert_path=tls_cert_path, ) assert client_store is not None _assert_non_existing_entity_feature_views_entity( @@ -165,24 +165,28 @@ def _assert_client_server_online_stores_are_matching( def _create_server_store_spin_feature_server( - temp_dir, auth_config: str, permissions_list, ssl_mode: bool + temp_dir, auth_config: str, permissions_list, tls_mode ): store = default_store(str(temp_dir), auth_config, permissions_list) feast_server_port = free_port() - is_ssl_mode, ssl_key_path, ssl_cert_path = ssl_mode + is_tls_mode, tls_key_path, tls_cert_path = tls_mode server_url = next( start_feature_server( repo_path=str(store.repo_path), server_port=feast_server_port, - ssl_key_path=ssl_key_path, - ssl_cert_path=ssl_cert_path, + tls_key_path=tls_key_path, + tls_cert_path=tls_cert_path, ) ) - if is_ssl_mode: - logger.info(f"Online Server started successfully in SSL mode, {server_url}") + if is_tls_mode: + logger.info( + f"Online Server started successfully in TLS(SSL) mode, {server_url}" + ) else: - logger.info(f"Online Server started successfully in Non-SSL mode, {server_url}") + logger.info( + f"Online Server started successfully in Non-TLS(SSL) mode, {server_url}" + ) return ( store, @@ -196,7 +200,7 @@ def _create_remote_client_feature_store( server_registry_path: str, feature_server_url: str, auth_config: str, - ssl_cert_path: str = "", + tls_cert_path: str = "", ) -> FeatureStore: project_name = "REMOTE_ONLINE_CLIENT_PROJECT" runner = CliRunner() @@ -208,7 +212,7 @@ def _create_remote_client_feature_store( registry_path=server_registry_path, feature_server_url=feature_server_url, auth_config=auth_config, - ssl_cert_path=ssl_cert_path, + tls_cert_path=tls_cert_path, ) return FeatureStore(repo_path=repo_path) @@ -219,7 +223,7 @@ def _overwrite_remote_client_feature_store_yaml( registry_path: str, feature_server_url: str, auth_config: str, - ssl_cert_path: str = "", + tls_cert_path: str = "", ): repo_config = os.path.join(repo_path, "feature_store.yaml") @@ -235,8 +239,8 @@ def _overwrite_remote_client_feature_store_yaml( """ ) - if ssl_cert_path: - config_content += f" cert: {ssl_cert_path}\n" + if tls_cert_path: + config_content += f" cert: {tls_cert_path}\n" with open(repo_config, "w") as repo_config_file: repo_config_file.write(config_content) diff --git a/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py b/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py index 1a91f437e4a..25c5fe3eb8c 100644 --- a/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py +++ b/sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py @@ -30,7 +30,7 @@ @pytest.fixture def start_registry_server( - request, auth_config, server_port, feature_store, monkeypatch, ssl_mode + request, auth_config, server_port, feature_store, monkeypatch, tls_mode ): if "kubernetes" in auth_config: mock_utils.mock_kubernetes(request=request, monkeypatch=monkeypatch) @@ -44,18 +44,18 @@ def start_registry_server( assertpy.assert_that(server_port).is_not_equal_to(0) - is_ssl_mode, ssl_key_path, ssl_cert_path = ssl_mode - if is_ssl_mode: - print(f"Starting Registry in SSL mode at {server_port}") + is_tls_mode, tls_key_path, tls_cert_path = tls_mode + if is_tls_mode: + print(f"Starting Registry in TLS mode at {server_port}") server = start_server( store=feature_store, port=server_port, wait_for_termination=False, - ssl_key_path=ssl_key_path, - ssl_cert_path=ssl_cert_path, + tls_key_path=tls_key_path, + tls_cert_path=tls_cert_path, ) else: - print(f"Starting Registry in Non-SSL mode at {server_port}") + print(f"Starting Registry in Non-TLS mode at {server_port}") server = start_server( feature_store, server_port, @@ -76,7 +76,7 @@ def start_registry_server( def test_registry_apis( auth_config, - ssl_mode, + tls_mode, temp_dir, server_port, start_registry_server, @@ -85,7 +85,7 @@ def test_registry_apis( ): print(f"Running for\n:{auth_config}") remote_feature_store = get_remote_registry_store( - server_port, feature_store, ssl_mode + server_port, feature_store, tls_mode ) permissions = _test_list_permissions(remote_feature_store, applied_permissions) _test_get_entity(remote_feature_store, applied_permissions) diff --git a/sdk/python/tests/utils/auth_permissions_util.py b/sdk/python/tests/utils/auth_permissions_util.py index 7008158b5df..6f0a3c8eeac 100644 --- a/sdk/python/tests/utils/auth_permissions_util.py +++ b/sdk/python/tests/utils/auth_permissions_util.py @@ -58,8 +58,8 @@ def start_feature_server( repo_path: str, server_port: int, metrics: bool = False, - ssl_key_path: str = "", - ssl_cert_path: str = "", + tls_key_path: str = "", + tls_cert_path: str = "", ): host = "0.0.0.0" cmd = [ @@ -72,11 +72,11 @@ def start_feature_server( str(server_port), ] - if ssl_cert_path and ssl_cert_path: + if tls_cert_path and tls_cert_path: cmd.append("--key") - cmd.append(ssl_key_path) + cmd.append(tls_key_path) cmd.append("--cert") - cmd.append(ssl_cert_path) + cmd.append(tls_cert_path) feast_server_process = subprocess.Popen( cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE @@ -106,7 +106,7 @@ def start_feature_server( online_server_url = ( f"https://localhost:{server_port}" - if ssl_key_path and ssl_cert_path + if tls_key_path and tls_cert_path else f"http://localhost:{server_port}" ) @@ -126,13 +126,13 @@ def start_feature_server( ) -def get_remote_registry_store(server_port, feature_store, ssl_mode): - is_ssl_mode, _, ssl_cert_path = ssl_mode - if is_ssl_mode: +def get_remote_registry_store(server_port, feature_store, tls_mode): + is_tls_mode, _, tls_cert_path = tls_mode + if is_tls_mode: registry_config = RemoteRegistryConfig( registry_type="remote", path=f"localhost:{server_port}", - cert=ssl_cert_path, + cert=tls_cert_path, ) else: registry_config = RemoteRegistryConfig( From 6b44f2e0b487960ae70b85984c6d3ddc36ec4208 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Thu, 31 Oct 2024 16:45:22 -0400 Subject: [PATCH 8/9] Incorporating code review comments. renamed SSL to TLS. some places kept both intentionally. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- sdk/python/feast/infra/online_stores/remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/python/feast/infra/online_stores/remote.py b/sdk/python/feast/infra/online_stores/remote.py index efee13ef2e1..8cc75ade445 100644 --- a/sdk/python/feast/infra/online_stores/remote.py +++ b/sdk/python/feast/infra/online_stores/remote.py @@ -42,8 +42,8 @@ class RemoteOnlineStoreConfig(FeastConfigBaseModel): If type is 'remote', then this is a URL for registry server """ cert: StrictStr = "" - """ str: Path to the public certificate when the online server starts in SSL mode. This may be needed if the online server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. - If type is 'remote', then this configuration is needed to connect to remote online server in SSL mode. """ + """ str: Path to the public certificate when the online server starts in TLS(SSL) mode. This may be needed if the online server started with a self-signed certificate, typically this file ends with `*.crt`, `*.cer`, or `*.pem`. + If type is 'remote', then this configuration is needed to connect to remote online server in TLS mode. """ class RemoteOnlineStore(OnlineStore): From 337d103c999516ebe8bebd0805c3e5c226f703f8 Mon Sep 17 00:00:00 2001 From: lrangine <19699092+lokeshrangineni@users.noreply.github.com> Date: Fri, 1 Nov 2024 10:50:08 -0400 Subject: [PATCH 9/9] Incorporating code review comments. Adding validation at the service level. Signed-off-by: lrangine <19699092+lokeshrangineni@users.noreply.github.com> --- sdk/python/feast/feature_server.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sdk/python/feast/feature_server.py b/sdk/python/feast/feature_server.py index 3c98754cc36..2b222b1ad42 100644 --- a/sdk/python/feast/feature_server.py +++ b/sdk/python/feast/feature_server.py @@ -343,6 +343,10 @@ def start_server( tls_cert_path: str, metrics: bool, ): + if (tls_key_path and not tls_cert_path) or (not tls_key_path and tls_cert_path): + raise ValueError( + "Both key and cert file paths are required to start server in TLS mode." + ) if metrics: logger.info("Starting Prometheus Server") start_http_server(8000)