Conversation
so it doesn't affect JSON output
|
Warning Rate limit exceeded@lionello has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughRefactors tenant/workspace typing and propagation across CLI, client, and auth layers: replaces DEFANG_ORG with DEFANG_WORKSPACE, introduces TenantLabel and TenantNameOrID flag behavior, changes Connect/ConnectWithTenant and NewGrpcClient signatures, removes tenant-selection helper and context from SetupCommands, and updates login/auth flows to be tenant-aware. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI User
participant CLI as Commands
participant Auth as Auth Service
participant Conn as Connect Manager
participant Client as GrpcClient
participant Server as Defang Server
Note over User,CLI: User runs a command that may require auth
User->>CLI: invoke command
CLI->>CLI: ensure global.Tenant set
alt Not authenticated
CLI->>Auth: InteractiveLogin(cluster)
Auth->>Auth: OAuth/device flow → token
Auth-->>CLI: token
CLI->>Conn: ConnectWithTenant(ctx, cluster, global.Tenant)
else Authenticated or direct connect
CLI->>Conn: ConnectWithTenant(ctx, cluster, global.Tenant)
end
Conn->>Client: NewGrpcClient(host, token, requestedTenant)
Client->>Server: WhoAmI (Tenant header = requestedTenant if set)
Server-->>Client: server-validated tenant
Client->>Conn: update Client.Tenant = server tenant
Conn-->>CLI: return validated client
CLI->>Client: Execute gRPC request
Client->>Server: gRPC call
Server-->>Client: response
Client-->>CLI: result
CLI-->>User: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/cli/client/byoc/gcp/byoc.go (1)
362-364: Fix DEFANG_ORG to useb.TenantNamefor consistency across providersGCP hardcodes
DEFANG_ORGto"defang"while AWS and DO both usestring(b.TenantName). This inconsistency may cause unexpected behavior when tenant-specific logic is required. The TODO comment in GCP byoc.go (line 362) acknowledges this needs clarification.
🧹 Nitpick comments (3)
src/pkg/cli/client/byoc/baseclient.go (1)
301-306: Consider adding validation for emptyprojectName.The method does not validate
projectName. If it's empty, the result would be malformed (e.g.,name..tenant.defang.app). Other methods in this file usepkg.Ensure(projectName != "", ...)for validation.🔎 Proposed validation
func (b ByocBaseClient) ServicePublicDNS(name string, projectName string) string { + pkg.Ensure(projectName != "", "ProjectName not set") if b.PulumiStack != "" && b.PulumiStack != "beta" { projectName = projectName + "-" + b.PulumiStack } return dns.SafeLabel(name) + "." + dns.SafeLabel(projectName) + "." + dns.SafeLabel(string(b.TenantName)) + ".defang.app" }src/pkg/login/login.go (2)
57-59: Consider removing or documenting commented-out code.These lines are commented out without explanation. If they're being removed as part of the refactor (since the new login page shows ToS), either remove them entirely or add a comment explaining why they're preserved.
🔎 Suggested cleanup
- // tenant := cli.TenantFromToken(token) // show the tenant implied by the freshly issued token - // host := client.NormalizeHost(cluster) - // term.Info("Successfully logged in to", host, "("+tenant.String()+" tenant)") -
74-76: Consider removing commented-out ToS code.The comment on line 73 explains this is no longer needed. Consider removing the dead code entirely.
🔎 Suggested cleanup
// The new login page shows the ToS so a successful login implies the user agreed - // if err := NonInteractiveAgreeToS(ctx, fabric); err != nil { - // term.Debug("unable to agree to terms:", err) // not fatal - // } return nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
README.mdpkgs/npm/README.mdsrc/README.mdsrc/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/workspace.gosrc/cmd/cli/main.gosrc/cmd/gendocs/main.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/agent/tools/services_test.gosrc/pkg/auth/interceptor.gosrc/pkg/cli/activeDeployments_test.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/client/mock.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/configList_test.gosrc/pkg/cli/connect.gosrc/pkg/cli/connect_test.gosrc/pkg/cli/deploymentsList_test.gosrc/pkg/cli/destroy_test.gosrc/pkg/cli/getServices_test.gosrc/pkg/clouds/aws/route53_test.gosrc/pkg/login/agree_tos.gosrc/pkg/login/login.gosrc/pkg/login/login_test.gosrc/pkg/term/colorizer.gosrc/pkg/term/colorizer_test.gosrc/pkg/types/tenant.go
🧰 Additional context used
🧬 Code graph analysis (33)
src/pkg/agent/tools/services_test.go (1)
src/pkg/login/login.go (1)
InteractiveLoginMCP(47-49)
src/pkg/cli/client/client.go (1)
src/pkg/types/tenant.go (1)
TenantName(4-4)
src/cmd/gendocs/main.go (1)
src/cmd/cli/command/commands.go (1)
SetupCommands(145-371)
src/cmd/cli/command/globals.go (1)
src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)
src/pkg/clouds/aws/route53_test.go (1)
src/pkg/clouds/aws/route53.go (1)
GetDelegationSetByZone(54-63)
src/pkg/cli/client/byoc/do/byoc.go (3)
src/pkg/cli/client/byoc/aws/byoc.go (1)
NewByocProvider(126-133)src/pkg/cli/client/byoc/gcp/byoc.go (1)
NewByocProvider(105-111)src/pkg/types/tenant.go (1)
TenantName(4-4)
src/pkg/cli/client/mock.go (1)
src/protos/io/defang/v1/defangv1connect/fabric.connect.go (1)
FabricControllerClient(163-217)
src/pkg/agent/tools/deploy.go (4)
src/pkg/login/login.go (1)
InteractiveLoginMCP(47-49)src/pkg/agent/common/common.go (1)
MCPDevelopmentClient(12-12)src/pkg/auth/auth.go (1)
ErrNoBrowser(18-21)src/pkg/cli/connect.go (1)
Connect(15-21)
src/pkg/agent/tools/default_tool_cli.go (3)
src/pkg/cli/connect.go (1)
ConnectWithTenant(23-34)src/pkg/types/tenant.go (1)
TenantUnset(12-12)src/pkg/login/login.go (1)
InteractiveLoginMCP(47-49)
src/pkg/cli/client/byoc/baseclient.go (3)
src/pkg/types/tenant.go (1)
TenantName(4-4)src/pkg/utils.go (1)
Getenv(48-53)src/pkg/dns/utils.go (1)
SafeLabel(5-7)
src/pkg/login/login_test.go (2)
src/pkg/login/login.go (2)
LoginFlow(20-20)InteractiveLogin(43-45)src/pkg/auth/auth.go (1)
LoginFlow(33-33)
src/pkg/cli/activeDeployments_test.go (2)
src/pkg/cli/connect.go (1)
Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/login/agree_tos.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/client/state.go (1)
TermsAccepted(63-65)
src/pkg/agent/tools/interfaces.go (1)
src/pkg/login/login.go (1)
InteractiveLoginMCP(47-49)
src/pkg/cli/client/byoc/aws/byoc.go (3)
src/pkg/cli/client/byoc/do/byoc.go (1)
NewByocProvider(65-79)src/pkg/cli/client/byoc/gcp/byoc.go (1)
NewByocProvider(105-111)src/pkg/types/tenant.go (1)
TenantName(4-4)
src/cmd/cli/command/commands_test.go (1)
src/cmd/cli/command/commands.go (1)
SetupCommands(145-371)
src/pkg/cli/client/byoc/gcp/byoc.go (3)
src/pkg/cli/client/byoc/aws/byoc.go (1)
NewByocProvider(126-133)src/pkg/cli/client/byoc/do/byoc.go (1)
NewByocProvider(65-79)src/pkg/types/tenant.go (1)
TenantName(4-4)
src/pkg/cli/configList_test.go (2)
src/pkg/cli/connect.go (1)
Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/agent/tools/deploy_test.go (1)
src/pkg/login/login.go (1)
InteractiveLoginMCP(47-49)
src/pkg/login/login.go (4)
src/pkg/auth/auth.go (3)
LoginFlow(33-33)CliFlow(36-36)McpFlow(37-37)src/pkg/dryrun/dryrun.go (2)
DoDryRun(6-6)ErrDryRun(8-8)src/pkg/cli/connect.go (1)
ConnectWithTenant(23-34)src/pkg/track/track.go (1)
Tracker(23-25)
src/pkg/cli/getServices_test.go (2)
src/pkg/cli/connect.go (1)
Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/cli/destroy_test.go (2)
src/pkg/cli/connect.go (1)
Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/cli/client/grpc.go (2)
src/pkg/types/tenant.go (3)
TenantNameOrID(8-8)TenantNameOrID(31-33)TenantName(4-4)src/pkg/auth/interceptor.go (1)
NewAuthInterceptor(18-21)
src/pkg/term/colorizer_test.go (1)
src/pkg/term/colorizer.go (1)
Errorf(322-324)
src/pkg/cli/connect_test.go (2)
src/pkg/cli/connect.go (2)
ConnectWithTenant(23-34)Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/auth/interceptor.go (2)
src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)src/pkg/http/client.go (1)
Header(13-13)
src/pkg/cli/client/byoc/aws/byoc_test.go (1)
src/pkg/types/tenant.go (1)
TenantName(4-4)
src/pkg/cli/client/playground.go (3)
src/pkg/cli/client/provider.go (2)
ServerStream(36-41)BootstrapCommandRequest(20-23)src/pkg/cli/tail.go (1)
Tail(118-153)src/pkg/cli/bootstrap.go (1)
BootstrapCommand(16-39)
src/cmd/cli/command/commands.go (4)
src/pkg/cli/connect.go (2)
Connect(15-21)ConnectWithTenant(23-34)src/pkg/login/login.go (1)
InteractiveLogin(43-45)src/pkg/cli/whoami.go (1)
Whoami(25-59)src/pkg/cli/token.go (1)
Token(16-54)
src/cmd/cli/main.go (1)
src/cmd/cli/command/commands.go (1)
SetupCommands(145-371)
src/cmd/cli/command/globals_test.go (1)
src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)
src/pkg/cli/deploymentsList_test.go (2)
src/pkg/cli/connect.go (1)
Connect(15-21)src/pkg/types/tenant.go (1)
TenantUnset(12-12)
src/pkg/cli/connect.go (3)
src/pkg/types/tenant.go (3)
TenantNameOrID(8-8)TenantNameOrID(31-33)TenantName(4-4)src/pkg/cli/client/cluster.go (2)
NormalizeHost(18-26)GetExistingToken(44-69)src/pkg/term/colorizer.go (2)
Debugf(298-300)Debug(294-296)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (65)
src/pkg/term/colorizer_test.go (1)
96-124: LGTM! Improved test coverage for stderr routing.The variable renaming (
expected→expectedOut,got→gotOut) and the addition of stderr validation (expectedErr,gotErr) improve test clarity and ensure that debug output is correctly routed to stderr. This change aligns with the commit message about routing debug output to stderr so it doesn't affect JSON output.src/pkg/clouds/aws/route53_test.go (1)
15-16: Test is marked as broken and skipped.The test is currently skipped, which prevents it from catching issues introduced by the refactoring. While the skip acknowledges known problems, the underlying issue (see comments on lines 39 and 61) should be resolved to restore test coverage for delegation set operations.
src/pkg/term/colorizer.go (1)
201-201: LGTM! Debug output correctly routed to stderr.This change ensures debug messages don't interfere with structured output (e.g., JSON) written to stdout, which is a standard CLI best practice.
Also applies to: 208-208
pkgs/npm/README.md (1)
51-51: Consistent with root README—same backward compatibility concern applies.The
DEFANG_WORKSPACErename here mirrors the change in the root README.md. Ensure backward compatibility handling forDEFANG_ORGapplies consistently across all documentation.src/pkg/agent/tools/services_test.go (1)
93-95: LGTM! Mock signature correctly updated.The mock's
InteractiveLoginMCPsignature now matches the updated interface, removing the fabric client parameter as part of the broader refactor to centralize client handling.src/README.md (1)
51-51: Documentation consistent across READMEs.The
DEFANG_WORKSPACEupdate is consistent with the root and npm README files. The same backward compatibility verification applies here.src/pkg/login/login_test.go (2)
49-51: Mock signature correctly updated to match the refactored API.The mock login method no longer requires the fabric client parameter, aligning with the simplified authentication flow.
71-71: Test call sites correctly updated.The
InteractiveLogincalls now pass only the fabric string, consistent with the updated function signature. Token storage validation logic remains intact.Also applies to: 87-87
src/cmd/gendocs/main.go (1)
21-21: LGTM! Consistent with the updated SetupCommands API.The removal of the context parameter aligns with the simplified
SetupCommandssignature. For documentation generation, context is not required.README.md (1)
181-181: No action required—backward compatibility is already implemented.The
DEFANG_ORG→DEFANG_WORKSPACErename maintains backward compatibility through a fallback mechanism that recognizesDEFANG_ORGwhenDEFANG_WORKSPACEis not set, and displays a deprecation warning to users.src/cmd/cli/command/workspace.go (1)
25-25: The direct access toglobal.Tenantis safe and properly initialized.The flag is registered via Cobra's
Var()method which handles custom type parsing, and Cobra marks required flags and validates relationships in PreRunE. Theglobal.Tenantfield is initialized through Cobra's PersistentFlags system incommands.goand synchronized with environment variables (DEFANG_WORKSPACE or DEFANG_ORG) viasyncFlagsWithEnv()before command execution. No validation or fallback logic from agetTenantSelection()function needs to be preserved, as that function does not exist in the current codebase.src/pkg/cli/client/byoc/aws/byoc_test.go (1)
31-31: LGTM: Type change aligns with refactoring objectives.The field type change from
types.TenantNameOrIDtotypes.TenantNamecorrectly reflects the stricter type semantics introduced in this PR, distinguishing tenant names (DNS labels) from tenant IDs.src/pkg/cli/getServices_test.go (1)
71-71: LGTM: Connect call updated to match new signature.The change correctly uses
types.TenantUnsetto indicate that no specific tenant/workspace is being selected for this test connection.src/cmd/cli/main.go (1)
35-35: LGTM: API surface change correctly applied.The removal of the
ctxparameter fromSetupCommandsis appropriate—command setup doesn't require cancellation support, while command execution (line 36) still receives the context.src/pkg/cli/client/mock.go (1)
114-116: LGTM: Method rename improves API clarity.Renaming from
GetController()toGetFabricClient()better describes what the method returns and aligns with interface changes elsewhere in the codebase.src/pkg/agent/tools/interfaces.go (1)
24-24: LGTM: Simplified authentication interface.Removing the
fabric *client.GrpcClientparameter simplifies the authentication flow. Callers now reconnect after successful login to obtain a fresh client with the new token, as seen in deploy.go.src/cmd/cli/command/globals.go (2)
82-82: LGTM: Type-safe tenant handling.The change from
stringtotypes.TenantNameOrIDprovides better type safety and enables Cobra flag integration via theType()andSet()methods defined on the type.
204-211: LGTM: Environment variable handling correctly updated.The updated logic properly maintains backward compatibility with the deprecated
DEFANG_ORGenvironment variable while prioritizingDEFANG_WORKSPACE.src/pkg/types/tenant.go (2)
3-4: LGTM: New TenantName type for DNS labels.The introduction of
TenantNameas a distinct type fromTenantNameOrIDprovides clearer semantics for components that specifically work with tenant DNS labels rather than arbitrary identifiers.
30-39: LGTM: Cobra flag integration correctly implemented.The
Type()andSet()methods properly implement thepflag.Valueinterface, enablingTenantNameOrIDto be used directly with Cobra flag parsing. TheSet()method performs no validation, which appears intentional since validation likely occurs at usage sites.src/pkg/agent/tools/deploy.go (1)
36-50: LGTM: Post-login reconnection correctly implemented.The updated flow properly handles authentication:
- Attempt initial connection
- On failure, perform interactive login (line 36)
- Reconnect with the fresh token (lines 45-49)
- Propagate any reconnection errors
This pattern correctly adapts to the simplified authentication interface where
InteractiveLoginMCPno longer receives or modifies a client instance.src/pkg/cli/destroy_test.go (1)
10-10: LGTM!The test correctly adapts to the new
Connectsignature by adding thetypesimport and passingtypes.TenantUnsetas the tenant parameter. This aligns with the refactored API where tenant handling is now explicit.Also applies to: 57-57
src/pkg/agent/tools/deploy_test.go (1)
59-62: LGTM!The mock signature correctly aligns with the refactored
InteractiveLoginMCPinterface that removes thefabricparameter. This matches the actual implementation insrc/pkg/login/login.gowhich now handles fabric connection internally.src/pkg/cli/client/client.go (2)
31-31: Return type change forGetTenantName:TenantNameOrID→TenantNameThis is a meaningful semantic change—
TenantNamerepresents a resolved tenant name (output), whileTenantNameOrIDis for input that may be unset. This improves type safety at the interface boundary.
27-27: Interface method rename:GetController→GetFabricClientThe rename improves clarity. All implementations of the
FabricClientinterface have been updated:GrpcClientin grpc.go andMockFabricClientin mock.go both implement the newGetFabricClient()method with no remaining references to the old method name.src/pkg/agent/tools/default_tool_cli.go (2)
48-51: Connect method updated to use tenant-aware connectionThe change to
ConnectWithTenantwithtypes.TenantUnsetis appropriate, and the TODO comment documents the known limitation for MCP workspace support.
116-117: SimplifiedInteractiveLoginMCPdelegationThe method now correctly delegates to
login.InteractiveLoginMCPwhich handles the fabric connection internally. This aligns with the broader refactor removing the fabric threading through function signatures.src/pkg/cli/configList_test.go (1)
11-11: LGTM!The test correctly adapts to the new
Connectsignature, consistent with other test updates in this PR.Also applies to: 51-51
src/pkg/cli/deploymentsList_test.go (1)
11-11: LGTM!The test correctly adapts to the new
Connectsignature with the explicit tenant parameter, consistent with the tenant refactoring across the codebase.Also applies to: 61-61
src/pkg/cli/activeDeployments_test.go (1)
10-10: LGTM!The test correctly adapts to the updated
Connectsignature by importing thetypespackage and usingtypes.TenantUnsetfor the tenant parameter. This is appropriate for tests that don't require tenant-specific behavior.Also applies to: 51-51
src/pkg/cli/client/byoc/gcp/byoc.go (1)
105-111: LGTM!The constructor signature update to use
types.TenantNameis consistent with the other BYOC providers (AWS and DO) as shown in the relevant code snippets.src/pkg/auth/interceptor.go (2)
13-21: LGTM!The interceptor now properly uses the
types.TenantNameOrIDtype with itsIsSet()method for conditional header setting. This provides type safety and clearer intent compared to raw string comparison.
27-29: Consistent tenant header handling in both unary and streaming paths.Both
WrapUnaryandWrapStreamingClientuse the same pattern for conditionally setting the tenant header, ensuring consistent behavior across request types.Also applies to: 39-41
src/cmd/cli/command/commands_test.go (2)
86-88: LGTM!The
init()function correctly adapts to the updatedSetupCommandssignature that no longer requires a context parameter.
267-268: LGTM!The test correctly uses the renamed
SetFabricClientmethod, which provides a more explicit API for setting the mock fabric controller client.src/pkg/cli/client/byoc/aws/byoc.go (2)
126-133: LGTM!The constructor signature update to use
types.TenantNameis consistent with the other BYOC providers. The initialization pattern remains clean and correct.
480-480: Good documentation of backward compatibility constraint.The inline comment explaining that
DEFANG_ORGis kept for CD backward compatibility is helpful. The explicitstring(b.TenantName)conversion is required sinceTenantNameis a defined type.src/pkg/cli/client/byoc/do/byoc.go (3)
31-31: Import addition is appropriate.The
logspackage import supports thelogs.LogTypeusage in the file (e.g., line 425).
65-79: LGTM!The constructor signature update to use
types.TenantNameis consistent with the AWS and GCP BYOC providers.
659-659: Consistent with AWS provider.The
DEFANG_ORGenvironment variable correctly usesstring(b.TenantName), matching the AWS provider's approach.src/cmd/cli/command/globals_test.go (2)
14-14: LGTM! Type-safe tenant handling in tests.The test updates correctly reflect the migration from
stringtotypes.TenantNameOrIDfor theTenantfield. The use offlags.Var()is appropriate sinceTenantNameOrIDimplements thepflag.Valueinterface (withSet,String, andTypemethods as shown in the relevant snippets).Also applies to: 255-255, 374-374, 395-395, 444-444
406-412: Verify intended deprecation ofDEFANG_TENANTenvironment variable.The test case "tenant env ignored" shows that
DEFANG_TENANTalone now returns an empty tenant, effectively deprecating this env var. Ensure this breaking change is documented and users are informed to migrate toDEFANG_WORKSPACE.src/pkg/cli/client/playground.go (2)
25-96: LGTM! Consistent migration to FabricClient interface.All method implementations correctly migrate from the old controller interface to
GetFabricClient(), maintaining the same request wrapping pattern withconnect.NewRequest(req). The changes are uniform and preserve the original behavior.
140-146: LGTM!The explicit
string()conversion forTenantNameis correct and cleaner than a method call.src/pkg/login/agree_tos.go (1)
18-65: LGTM! Clean parameter rename for clarity.The parameter rename from
ctofabricimproves code readability by making the interface type explicit. All internal call sites are correctly updated, and the control flow remains unchanged.src/pkg/cli/client/byoc/baseclient.go (1)
55-72: LGTM! Stronger typing for tenant handling.The migration from
stringtotypes.TenantNamefor theTenantNamefield provides better type safety. The constructor correctly acceptstypes.TenantNamedirectly, requiring callers to resolveTenantNameOrIDtoTenantNamebefore instantiation.src/pkg/cli/connect_test.go (2)
20-168: LGTM! Comprehensive test coverage for tenant-aware connections.The tests thoroughly verify the new
ConnectWithTenantflow including:
- Network error scenarios with
types.TenantUnset- Proper distinction between
GetRequestedTenant()(client-requested) andGetTenantName()(server-returned)- Explicit tenant header propagation
- Legacy cluster prefix handling
The test structure clearly validates both the request and response sides of tenant handling.
171-181: LGTM!The mock correctly captures the tenant header from incoming requests for test verification and returns configurable tenant responses.
src/pkg/cli/connect.go (3)
14-21: LGTM! Simplified Connect function.The new
Connectfunction cleanly builds aGrpcClientwith the requested tenant. The removal of the error return simplifies the API since client construction itself doesn't fail—validation happens later inConnectWithTenant.
23-34: LGTM! Tenant validation via WhoAmI.The function correctly validates the tenant by calling
WhoAmIand updatesgrpcClient.Tenantwith the server-validated tenant name on success. Returning the client even on error allows callers to use it for limited operations (e.g., login).
36-50: LGTM!The
NewProviderfunction correctly usesfabricClient.GetTenantName()to pass the validated tenant to BYOC providers.src/cmd/cli/command/commands.go (7)
145-145: LGTM! Simplified SetupCommands signature.Removing the
ctxparameter makes sense since command setup doesn't require a context—execution-time context is available viacmd.Context().
171-171: LGTM!The
--workspaceflag correctly binds toglobal.Tenantof typeTenantNameOrID.
403-404: Temporary tracker for pre-auth tracking.Good pattern: creating a temporary tracker using
cli.Connectbefore authentication allows tracking of the login flow itself. The tracker is updated at line 453 after successful connection.
445-453: LGTM! Connection and tracker flow.The flow correctly establishes the authenticated connection via
ConnectWithTenant, allows the login flow to continue onCodeUnauthenticated, and updates the global tracker with the real client after successful authentication.
509-509: LGTM!The simplified
InteractiveLogincall aligns with the updated signature inlogin.go.
554-554: LGTM!Direct use of
global.Tenantis cleaner than the removedgetTenantSelection()helper.
1154-1154: LGTM!Consistent use of
global.Tenantfor the token command.src/pkg/login/login.go (4)
22-24: LGTM! Simplified AuthService interface.Removing the
FabricClientparameter fromloginis appropriate since token acquisition via OAuth doesn't require an existing client connection.
28-39: LGTM!The implementation correctly performs the OAuth code flow and exchanges the code for a token without needing an existing fabric client.
61-63: LGTM! Dry-run guard.The dry-run check is correctly placed after token acquisition but before any file system operations, ensuring dry runs don't modify state.
127-137: LGTM! Re-authentication flow with tenant preservation.The flow correctly:
- Calls the simplified
InteractiveLogin- Reconnects using
GetRequestedTenant()to preserve the tenant context- Updates the global tracker with the new fabric client
src/pkg/cli/client/grpc.go (3)
19-25: LGTM! Clean tenant field separation.Good design:
requestedTenant(what was requested by the user) is distinct fromTenant(validated by server). TherequestedTenantis private whileTenantis public, allowing external code to read the validated tenant.
27-50: LGTM! Constructor initializes requestedTenant only.The constructor correctly:
- Passes
requestedTenantto the auth interceptor for header inclusion- Stores
requestedTenantin the client- Leaves
Tenantunset—it's populated later byConnectWithTenantafter server validation
59-73: LGTM! Clean accessor methods.The accessor methods provide clear semantics:
GetRequestedTenant(): returns the originally requested tenant (name or ID)GetTenantName(): returns the server-validated tenant nameGetFabricClient()/SetFabricClient(): for internal client access
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/connect.gosrc/pkg/types/tenant.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/cli/client/byoc/aws/byoc_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
src/pkg/cli/client/client.go (2)
src/protos/io/defang/v1/defangv1connect/fabric.connect.go (1)
FabricControllerClient(163-217)src/pkg/types/tenant.go (1)
TenantLabel(4-4)
src/pkg/cli/client/byoc/aws/byoc.go (3)
src/pkg/cli/client/byoc/do/byoc.go (1)
NewByocProvider(65-79)src/pkg/cli/client/byoc/gcp/byoc.go (1)
NewByocProvider(105-111)src/pkg/types/tenant.go (1)
TenantLabel(4-4)
src/pkg/cli/client/byoc/gcp/byoc.go (3)
src/pkg/cli/client/byoc/aws/byoc.go (1)
NewByocProvider(126-133)src/pkg/cli/client/byoc/do/byoc.go (1)
NewByocProvider(65-79)src/pkg/types/tenant.go (1)
TenantLabel(4-4)
src/pkg/cli/client/byoc/baseclient.go (3)
src/pkg/types/tenant.go (1)
TenantLabel(4-4)src/pkg/utils.go (1)
Getenv(48-53)src/pkg/dns/utils.go (1)
SafeLabel(5-7)
src/pkg/cli/client/byoc/do/byoc.go (3)
src/pkg/cli/client/byoc/aws/byoc.go (1)
NewByocProvider(126-133)src/pkg/cli/client/byoc/gcp/byoc.go (1)
NewByocProvider(105-111)src/pkg/types/tenant.go (1)
TenantLabel(4-4)
src/pkg/cli/client/grpc.go (2)
src/pkg/types/tenant.go (3)
TenantNameOrID(8-8)TenantNameOrID(31-33)TenantLabel(4-4)src/pkg/auth/interceptor.go (1)
NewAuthInterceptor(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (15)
src/pkg/types/tenant.go (2)
3-4: LGTM! Clear type definition for tenant DNS labels.The new
TenantLabeltype provides type safety and clearly indicates its purpose as a DNS label representation.
30-39: LGTM! Standard Cobra flag implementation.The
Type()andSet()methods correctly implement thepflag.Valueinterface for Cobra flag parsing. TheSet()method accepts any string without validation, which provides flexibility for accepting various tenant identifier formats.src/pkg/cli/client/grpc.go (3)
27-49: LGTM! Constructor correctly initializes with requested tenant.The refactored constructor cleanly accepts and stores the
requestedTenant, passing it to the authentication interceptor. This aligns with the new tenant resolution flow where the requested tenant is sent to the server.
59-73: LGTM! Method renames improve API clarity.The renamed methods (
GetFabricClient,SetFabricClient) are more descriptive, and the newGetRequestedTenant()method provides appropriate access to the user-supplied tenant identifier. The distinction between requested tenant (input) and resolved tenant (output from WhoAmI) is clear.
23-24: TheTenantfield is uninitialized in the constructor and only populated after a successfulWhoAmIcall inConnectWithTenant. This is intentional—the codebase has an explicit test validating thatGetTenantName()returns an empty value whenConnect()is used withoutConnectWithTenant(). All production usages ofGetTenantName()occur afterConnectWithTenantsucceeds or have theTenantfield explicitly set, so the empty state is handled correctly by existing code.src/pkg/cli/client/client.go (1)
27-31: LGTM! Interface updates align with implementation.The method renames and return type changes are consistent with the implementation in
grpc.go. Based on learnings, theTenantLabelname was chosen to clarify that this represents the tenant's DNS label, addressing previous confusion about "TenantName" usage.src/pkg/cli/client/byoc/aws/byoc.go (2)
126-133: LGTM! Constructor updated to accept TenantLabel.The constructor signature correctly uses
types.TenantLabel. Minor note: the parameter nametenantNamemight be slightly misleading since it's now aTenantLabel, but this is consistent across all BYOC providers and doesn't cause functional issues.
480-480: LGTM! Environment variable correctly uses TenantLabel.The conversion from
TenantLabelto string is explicit, and the comment clearly explains the backward compatibility reason for keeping theDEFANG_ORGvariable name. This is consistent with the refactoring goals.src/pkg/cli/client/byoc/gcp/byoc.go (1)
105-111: LGTM! Constructor signature consistent with other BYOC providers.The constructor correctly accepts
types.TenantLabeland passes it to the base client, matching the pattern used in AWS and DO providers.src/pkg/cli/client/byoc/do/byoc.go (2)
65-79: LGTM! Constructor consistent with BYOC provider pattern.The constructor correctly accepts
types.TenantLabeland passes it to the base client, following the same pattern as the AWS provider.
659-659: LGTM! Environment variable correctly uses TenantLabel.The
DEFANG_ORGenvironment variable correctly usesstring(b.TenantLabel), consistent with the AWS provider implementation.src/pkg/cli/client/byoc/baseclient.go (2)
55-72: LGTM! Base client correctly updated to use TenantLabel.The field rename from
TenantNametoTenantLabeland the type change totypes.TenantLabelare consistent with the overall refactoring. The constructor properly accepts and assigns the new type.
301-306: LGTM! New ServicePublicDNS method constructs correct DNS names.The method correctly builds a public DNS name using
SafeLabelfor DNS compatibility. The special handling for the "beta" stack maintains backward compatibility, and the use ofTenantLabelaligns with the refactoring goals.src/pkg/cli/connect.go (2)
14-21: LGTM! Simplified Connect function with explicit tenant parameter.The refactored
Connectfunction removes implicit tenant derivation from tokens and requires callers to explicitly provide therequestedTenant. This makes the tenant source clear and simplifies the connection logic. The debug logging aids troubleshooting.
23-34: Verify error handling when WhoAmI fails.The function returns the client even when
WhoAmIfails (intentional graceful degradation). However,grpcClient.Tenantremains empty in error cases. While callers suppress onlyCodeUnauthenticatederrors and immediately return other errors, and the happy path (GetVersions) doesn't require Tenant, downstream code lacks explicit guards before usingGetTenantName(). Consider adding a code comment explaining why this design is safe, or add validation in callers that use the client after error suppression.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Description
Instead of #1729. Resolving several places where the "name or ID" was used for the subdomain, instead of the "tenant name" from Fabric.
Linked Issues
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.