direct client to exporter connectivity #361
Conversation
WalkthroughThe changes add support for alternative endpoints across the Jumpstarter codebase. Functions in client modules now accept a new Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant CFP as ClientFromPath
participant CFC as ClientFromChannel
participant Sess as Session
participant SC as SecureChannelCreator
participant Stub as SmartExporterStub
Env->>CFP: Read JMP_USE_ALT_ENDPOINTS flag
CFP->>CFC: Pass use_alternative_endpoints flag
CFC->>Sess: Call GetReport()
Sess-->>CFC: Return report with alternative_endpoints
alt use_alternative_endpoints enabled
CFC->>SC: Create secure channels for each alternative endpoint
SC-->>CFC: Return list of secure channels
CFC->>Stub: Initialize SmartExporterStub with reversed channels
else Standard flow
CFC->>Stub: Initialize SmartExporterStub with default channel
end
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/exporter/tls.py (1)
33-74: Address the TODO for token generation & consider manageability of the self-signed certificate.
- The
# FIXME: generate and check tokencomment signals that a critical feature (token generation/validation) is not yet implemented. Ensure proper handling before production usage.- Storing private keys and certificates in code is acceptable here for ephemeral usage, though be mindful of secure distribution if used in more permanent scenarios.
Otherwise, this function effectively configures TLS with SAN for each endpoint.Do you want me to propose a minimal token-based authentication flow or file a follow-up ticket?
packages/jumpstarter/jumpstarter/exporter/session.py (1)
100-100: Good usage of_alternative_endpointsinGetReport.Exposing
alternative_endpointsin the response provides a cohesive integration for clients. As a follow-up, confirm whether clients expect these endpoints immediately or only after a successful handshake.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py (1)
32-32: Consider ignoring or excluding auto-generated files from lint checks.Ruff flags this line for exceeding the max line length. Typically, large descriptor strings in generated protobuf code are best handled by ignoring the file or adjusting the generator’s formatting options.
🧰 Tools
🪛 Ruff (0.8.2)
32-32: Line too long (6808 > 120)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(2 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(4 hunks)packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/jumpstarter/jumpstarter/exporter/exporter.py (3)
packages/jumpstarter/jumpstarter/config/exporter.py (1) (1)
serve_unix_async(143-151)packages/jumpstarter/jumpstarter/exporter/session.py (1) (1)
serve_unix_async(77-80)packages/jumpstarter/jumpstarter/client/lease.py (1) (1)
serve_unix_async(155-157)
packages/jumpstarter/jumpstarter/client/client.py (2)
packages/jumpstarter/jumpstarter/exporter/session.py (1) (1)
GetReport(91-101)packages/jumpstarter/jumpstarter/config/client.py (1) (1)
channel(54-60)
packages/jumpstarter/jumpstarter/exporter/session.py (3)
packages/jumpstarter/jumpstarter/exporter/tls.py (1) (1)
with_alternative_endpoints(33-74)packages/jumpstarter/jumpstarter/config/exporter.py (2) (2)
serve_unix_async(143-151)serve_unix(154-157)packages/jumpstarter/jumpstarter/client/lease.py (2) (2)
serve_unix_async(155-157)serve_unix(172-174)
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/client/client.py
[error] 1-1: Ruff: Import block is un-sorted or un-formatted.
🪛 Ruff (0.8.2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py
32-32: Line too long (6808 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (13)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (2)
767-767: Code formatting looks goodThe addition of a blank line before the test function improves readability by creating better visual separation between test cases.
1145-1145: Code formatting looks goodThe addition of a blank line before the test function creates better visual separation between test cases, improving readability.
packages/jumpstarter/jumpstarter/exporter/exporter.py (2)
28-28: Well-implemented attribute additionGood use of
field(default_factory=list)to avoid the mutable default argument issue. This new attribute will store alternative endpoints for the exporter.
53-53: Properly integrates the alternative_endpoints parameterThe session method now correctly passes the alternative_endpoints to serve_unix_async, enabling the exporter to use these endpoints during session creation.
packages/jumpstarter/jumpstarter/config/exporter.py (3)
5-5: Appropriate import additionThe import of
Listfrom typing is necessary for the new alternative_endpoints field type annotation.
85-86: Well-implemented attribute additionGood implementation of the alternative_endpoints attribute with proper type annotation and default_factory to avoid mutable default issues.
174-174: Correctly passes alternative_endpoints to ExporterThe serve method now properly passes the alternative_endpoints to the Exporter constructor, ensuring configuration is properly propagated.
packages/jumpstarter/jumpstarter/client/client.py (1)
30-30: Good parameter addition with sensible defaultAdding the optional use_alternative_endpoints parameter with a default of True maintains backward compatibility while enabling the new feature.
packages/jumpstarter/jumpstarter/exporter/tls.py (1)
12-31: Great job onparse_endpoint.The approach using
rpartition(':')and handling IPv6 bracket stripping is clear and robust. As a minor caution, consider confirming thathostis non-empty to avoid acceptance of malformed endpoints (e.g.,:8080).packages/jumpstarter/jumpstarter/exporter/session.py (3)
18-19: Thorough integration of alternative endpoints.
_alternative_endpointsis introduced and properly assigned if the parameter is non-null.- The
with_alternative_endpointscall ensures each endpoint is secured via TLS.However, be cautious when combining insecure and secure ports on the same server. Ensure the runtime usage scenario is well-understood and tested.
Also applies to: 57-65
83-85: Synchronous wrapper usage is straightforward.Wrapping
serve_unix_asyncwith an optionalalternative_endpointsparameter ensures consistent usage across both async and sync methods. This is helpful for clients that require synchronous flows.
77-79:Details
❓ Verification inconclusive
Confirm handling of Unix socket endpoints.
Serving a Unix socket while also binding alternative TLS endpoints is logical, but consider verifying or documenting that an empty list of alternative endpoints won't produce unexpected behavior.
🏁 Script executed:
#!/bin/bash # Description: Check if the code gracefully handles empty alternative_endpoints. grep -A5 "async def serve_unix_async" packages/jumpstarter/jumpstarter/exporter/session.pyLength of output: 354
Confirm behavior of empty
alternative_endpointsin Unix socket servingAfter reviewing the snippet, it's clear that the
serve_unix_asyncmethod passes thealternative_endpointsparameter directly toserve_ports_asyncwithout any additional guard or normalization when it’s an empty list. To avoid potential surprises at runtime, please verify thatserve_ports_asyncis designed to handle an empty list (as opposed toNone) correctly. If the existing behavior is intentional, consider adding inline documentation or a docstring note clarifying that an empty list is a supported and benign input.
- Location:
packages/jumpstarter/jumpstarter/exporter/session.py(lines 77–79)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py (1)
75-112: New fields for alternative endpoint handling look good.The addition of
_ENDPOINT, references toalternative_endpointsinGetReportResponse, and expansions forStreamDriverCallalign with the rest of the codebase. Implementation is consistent with the session changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/pyproject.toml (2)
19-19: Dependency Addition: Runtime Cryptography Inclusion
The addition of"cryptography>=43.0.3"to the runtime dependencies (line 19) is clear and aligns with the objective to support new secure channel capabilities. Please verify that the specified minimum version meets your security and feature requirements.
23-32: Dev Dependency Cleanup: Removal of Cryptography
The removal of"cryptography>=43.0.3"from the development dependency group helps to avoid redundancy and ensures that the package is only installed for runtime. Confirm that no development tooling or tests depend on cryptography being explicitly listed in the dev section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
packages/jumpstarter/pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
NickCao
force-pushed
the
direct
branch
2 times, most recently
from
be45de3 to
c7e12e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/client/client.py (1)
38-52:⚠️ Potential issueAdd error handling for failed connection attempts
The code attempts to connect to alternative endpoints without proper error handling. If connection to an endpoint fails, it will silently continue to the next one.
Apply this diff to log failed connection attempts:
if endpoint.certificate: + try: channels.append( grpc.aio.secure_channel( endpoint.endpoint, grpc.ssl_channel_credentials( root_certificates=endpoint.certificate.encode(), private_key=endpoint.client_private_key.encode(), certificate_chain=endpoint.client_certificate.encode(), ), ) ) + except Exception as e: + # Log failed connection attempt + import logging + logger = logging.getLogger(__name__) + logger.debug(f"Failed to connect to alternative endpoint {endpoint.endpoint}: {e}")
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/client.py (1)
36-36: Inefficient stub creationA temporary
SmartExporterStubis created just to callGetReport, then discarded. This is inefficient as it creates stub objects that are immediately thrown away.Consider refactoring to reuse the stub:
- response = await SmartExporterStub([channel]).GetReport(empty_pb2.Empty()) + temp_stub = SmartExporterStub([channel]) + response = await temp_stub.GetReport(empty_pb2.Empty())packages/jumpstarter/jumpstarter/client/grpc.py (1)
259-279: Improve stub method invocation performanceThe current implementation of
__getattr__checks the state of every channel on each method call, which could cause performance issues when dealing with many channels or frequent method calls.Consider implementing a caching mechanism for ready channels to avoid repeated connection attempts:
@dataclass(frozen=True, slots=True) class SmartExporterStub: channels: InitVar[list[Channel]] __stubs: dict[Channel, Any] = field(init=False, default_factory=OrderedDict) + __ready_channel: Any = field(init=False, default=None) + __ready_stub: Any = field(init=False, default=None) def __post_init__(self, channels): for channel in channels: stub = SimpleNamespace() jumpstarter_pb2_grpc.ExporterServiceStub.__init__(stub, channel) router_pb2_grpc.RouterServiceStub.__init__(stub, channel) self.__stubs[channel] = stub def __getattr__(self, name): + # First check if we have a cached ready channel + if self.__ready_channel is not None: + if self.__ready_channel.get_state(try_to_connect=False) == ChannelConnectivity.READY: + return getattr(self.__ready_stub, name) + # Reset cache if no longer ready + self.__ready_channel = None + self.__ready_stub = None + for channel, stub in self.__stubs.items(): # find the first channel that's ready if channel.get_state(try_to_connect=True) == ChannelConnectivity.READY: + # Cache the ready channel and stub + self.__ready_channel = channel + self.__ready_stub = stub return getattr(stub, name) # or fallback to the last channel (via router) return getattr(next(reversed(self.__stubs.values())), name)packages/jumpstarter/jumpstarter/client/core.py (1)
64-64: Improve type annotation for stubThe current type annotation for
stubisAny, which reduces type safety and IDE auto-completion capabilities.Use a more specific type annotation to improve code quality:
- stub: Any + stub: 'SmartExporterStub'Also, consider adding an import statement:
+from jumpstarter.client.grpc import SmartExporterStub
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
buf.gen.yaml(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/core.py(7 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(4 hunks)packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)packages/jumpstarter/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter/pyproject.toml
- packages/jumpstarter/jumpstarter/exporter/tls.py
- buf.gen.yaml
- packages/jumpstarter/jumpstarter/exporter/exporter.py
- packages/jumpstarter/jumpstarter/exporter/session.py
🧰 Additional context used
🧬 Code Definitions (2)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/config/client.py (1) (1)
channel(55-61)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (1) (1)
ExporterServiceStub(489-524)
packages/jumpstarter/jumpstarter/client/core.py (2)
packages/jumpstarter/jumpstarter/exporter/session.py (3) (3)
DriverCall(103-105)StreamingDriverCall(107-110)Stream(112-125)packages/jumpstarter/jumpstarter/driver/base.py (3) (3)
DriverCall(87-112)StreamingDriverCall(114-142)Stream(145-169)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (2)
packages/jumpstarter/jumpstarter/client/client.py (1)
53-53:Details
❓ Verification inconclusive
Clarify channel prioritization strategy
The code reverses the list of channels when creating the final stub, which means alternative endpoints are prioritized over the primary channel. This may not be the intended behavior, as typically the primary channel would be the preferred one.
🏁 Script executed:
#!/bin/bash # Search for comments or documentation about channel prioritization strategy rg -A 3 -B 3 "endpoint.*priorit|priorit.*endpoint|channel.*order|order.*channel" --type pyLength of output: 92
Channel Prioritization: Clarification Needed
It appears that reversing the channel list causes the last defined channel to be used as the primary endpoint. Since our search didn’t reveal any documentation or inline comments that specify this behavior, please verify if this inversion is intentional. If the primary channel should indeed be favored, consider either removing the reversal or adding explicit comments that justify this ordering.
- File: packages/jumpstarter/jumpstarter/client/client.py, line 53
- Code:
stub = SmartExporterStub(list(reversed(channels)))packages/jumpstarter/jumpstarter/client/core.py (1)
91-91: Updated stub usage looks goodThe changes from using individual service stubs to using the consolidated
SmartExporterStubare consistent and correctly implemented across all methods.Also applies to: 115-115, 130-134, 144-148, 162-163
NickCao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/core.py(7 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/env.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(4 hunks)packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)packages/jumpstarter/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter/jumpstarter/config/env.py
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter/pyproject.toml
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter/jumpstarter/exporter/exporter.py
- packages/jumpstarter/jumpstarter/client/core.py
- packages/jumpstarter/jumpstarter/exporter/tls.py
- packages/jumpstarter/jumpstarter/exporter/session.py
🧰 Additional context used
🧬 Code Definitions (3)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (1)
packages/jumpstarter/jumpstarter/common/utils.py (1) (1)
launch_shell(92-120)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/config/client.py (1) (1)
channel(57-63)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (1) (1)
ExporterServiceStub(489-524)
packages/jumpstarter/jumpstarter/client/client.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (1) (1)
SmartExporterStub(260-278)packages/jumpstarter/jumpstarter/client/base.py (1) (1)
DriverClient(19-99)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (17)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (1)
25-27: Pass new parameter tolaunch_shellsuccessfully
The addition ofconfig.useAlternativeEndpointsaligns with the updatedlaunch_shellfunction signature which expects a boolean foruse_alternative_endpoints. This enables proper handling of alternative endpoints without breaking existing code flow.packages/jumpstarter/jumpstarter/config/client.py (2)
53-54: NewuseAlternativeEndpointsattribute
Introducing this boolean attribute with a default ofFalsepreserves backward compatibility. A docstring or comment clarifying its usage could improve maintainability.
177-177: Passinguse_alternative_endpointsto theLeaseconstructor
Forwarding the new configuration flag ensures consistency. Good job tying this back to the new usage inLease.packages/jumpstarter/jumpstarter/client/lease.py (2)
44-44: Introduceuse_alternative_endpointsinLease
The default value ofFalseprevents unintended behavior while enabling future toggling for alternative endpoints.
188-195: Passing the flag inconnect_async
Propagatinguse_alternative_endpointstoclient_from_pathintegrates well with the rest of the configuration. This helps unify the lease handling logic with the client's ability to use alternative endpoints.packages/jumpstarter/jumpstarter/client/grpc.py (4)
3-7: Imports for multiple stubs and connectivity checks
These imports (e.g.,OrderedDict,SimpleNamespace,ChannelConnectivity, etc.) properly support the newSmartExporterStubclass and its multi-channel logic.Also applies to: 11-11, 13-13
259-264: IntroducingSmartExporterStubclass
Using a frozen dataclass withslots=Trueis a memory-efficient approach. Documenting its responsibilities or adding a docstring could improve clarity for other contributors.
265-270: Uncommon initialization pattern for stubs
This direct invocation of the stubs’__init__on aSimpleNamespacewas previously flagged. It's potentially fragile if stub implementations change. Adopting the conventional approach (e.g., assigning actualExporterServiceStub(channel)instances to attributes) is recommended.
272-278: Fallback logic for no ready channels
As noted in the past review, consider more robust error handling or logging when no channels are ready. Documenting the fallback approach helps clarify how the code behaves in unexpected situations.packages/jumpstarter/jumpstarter/client/client.py (5)
10-10: Good choice using SmartExporterStubThe switch from ExporterServiceStub to SmartExporterStub enables the multi-channel functionality needed for alternative endpoints.
43-44: Architecture change to support multiple channelsThe code now initializes SmartExporterStub with a list containing just the original channel, setting up the foundation for alternative endpoint support.
45-59: Address TODOs for error handling and token authenticationThe implementation for alternative endpoint connection looks good, but you should address the error handling when connecting to alternative endpoints.
Consider implementing error handling as previously suggested:
for endpoint in response.alternative_endpoints: if endpoint.certificate: + try: channels.append( grpc.aio.secure_channel( endpoint.endpoint, grpc.ssl_channel_credentials( root_certificates=endpoint.certificate.encode(), private_key=endpoint.client_private_key.encode(), certificate_chain=endpoint.client_certificate.encode(), ), ) ) + except Exception as e: + # Log failed connection attempt + logger.debug(f"Failed to connect to alternative endpoint {endpoint.endpoint}: {e}")Also add the logger import at the top of the file:
import logging logger = logging.getLogger(__name__)
60-60:Details
❓ Verification inconclusive
Consider the channel priority logic
You're reversing the channels list before passing to SmartExporterStub, which prioritizes alternative endpoints over the original channel. Ensure this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Find where SmartExporterStub is used to understand channel prioritization rg -A 2 "SmartExporterStub" --type pyLength of output: 1052
Channel Priority Logic: Confirm Intentional Reversal
The code currently initializes
SmartExporterStubwith a reversed list of channels (i.e.list(reversed(channels))), which means the endpoint originally positioned last in thechannelslist becomes the primary one. We also see another usage where a single channel is passed directly. Please confirm that this inversion is deliberate—specifically, that you intend for alternative endpoints (from the reversed list) to take priority over the original channel. If so, consider adding a clarifying comment to highlight this behavior.
80-80:Details
❓ Verification inconclusive
API change from
channeltostubChanging the parameter from
channeltostubis a significant API change. Ensure all client implementations are updated to work with this new parameter.
🏁 Script executed:
#!/bin/bash # Find all client implementations that might be using the channel parameter ast-grep --pattern "client = client_class(\n $$$\n channel=$_,\n $$$\n)"Length of output: 94
I'm not getting a definitive verification from the ast-grep output. It looks like the command didn't return clear results regarding usages of the legacy
channelparameter. To be sure that all client implementations have been updated to use the newstubparameter, please run an additional search across the repository. For example, you can try the following shell script to search for any remaining occurrences of"channel=":#!/bin/bash # Search for any remaining occurrences of the legacy "channel=" usage in the repository rg --ignore-case "channel=" -n .If this search produces any hits, please review them and update the corresponding client implementations to use
stubinstead. If no results appear, then the API change appears to have been fully applied.Action: Please verify that no implementations still reference
channel.
• File: packages/jumpstarter/jumpstarter/client/client.py (line 80) already usesstub=stub,.
• Next Step: Run the above script and manually confirm no legacy usages remain.packages/jumpstarter/jumpstarter/common/utils.py (3)
10-10: Good addition of environment variable constantAdding the JMP_USE_ALTERNATIVE_ENDPOINTS constant is a clean approach to propagate the setting through the environment.
55-56: Good environment variable handlingThe conversion from string environment variable to boolean is clean and maintainable.
116-116: Good environment variable settingThe environment variable setting is consistent with how the value is read in
env_async().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
210-210: LGTM: New configuration field for alternative endpoints added consistently.The
useAlternativeEndpoints: falsefield has been consistently added to all relevant test configurations, which matches the behavior described in the PR objectives for direct client-to-exporter connectivity.Consider adding a specific test case that verifies the behavior when this field is set to
trueto ensure that both states of this feature are properly tested.Also applies to: 247-247, 282-282
packages/jumpstarter/jumpstarter/common/utils_test.py (1)
8-14: LGTM: Updated function calls with new parameter.The
launch_shellfunction calls have been properly updated to include the newuse_alternative_endpointsparameter, which aligns with the function signature change in the implementation file.Consider adding a test case with
use_alternative_endpoints=Trueto verify that the alternative endpoints functionality works correctly when enabled.Also applies to: 18-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/common/utils_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter/jumpstarter/common/utils_test.py (1)
packages/jumpstarter/jumpstarter/common/utils.py (1) (1)
launch_shell(92-120)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/config/client.py (1)
53-54: Consider using field_alias convention for better Python/JSON compatibilityThe addition of
use_alternative_endpointswith a camelCase alias is good, but consider following the pattern used in the jumpstarter_kubernetes package where snake_case is used in Python and camelCase in output YAML/JSON.- use_alternative_endpoints: bool = Field(alias="useAlternativeEndpoints", default=False) + use_alternative_endpoints: bool = Field( + alias="useAlternativeEndpoints", + description="Whether to use alternative endpoints for direct client connection", + default=False + )This approach maintains Python standards while ensuring proper serialization to YAML/JSON. Adding a description would also improve code documentation.
packages/jumpstarter/jumpstarter/exporter/tls.py (5)
13-15: Add docstring and consider parameterizing RSA key parameters.The function lacks documentation explaining its purpose, parameters, and return value. Additionally, consider parameterizing the RSA key parameters (key size, public exponent) to allow for flexibility.
def with_alternative_endpoints(server, endpoints: list[str]): + """Set up mutual TLS authentication for alternative endpoints. + + Args: + server: The gRPC server instance to add secure ports to + endpoints: A list of endpoint strings to secure + + Returns: + List of Endpoint protocol buffer messages containing the endpoint information + and authentication credentials + """ + # RSA parameters + KEY_SIZE = 2048 + PUBLIC_EXPONENT = 65537 + - key = rsa.generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend()) - client_key = rsa.generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend()) + key = rsa.generate_private_key(public_exponent=PUBLIC_EXPONENT, key_size=KEY_SIZE, backend=default_backend()) + client_key = rsa.generate_private_key(public_exponent=PUBLIC_EXPONENT, key_size=KEY_SIZE, backend=default_backend())
17-37: Document certificate generation and consider adding SAN to client certificate.The certificate generation process should be documented to explain the reasoning behind using self-signed certificates. Additionally, consider adding a Subject Alternative Name to the client certificate for consistency with the server certificate.
+ # Generate self-signed server certificate crt = ( x509.CertificateBuilder() .subject_name(x509.Name([])) .issuer_name(x509.Name([])) .public_key(key.public_key()) .serial_number(x509.random_serial_number()) .not_valid_before(datetime.now()) .not_valid_after(datetime.now() + timedelta(days=365)) .add_extension(x509.SubjectAlternativeName([x509.DNSName(SAN)]), critical=False) .sign(private_key=key, algorithm=hashes.SHA256(), backend=default_backend()) ) + + # Generate self-signed client certificate client_crt = ( x509.CertificateBuilder() .subject_name(x509.Name([])) .issuer_name(x509.Name([])) .public_key(client_key.public_key()) .serial_number(x509.random_serial_number()) .not_valid_before(datetime.now()) .not_valid_after(datetime.now() + timedelta(days=365)) + .add_extension(x509.SubjectAlternativeName([x509.DNSName(SAN)]), critical=False) .sign(private_key=client_key, algorithm=hashes.SHA256(), backend=default_backend()) )
39-51: Document why keys are not encrypted.The code is serializing private keys without encryption. While this might be acceptable for ephemeral keys used in-memory, it's worth documenting the reasoning to clarify the security implications.
+ # Serialize server certificate and key pem_crt = crt.public_bytes(serialization.Encoding.PEM) pem_key = key.private_bytes( encoding=serialization.Encoding.PEM, format=serialization.PrivateFormat.TraditionalOpenSSL, + # Keys are not encrypted as they're only used in-memory for the duration of the connection encryption_algorithm=serialization.NoEncryption(), ) + # Serialize client certificate and key pem_client_crt = client_crt.public_bytes(serialization.Encoding.PEM) pem_client_key = client_key.private_bytes( encoding=serialization.Encoding.PEM, format=serialization.PrivateFormat.TraditionalOpenSSL, + # Keys are not encrypted as they're only used in-memory for the duration of the connection encryption_algorithm=serialization.NoEncryption(), )
53-55: Add comment explaining mutual TLS setup.This section sets up mutual TLS (mTLS) authentication requiring client certificate verification. Adding a comment would clarify the security model being implemented.
+ # Set up mutual TLS authentication requiring client certificate verification server_credentials = grpc.ssl_server_credentials( [(pem_key, pem_crt)], root_certificates=pem_client_crt, require_client_auth=True )
57-68: Add error handling and consider logging.The code adds secure ports and builds endpoint information without any error handling. Consider adding error handling for potential failures in adding secure ports to the server.
endpoints_pb = [] for endpoint in endpoints: - server.add_secure_port(endpoint, server_credentials) + try: + port = server.add_secure_port(endpoint, server_credentials) + if port == 0: + # Port 0 indicates failure in some gRPC implementations + print(f"Warning: Failed to add secure port for endpoint {endpoint}") + except Exception as e: + print(f"Error adding secure port for endpoint {endpoint}: {e}") + continue endpoints_pb.append( jumpstarter_pb2.Endpoint( endpoint=endpoint, certificate=pem_crt, client_certificate=pem_client_crt, client_private_key=pem_client_key, ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/env.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter/jumpstarter/config/env.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py
- packages/jumpstarter/jumpstarter/common/utils.py
🧰 Additional context used
🧬 Code Definitions (2)
packages/jumpstarter/jumpstarter/config/client.py (3)
packages/jumpstarter/jumpstarter/config/exporter.py (2) (2)
dump_yaml(122-123)dump_yaml(189-190)packages/jumpstarter/jumpstarter/client/grpc.py (3) (3)
dump_yaml(102-103)dump_yaml(120-121)dump_yaml(138-139)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py (1) (1)
dump_yaml(11-12)
packages/jumpstarter/jumpstarter/client/client.py (3)
packages/jumpstarter/jumpstarter/client/grpc.py (1) (1)
SmartExporterStub(260-278)packages/jumpstarter/jumpstarter/client/base.py (1) (1)
DriverClient(19-99)packages/jumpstarter/jumpstarter/exporter/session.py (1) (1)
GetReport(91-101)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (12)
packages/jumpstarter/jumpstarter/config/client.py (3)
177-177: Good handling of new parameter in lease_asyncThe parameter is correctly passed to the Lease constructor from the client config.
241-249: Correct use of by_alias parameter in model_dumpThe addition of
by_alias=Trueensures the field will be serialized using its camelCase alias as specified in the Field definition, which is consistent with the formatting standards across the codebase.
254-261: Consistent implementation of by_alias in dump_yamlThe change to include
by_alias=Truein dump_yaml is aligned with other similar methods in the codebase, maintaining consistency in YAML serialization.packages/jumpstarter/jumpstarter/client/client.py (8)
10-10: Smart refactoring of exporter stub importThe import now uses
SmartExporterStubwhich handles multiple gRPC channels, supporting the alternative endpoints feature.
13-13: Import of SAN (Subject Alternative Name) for TLS configurationThe import of
SANfrom the TLS module is necessary for secure connections to alternative endpoints.
17-24: Properly extended function signature for alternative endpoints supportGood addition of the optional
use_alternative_endpointsparameter with a default value ofFalse, maintaining backward compatibility.
28-28: Consistent propagation of the use_alternative_endpoints parameterCorrectly passing the parameter from
client_from_pathtoclient_from_channel.
37-37: Parameter consistency in client_from_channel functionThe
use_alternative_endpointsparameter is consistently added with the same default value.
44-45: Improved stub initialization using SmartExporterStubThe code now uses the more flexible
SmartExporterStubthat can manage multiple channels, which is essential for the alternative endpoints feature.
82-82: Updated client constructor parameter from channel to stubThe change from passing
channelto passingstubto the client constructor correctly adapts to the new multi-channel architecture.
46-62:Details
✅ Verification successful
Implement alternative endpoints connection
The implementation correctly processes alternative endpoints when enabled, creating secure channels with proper TLS configuration.
Let's verify that the secure channel configuration is correct:
🏁 Script executed:
#!/bin/bash # Description: Check the format and structure of SAN (Subject Alternative Name) constant in the TLS module # to ensure it's properly used in the SSL target name override option. # Find the definition of SAN in the codebase rg -A 2 "SAN\s*=" --type pyLength of output: 196
Secure Channel Alternative Endpoints Verified
The TLS configuration for alternative endpoints is correctly implemented. Verification confirms that the SAN constant, used in the
grpc.ssl_target_name_overrideoption, is defined inpackages/jumpstarter/jumpstarter/exporter/tls.pyas"localhost". This aligns with the intended configuration for secure channel setup.
- File reviewed:
packages/jumpstarter/jumpstarter/client/client.py(Lines 46–62)- Key detail:
SANis defined as"localhost"inpackages/jumpstarter/jumpstarter/exporter/tls.pyNo further changes are needed.
packages/jumpstarter/jumpstarter/exporter/tls.py (1)
1-11: Add documentation and improve constants.The file lacks descriptive documentation for its purpose and usage. Consider adding a module-level docstring that explains what this module does and why it's needed (peer-to-peer mTLS).
Additionally, the constant
SANis hardcoded as "localhost". Consider making this configurable or at least adding a comment explaining why it's set to "localhost".from jumpstarter_protocol import jumpstarter_pb2 +# Certificate Subject Alternative Name SAN = "localhost"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter/jumpstarter/exporter/tls.py (5)
10-10: Document the purpose of SAN constantThe SAN constant is used for the Subject Alternative Name in certificates, but its purpose and why "localhost" is used isn't immediately clear from the code context.
-SAN = "localhost" +# Using a fixed SAN of "localhost" since we're skipping SAN verification on the client side +# with certificate pinning for security +SAN = "localhost"
32-42: Extract certificate validity period as a constantThe certificate validity period is hardcoded to 365 days. Consider making this a named constant for better maintainability.
+# Certificate validity period +CERT_VALIDITY_DAYS = 365 + crt = ( x509.CertificateBuilder() .subject_name(x509.Name([])) .issuer_name(x509.Name([])) .public_key(key.public_key()) .serial_number(x509.random_serial_number()) .not_valid_before(datetime.now()) - .not_valid_after(datetime.now() + timedelta(days=365)) + .not_valid_after(datetime.now() + timedelta(days=CERT_VALIDITY_DAYS)) .add_extension(x509.SubjectAlternativeName([x509.DNSName(SAN)]), critical=False) .sign(private_key=key, algorithm=hashes.SHA256(), backend=default_backend()) )
43-52: Consider adding SAN to client certificateThe server certificate has a Subject Alternative Name (SAN) extension, but the client certificate doesn't. While the client certificate might not need hostname verification, adding a SAN for consistency might be beneficial.
client_crt = ( x509.CertificateBuilder() .subject_name(x509.Name([])) .issuer_name(x509.Name([])) .public_key(client_key.public_key()) .serial_number(x509.random_serial_number()) .not_valid_before(datetime.now()) - .not_valid_after(datetime.now() + timedelta(days=365)) + .not_valid_after(datetime.now() + timedelta(days=CERT_VALIDITY_DAYS)) + .add_extension(x509.SubjectAlternativeName([x509.DNSName(SAN)]), critical=False) .sign(private_key=client_key, algorithm=hashes.SHA256(), backend=default_backend()) )
54-66: Document security implications of NoEncryption for private keysThe private keys are serialized without encryption. While this might be appropriate for ephemeral keys used in a controlled environment, it's worth documenting this design decision.
pem_key = key.private_bytes( encoding=serialization.Encoding.PEM, format=serialization.PrivateFormat.TraditionalOpenSSL, + # No encryption is used as these keys are ephemeral and only used for the duration of a lease encryption_algorithm=serialization.NoEncryption(), )
84-84: Consider adding basic error handlingThe function assumes all operations succeed but doesn't handle potential failures in certificate generation or adding secure ports. Consider adding try-except blocks for robustness.
- return endpoints_pb + try: + return endpoints_pb + except Exception as e: + # Log the error and/or clean up any partially created resources + raise RuntimeError(f"Failed to set up alternative endpoints: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (4)
packages/jumpstarter/jumpstarter/exporter/tls.py (4)
13-27: Good documentation of mTLS implementation and rationaleThe detailed docstring clearly explains:
- The purpose of alternative endpoints
- When they're useful (network constraints)
- The security model using mTLS
- The connection flow and fallback mechanism
This helps maintainers understand the security architecture and design decisions.
29-30: Extract cryptographic parameters as constantsThe RSA key generation parameters are hardcoded. For better maintainability and to follow the suggestion in past reviews, consider extracting these values as named constants.
+# RSA key generation parameters +RSA_KEY_SIZE = 2048 +RSA_PUBLIC_EXPONENT = 65537 + - key = rsa.generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend()) - client_key = rsa.generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend()) + key = rsa.generate_private_key(public_exponent=RSA_PUBLIC_EXPONENT, key_size=RSA_KEY_SIZE, backend=default_backend()) + client_key = rsa.generate_private_key(public_exponent=RSA_PUBLIC_EXPONENT, key_size=RSA_KEY_SIZE, backend=default_backend())
68-70: LGTM - Server credentials properly configured for mTLSThe server credentials are correctly configured to require client authentication, implementing mutual TLS as described in the docstring.
72-83: Properly handles multiple alternative endpointsThe code correctly iterates through all provided endpoints, adds each to the server with secure credentials, and builds appropriate protobuf messages containing all necessary information for clients to establish secure connections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
272-278: Implement attribute error handlingThe current implementation doesn't handle the case where the requested attribute doesn't exist on the stub objects. This could lead to confusing error messages since the AttributeError would come from the last stub, not necessarily from the one that would have been used.
def __getattr__(self, name): + # First check if the attribute exists on any stub + attr_exists = False + for stub in self.__stubs.values(): + if hasattr(stub, name): + attr_exists = True + break + + if not attr_exists: + raise AttributeError(f"'{self.__class__.__name__}' has no attribute '{name}'") + for channel, stub in self.__stubs.items(): # find the first channel that's ready if channel.get_state(try_to_connect=True) == ChannelConnectivity.READY: return getattr(stub, name) # or fallback to the last channel (via router) return getattr(next(reversed(self.__stubs.values())), name)packages/jumpstarter/jumpstarter/common/utils.py (1)
100-107: Update function docstring to include the new parameterThe function docstring should be updated to document the
use_alternative_endpointsparameter, explaining its purpose and effect when enabled."""Launch a shell with a custom prompt indicating the exporter type. Args: host: The jumpstarter host path context: The context of the shell ("local" or "remote") allow: List of allowed drivers unsafe: Whether to allow drivers outside of the allow list + use_alternative_endpoints: Whether to use alternative endpoints for communication """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
packages/jumpstarter-cli/jumpstarter_cli/shell.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/core.py(7 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/common/utils_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)packages/jumpstarter/jumpstarter/config/env.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(4 hunks)packages/jumpstarter/jumpstarter/exporter/tls.py(1 hunks)packages/jumpstarter/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/jumpstarter/jumpstarter/config/env.py
- packages/jumpstarter/jumpstarter/exporter/exporter.py
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter/pyproject.toml
- packages/jumpstarter/jumpstarter/config/client_config_test.py
- packages/jumpstarter/jumpstarter/client/core.py
- packages/jumpstarter/jumpstarter/exporter/tls.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/client/client.py
- packages/jumpstarter/jumpstarter/common/utils_test.py
- packages/jumpstarter/jumpstarter/exporter/session.py
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
channel(57-63)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (1)
ExporterServiceStub(489-524)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (8)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
266-270: Update stub initialization to use standard constructor callsThe stub initialization approach here directly calls
__init__methods on aSimpleNamespaceobject, which is an unconventional pattern that might cause issues if the stub implementations change.- for channel in channels: - stub = SimpleNamespace() - jumpstarter_pb2_grpc.ExporterServiceStub.__init__(stub, channel) - router_pb2_grpc.RouterServiceStub.__init__(stub, channel) - self.__stubs[channel] = stub + for channel in channels: + stub = SimpleNamespace( + exporter=jumpstarter_pb2_grpc.ExporterServiceStub(channel), + router=router_pb2_grpc.RouterServiceStub(channel) + ) + self.__stubs[channel] = stub
272-278: Handle case when no channels are readyThe
__getattr__method falls back to the last channel when no channels are ready, but it doesn't handle potential errors or document this behavior.def __getattr__(self, name): for channel, stub in self.__stubs.items(): # find the first channel that's ready if channel.get_state(try_to_connect=True) == ChannelConnectivity.READY: return getattr(stub, name) + + if not self.__stubs: + raise RuntimeError("No channels available to create stub") + # or fallback to the last channel (via router) + import logging + logger = logging.getLogger(__name__) + logger.debug("No ready channels found, falling back to last channel") return getattr(next(reversed(self.__stubs.values())), name)
259-263: LGTM: Well-designed class structureThe
SmartExporterStubis well-designed with a frozen dataclass that manages multiple channels and their corresponding stubs. The use ofOrderedDictensures consistent fallback behavior when no ready channels are found.packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
34-40: Parameter added correctly to support alternative endpointsThe new parameter
use_alternative_endpointsis properly passed from the config object to thelaunch_shellfunction, following the established pattern for other configuration parameters. This change integrates well with the overall PR objective of adding support for alternative endpoints.packages/jumpstarter/jumpstarter/common/utils.py (4)
10-10: Import added correctly for new environment variableThe import statement correctly includes the new
JMP_USE_ALT_ENDPOINTSenvironment variable from the appropriate module.
55-64: Environment variable handling implemented properly in env_asyncThe changes properly retrieve and parse the
JMP_USE_ALT_ENDPOINTSenvironment variable, converting it to a boolean value. The implementation follows the pattern established for other environment variables in the codebase, providing a sensible default of "0" (false) if the variable is not set.The parameter is correctly passed to the
client_from_pathfunction, maintaining a consistent approach to configuration handling.
92-99: Function signature updated with backward compatibilityThe
launch_shellfunction signature has been properly updated to includeuse_alternative_endpointsas a keyword-only parameter with a default value ofFalse. This approach maintains backward compatibility while adding the new functionality, addressing the feedback from previous reviews.Using a keyword-only parameter (with
*,) is an appropriate design choice as it enforces explicit naming when this parameter is used, which improves code readability.
122-122: Environment variable set correctly in launch_shellThe
JMP_USE_ALT_ENDPOINTSenvironment variable is properly set based on the function parameter value, following the established pattern for other environment variables in this function.
Summary by CodeRabbit