Add Lease related methods to client service #115
Conversation
WalkthroughThis pull request introduces several enhancements across different parts of the system. A new GitHub Actions job ( Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (5)
✨ 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 (
|
NickCao
force-pushed
the
client-svc-lease
branch
from
3676888 to
ab7bd1d
Compare
![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
🔭 Outside diff range comments (2)
internal/service/controller_service.go (2)
653-676:⚠️ Potential issueEnforce namespace checks for security.
Currently, this method allows any authenticated client to retrieve any Exporter simply by passing in an identifier (including the Exporter’s namespace). Consider verifying that the requested Exporter’s namespace actually matches the requesting client’s namespace (or otherwise enforce authorization explicitly).A minimal fix might involve fetching the client object (e.g.,
jclient, err := s.controller.authenticateClient(ctx)) and checking:func (s *clientService) GetExporter( ctx context.Context, req *cpb.GetExporterRequest, ) (*cpb.Exporter, error) { - _, err := s.controller.authenticateClient(ctx) + jclient, err := s.controller.authenticateClient(ctx) if err != nil { return nil, err } namespace, name, err := ParseExporterIdentifier(req.Name) if err != nil { return nil, err } + // Ensure the Exporter is in the same namespace as the client + if namespace != jclient.Namespace { + return nil, fmt.Errorf("GetExporter permission denied") + } var exporter jumpstarterdevv1alpha1.Exporter ...
678-719:⚠️ Potential issueRestrict listing scope to the client's namespace.
As written, this method can list exporters in any namespace. If that’s intentional, ensure robust authorization measures. Otherwise, validate thatnamespace == jclient.Namespaceto prevent cross-namespace listing.func (s *clientService) ListExporters( ctx context.Context, req *cpb.ListExportersRequest, ) (*cpb.ListExportersResponse, error) { - _, err := s.controller.authenticateClient(ctx) + jclient, err := s.controller.authenticateClient(ctx) if err != nil { return nil, err } namespace, err := ParseNamespaceIdentifier(req.Parent) if err != nil { return nil, err } + if namespace != jclient.Namespace { + return nil, fmt.Errorf("ListExporters permission denied") + } selector, err := labels.Parse(req.Filter) ...
🧹 Nitpick comments (6)
.github/workflows/e2e.yaml (1)
10-15: Pin the action version for better stability.Using
@mainmight introduce unexpected breakage if the referenced action changes upstream. Consider pinning to a specific version or commit SHA.Example diff:
- - uses: jumpstarter-dev/jumpstarter-e2e@main + - uses: jumpstarter-dev/jumpstarter-e2e@v1.0.0internal/service/helpers.go (1)
31-31: Consider adding a more descriptive doc comment.Adding a short comment explaining parameter expectations (e.g.,
'kind' must be either exporters, leases, etc.) would aid readability and clarity.+// ParseIdentifier parses an identifier of the form "namespaces/<ns>/<kind>/<name>". // Returns namespace and name if valid, or an error otherwise. func ParseIdentifier(identifier string, kind string) (string, string, error) {api/v1alpha1/lease_helpers.go (2)
21-53: Consider validating duration and operator constraints.If a zero or nil
req.Durationis unexpected, consider validating or defaulting. Similarly, guard against unknown or invalidexp.Operatorvalues to prevent downstream issues.if req.Duration == nil { + // Optionally return an error or enforce a default duration: + // return nil // or handle a default } for _, exp := range req.Selector.MatchExpressions { + // Optionally verify that exp.Operator is in an allowed set: + // allowedOps := map[string]bool{"In": true, "NotIn": true, ...} + // if !allowedOps[exp.Operator] { + // return nil // or handle an error + // } }
55-106: Follow up on the remaining TODOs.The TODO comments for lease renewal and scheduled leases might warrant dedicated tracking to ensure they get implemented. Let me know if you need additional help or if I should open a new issue to highlight these tasks.
internal/service/controller_service.go (2)
73-76: Struct design looks good, but be mindful of potential circular references.
Embeddingcpb.UnimplementedClientServiceServeris correct for implementing the client service. However, referencingControllerServicefromclientServiceand vice versa can, in some cases, lead to circular dependencies or confusion. Ensure it’s well-documented to avoid maintainability issues.
847-879: Clarify “Delete” vs. “Release” semantics.
This method setslease.Spec.Release = trueinstead of deleting the resource. That’s fine if it’s intentionally a “finalize and release” operation, but the name may confuse users who anticipate a hard deletion. Consider renaming or adding doc comments to clarify behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/client.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/client_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
.github/workflows/e2e.yaml(1 hunks)api/v1alpha1/lease_helpers.go(1 hunks)internal/service/controller_service.go(8 hunks)internal/service/helpers.go(1 hunks)
🔇 Additional comments (10)
internal/service/helpers.go (3)
24-25: Clean consolidation of parsing logic.Delegating the parsing to
ParseIdentifiermakes the code more DRY and maintainable.
27-28: Consistent use ofParseIdentifierfor leases.This approach ensures uniform validation across different identifier types.
39-40: Logic and error message align well.This validation step ensures the correct
kindsegment. No issues identified.api/v1alpha1/lease_helpers.go (2)
8-11: New protobuf imports look correct.These imports properly support the introduced Protobuf conversion logic.
16-17: Pointer utilities correctly imported.Using
k8s.io/utils/ptrfor pointer convenience is a neat approach.internal/service/controller_service.go (5)
33-33: Good introduction of thecpbimport.
This import provides the Protobuf definitions for the new client-oriented gRPC service.
42-42: Use ofemptypbis appropriate.
Importingemptypbfromgoogle.golang.org/protobuf/types/known/emptypbis standard practice for methods returning empty payloads in gRPC.
777-807: Creation flow looks correct.
The method enforces namespace ownership and sets the client reference properly. The use ofuuid.NewV7()for lease naming is a secure and collision-resistant approach.
809-845: Updating the lease is handled well.
The permission checks and patching logic ensure the client can only update a lease it owns. This is consistent with the rest of the design.
916-916: Registering the new client service is correct.
Leveraging a separate service struct for client methods helps keep the system modular and avoids pollutingControllerServicewith client-specific gRPC endpoints.
NickCao
force-pushed
the
client-svc-lease
branch
from
f35e455 to
5a725ab
Compare
NickCao
force-pushed
the
client-svc-lease
branch
from
5a725ab to
741b13e
Compare
NickCao
force-pushed
the
client-svc-lease
branch
from
741b13e to
fd74fc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/service/utils/identifier.go (1)
36-71: Valid object identifier parsing
Checking for the correct number of segments, validating "namespaces" as the first segment, and matching the resource kind are all handled cleanly. You might consider refactoring the literal"namespaces"into a constant to avoid typos and centralize the logic, for instance:+const resourceNamespaceSegment = "namespaces" func ParseObjectIdentifier(identifier string, kind string) (key *kclient.ObjectKey, err error) { segments := strings.Split(identifier, "/") - if segments[0] != "namespaces" { + if segments[0] != resourceNamespaceSegment { return nil, status.Errorf( codes.InvalidArgument, ...internal/service/auth/auth.go (1)
37-55: Client authentication
Using the OIDCVerifyClientObjectTokenfollowed by a namespace check is logically sound. Consider whether returningcodes.Unauthenticated(instead ofPermissionDenied) might be appropriate if the token is invalid.api/v1alpha1/lease_helpers.go (2)
23-44: Consider validating the lease duration.Currently,
LeaseFromProtobufsimply converts and storesreq.Duration. If you anticipate invalid (e.g. negative or zero) durations, you could validate them here to avoid unexpected runtime behaviors.if err != nil { return nil, err } +// Optionally ensure the duration is positive +if req.Duration.AsDuration() <= 0 { + return nil, fmt.Errorf("lease duration must be positive") +} return &Lease{
46-88: Acknowledge theTODOfor extended lease management.The
EffectiveDurationcurrently defaults to the same value asDuration. Make sure to address your// TODO: implement lease renewalto adjustEffectiveDurationif lease-renewal functionality is required.Do you want help drafting the lease renewal logic?
internal/service/controller_service.go (1)
683-686: Registration of the new ClientService.Replacing the direct client-service logic with
clientsvcv1.NewClientServicemakes the architecture more modular. Just confirm that theauth.NewAuth(...)call handles concurrency or ephemeral contexts properly—generally it appears fine.cpb.RegisterClientServiceServer( server, - clientsvcv1.NewClientService(s.Client, *auth.NewAuth(s.Client, s.Authn, s.Authz, s.Attr)), + clientsvcv1.NewClientService( + s.Client, + *auth.NewAuth(s.Client, s.Authn, s.Authz, s.Attr), + ), )internal/service/client/v1/client_service.go (1)
192-229: UpdateLease flow.Correctly enforces an ownership check (
permission deniedif not the same client). Only updates the lease’sSpec.Durationfrom the new request. Consider whether additional fields should also be updatable.// If more lease fields need updating beyond duration, handle them here: jlease.Spec.Duration = desired.Spec.Duration +// e.g. jlease.Spec.Selector = desired.Spec.Selector?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/client/v1/client_grpc.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/client.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/client_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
.github/workflows/e2e.yaml(1 hunks)api/v1alpha1/exporter_helpers.go(2 hunks)api/v1alpha1/lease_helpers.go(1 hunks)internal/service/auth/auth.go(1 hunks)internal/service/client/v1/client_service.go(1 hunks)internal/service/controller_service.go(2 hunks)internal/service/helpers.go(0 hunks)internal/service/utils/identifier.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/service/helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e.yaml
🧰 Additional context used
🧠 Learnings (1)
internal/service/controller_service.go (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#115
File: internal/service/controller_service.go:740-775
Timestamp: 2025-03-13T14:39:31.845Z
Learning: Clients are allowed to see each other's leases in the Jumpstarter controller. The ListLeases function intentionally does not filter by client ownership.
🔇 Additional comments (21)
internal/service/utils/identifier.go (4)
12-34: Well-structured error handling for namespace identifier
The function correctly enforces a two-segment rule with "namespaces" as the first. This yields clear, precise gRPC errors when inputs are invalid.
73-75: Consistent string formatting
Building the identifier string mirrors the parsing logic, ensuring consistency between parse/unparse.
77-83: Exporter identifier convenience methods
Delegating to the generic Parse/UnparseObjectIdentifier with"exporters"fosters clean, DRY code.
85-91: Lease identifier convenience methods
Similarly, these methods neatly wrap the parse/unparse logic for"leases", maintaining consistency with naming conventions.api/v1alpha1/exporter_helpers.go (2)
25-30: Protobuf exporter conversion
Usingutils.UnparseExporterIdentifierensures a consistentNameformat in the Protobuf output, and labels are passed through directly.
32-41: ExporterList to Protobuf
This method elegantly maps each item to Protobuf viaToProtobuf(), and includesNextPageTokento support paging.internal/service/auth/auth.go (3)
16-21: Auth struct fields
The combination of client, authenticator, authorizer, and context attributes covers the dependencies needed for token checks and permission evaluation.
23-35: Auth constructor
Clear dependency injection ensures future testability and maintainability, bundling all required components in a unified struct.
57-75: Exporter authentication
This mirrorsAuthClient, providing consistent OIDC verification and namespace validation for exporters.api/v1alpha1/lease_helpers.go (2)
8-20: Imports look consistent with new Protobuf helpers.All newly introduced imports cleanly integrate with the existing module references. No concerns about unused or conflicting imports.
90-99: Conversion from LeaseList to a gRPC response is solid.This maintains pagination support via
NextPageToken. Looks clean and straightforward.internal/service/controller_service.go (1)
33-36: Imports for expanded client service integration.The newly added imports for
cpb,auth, andclientsvcv1are in line with the restructuring to delegate lease management responsibilities to the new client service.internal/service/client/v1/client_service.go (9)
1-15: License and package declaration.All licensing headers and package statements are in place. No issues here.
19-34: Imports and global references.The import set is well-organized. The usage of
controller.MatchingActiveLeasesandutilsfor identifier parsing is aligned with code conventions found elsewhere in this repository.
36-47: NewClientService constructor is straightforward.Dependency injection for both
ClientandAuthis clear. EnsureAuthClientchecks are robust for multi-tenant scenarios.
49-69: GetExporter method.Retrieves exporter by namespace and name with no ownership checks. This is consistent with the accepted policy that “Clients can see each other’s resources.” The usage of
AuthClientis correct for verifying at least the namespace access.
71-101: ListExporters method.Applying parent-namespace verification and label filtering aligns with existing patterns. Page size and token support is well-handled.
103-120: GetLease: read-only lease retrieval for any client.Consistent with the policy that clients can view each other’s leases. If future requirements change, consider adding explicit ownership checks.
122-157: ListLeases method.Retrieves all leases matching the label filter in the given namespace, with no explicit client ownership filter. This matches the “clients may see each other’s leases” guideline.
159-190: CreateLease flow.Generates a unique name via
uuid.NewV7()and relies onLeaseFromProtobuf. It setsSpec.ClientRefto the authenticated client. Logic looks correct.
231-260: DeleteLease with a “release” patch.Enforces ownership checks, then sets
Spec.Release = true. This approach is consistent across the codebase.
NickCao
force-pushed
the
client-svc-lease
branch
from
fd74fc9 to
614246b
Compare
See jumpstarter-dev/jumpstarter-protocol#17
Summary by CodeRabbit