OSAC-3053: implement fulfillment gRPC client stub - #94
Conversation
|
@rgolangh: This pull request references OSAC-3053 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe controller configuration now defines fulfillment endpoint, credentials, and TLS settings. Helm conditionally mounts the credential Secret and passes connection arguments. The CSI driver establishes an authenticated TLS gRPC connection and retains in-memory stubs when no endpoint is configured. ChangesFulfillment connection configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant ControllerDeployment
participant CSIDriver
participant FulfillmentEndpoint
Helm->>ControllerDeployment: Render endpoint and credential settings
ControllerDeployment->>CSIDriver: Pass fulfillment arguments and mount token file
CSIDriver->>FulfillmentEndpoint: Establish authenticated TLS gRPC connection
FulfillmentEndpoint-->>CSIDriver: Return connection or connection error
Possibly related PRs
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
177f9ca to
feb5645
Compare
90f59e0 to
8a1ffbd
Compare
There was a problem hiding this comment.
@rgolangh Now that the operator and fulfillment-service are in the same repo, should buf.gen.yaml generate from local proto directories (../fulfillment-service/proto/private) instead of pulling from BSR?
That would remove the version pinning and let proto changes flow through without a publish-then-pull cycle.
Not blocking, just wondering if that's planned as a follow-up.
There was a problem hiding this comment.
active discussion, see here: https://redhat.atlassian.net/browse/OSAC-1735
There was a problem hiding this comment.
I'll wait for this ticket to resolve, meanwhile this PR doesn't need a full blown client. This was a claude mistake
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Overall, changes LGTM.
Thanks for addressing my comments from the previous PR.
Please see my comment here before merging. Not sure if this something you'd like to tackle as part of this PR. It's a non-blocking 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@osac-csi-driver/charts/csi-driver/templates/controller-deployment.yaml`:
- Around line 29-38: Update the .Values.controller.fulfillment rendering block
so that when endpoint is set, both credentials.secretName and credentials.key
are validated as required and Helm rendering fails if either is empty; only
render --fulfillment-token-file after validation, while preserving the existing
endpoint and TLS flag behavior.
In `@osac-operator/pkg/fulfillment/conn.go`:
- Around line 48-50: Update the connection error returned by grpc.NewClient in
the fulfillment connection setup to omit the endpoint value and use a generic
descriptive message while preserving the wrapped underlying error. Keep the
existing successful connection flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9d34545-90c0-4652-8627-54c2ba23fb9c
⛔ Files ignored due to path filters (1)
osac-csi-driver/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
osac-csi-driver/Containerfileosac-csi-driver/charts/csi-driver/templates/controller-deployment.yamlosac-csi-driver/charts/csi-driver/values.yamlosac-csi-driver/cmd/osac-csi-driver/main.goosac-csi-driver/go.modosac-csi-driver/pkg/fulfillment/client.goosac-operator/cmd/main.goosac-operator/pkg/fulfillment/conn.go
🚧 Files skipped from review as they are similar to previous changes (5)
- osac-csi-driver/cmd/osac-csi-driver/main.go
- osac-csi-driver/go.mod
- osac-csi-driver/pkg/fulfillment/client.go
- osac-csi-driver/charts/csi-driver/values.yaml
- osac-csi-driver/Containerfile
| {{- with .Values.controller.fulfillment }} | ||
| {{- if .endpoint }} | ||
| - "--fulfillment-endpoint={{ .endpoint }}" | ||
| {{- if .credentials.secretName }} | ||
| - "--fulfillment-token-file=/etc/osac-csi/credentials/{{ .credentials.key }}" | ||
| {{- end }} | ||
| {{- if .tls.insecureSkipVerify }} | ||
| - "--grpc-insecure" | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail the Helm render when the endpoint has no credentials.
The chart documents credentials.secretName as required when endpoint is set. This block still renders --fulfillment-endpoint without --fulfillment-token-file when the Secret name is empty. main.go then passes an empty token file, and osac-operator/pkg/fulfillment/conn.go omits bearer credentials. The deployment can start with a configured but unauthenticated client.
Validate credentials.secretName and credentials.key when endpoint is set. If unauthenticated endpoints are supported, update the chart contract instead.
Proposed validation
{{- if .endpoint }}
+ {{- if not .credentials.secretName }}
+ {{- fail "controller.fulfillment.credentials.secretName is required when endpoint is set" }}
+ {{- end }}
+ {{- if not .credentials.key }}
+ {{- fail "controller.fulfillment.credentials.key is required when endpoint is set" }}
+ {{- end }}
- "--fulfillment-endpoint={{ .endpoint }}"
- {{- if .credentials.secretName }}
- "--fulfillment-token-file=/etc/osac-csi/credentials/{{ .credentials.key }}"
- {{- end }}
{{- if .tls.insecureSkipVerify }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- with .Values.controller.fulfillment }} | |
| {{- if .endpoint }} | |
| - "--fulfillment-endpoint={{ .endpoint }}" | |
| {{- if .credentials.secretName }} | |
| - "--fulfillment-token-file=/etc/osac-csi/credentials/{{ .credentials.key }}" | |
| {{- end }} | |
| {{- if .tls.insecureSkipVerify }} | |
| - "--grpc-insecure" | |
| {{- end }} | |
| {{- end }} | |
| {{- with .Values.controller.fulfillment }} | |
| {{- if .endpoint }} | |
| {{- if not .credentials.secretName }} | |
| {{- fail "controller.fulfillment.credentials.secretName is required when endpoint is set" }} | |
| {{- end }} | |
| {{- if not .credentials.key }} | |
| {{- fail "controller.fulfillment.credentials.key is required when endpoint is set" }} | |
| {{- end }} | |
| - "--fulfillment-endpoint={{ .endpoint }}" | |
| - "--fulfillment-token-file=/etc/osac-csi/credentials/{{ .credentials.key }}" | |
| {{- if .tls.insecureSkipVerify }} | |
| - "--grpc-insecure" | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-csi-driver/charts/csi-driver/templates/controller-deployment.yaml`
around lines 29 - 38, Update the .Values.controller.fulfillment rendering block
so that when endpoint is set, both credentials.secretName and credentials.key
are validated as required and Helm rendering fails if either is empty; only
render --fulfillment-token-file after validation, while preserving the existing
endpoint and TLS flag behavior.
| conn, err := grpc.NewClient(endpoint, dialOpts...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("connecting to fulfillment-service at %s: %w", endpoint, err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not include the endpoint in the returned error.
osac-operator/cmd/main.go logs this error. The configured endpoint can contain an internal hostname. Return a generic connection error instead.
Proposed fix
- return nil, fmt.Errorf("connecting to fulfillment-service at %s: %w", endpoint, err)
+ return nil, fmt.Errorf("connecting to fulfillment-service: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn, err := grpc.NewClient(endpoint, dialOpts...) | |
| if err != nil { | |
| return nil, fmt.Errorf("connecting to fulfillment-service at %s: %w", endpoint, err) | |
| conn, err := grpc.NewClient(endpoint, dialOpts...) | |
| if err != nil { | |
| return nil, fmt.Errorf("connecting to fulfillment-service: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-operator/pkg/fulfillment/conn.go` around lines 48 - 50, Update the
connection error returned by grpc.NewClient in the fulfillment connection setup
to omit the endpoint value and use a generic descriptive message while
preserving the wrapped underlying error. Keep the existing successful connection
flow unchanged.
Source: Coding guidelines
248c77d to
9d535e3
Compare
| RUN cd osac-csi-driver && go mod download | ||
|
|
||
| COPY --chown=1001:1001 osac-csi-driver/ osac-csi-driver/ | ||
| COPY --chown=1001:1001 osac-operator/pkg/ osac-operator/pkg/ |
There was a problem hiding this comment.
Is this still needed? The CSI driver no longer imports from osac-operator after the shared conn.go was dropped.
| gopkg.in/yaml.v2 v2.4.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/osac-project/osac-operator => ../osac-operator |
There was a problem hiding this comment.
Same question: is this replace still needed? No code imports from osac-operator now.
There was a problem hiding this comment.
no, this is all left overs, and will be removed in the following push
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Overall changes LGTM.
Left a couple of nits. PTAL.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, rgolangh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
9d535e3 to
9b31d59
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@osac-csi-driver/cmd/osac-csi-driver/main.go`:
- Line 69: Update the fulfillment endpoint log in the startup flow to avoid
including the value of fulfillmentEndpoint. Keep a status message indicating
that fulfillment is configured and connected, but remove the endpoint
interpolation entirely.
- Line 107: Update the startup connection flow around grpc.NewClient to verify
connectivity before reporting successful initialization: when the endpoint is
non-empty, trigger or establish the ClientConn connection and wait with a
bounded context, propagating timeout or connection errors; only log successful
startup after this check completes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ff208fe-3b2e-4010-b264-823be35c3a4e
📒 Files selected for processing (4)
osac-csi-driver/charts/csi-driver/templates/controller-deployment.yamlosac-csi-driver/charts/csi-driver/values.yamlosac-csi-driver/cmd/osac-csi-driver/main.goosac-csi-driver/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- osac-csi-driver/charts/csi-driver/values.yaml
- osac-csi-driver/charts/csi-driver/templates/controller-deployment.yaml
|
/lgtm /approve |
|
/lgtm |
1 similar comment
|
/lgtm |
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
Wire the gRPC connection to the fulfillment-service in the CSI driver. When --fulfillment-endpoint is set, dialFulfillment() establishes a TLS connection (ALPN-disabled for OpenShift router compat) with optional file-based bearer token auth. The connection is established but stubs are still used for VolumeClient and ControlPlaneClient until the Volume API is implemented server-side (OSAC-2872). - Add --fulfillment-token-file and --grpc-insecure CLI flags - Add dialFulfillment() with TLS (MinVersion 1.2) and fileTokenSource - Update Helm chart to mount a credentials Secret at /etc/osac-csi/credentials/ (CSI driver runs on tenant clusters where the hub SA token is not valid — AAP provisions the Secret) - Gate --fulfillment-token-file on credentials.secretName Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:36 PM UTC · Completed 8:57 PM UTC Commit: |
|
/lgtm |
ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| volumeMounts: | ||
| - name: osac-socket-dir | ||
| mountPath: /csi/osac | ||
| {{- if and .Values.controller.fulfillment.endpoint .Values.controller.fulfillment.credentials.secretName }} |
There was a problem hiding this comment.
[medium] edge-case
The volumeMounts/volumes conditionals access .Values.controller.fulfillment.credentials.secretName without nil guards. If a user explicitly sets credentials: null in their values override, the template will fail with a nil pointer dereference.
Suggested fix: Add nil guards or document that the nested structure must not be nulled.
| conn, err := dialFulfillment(*fulfillmentEndpoint, *grpcInsecure, *fulfillmentTokenFile) | ||
| if err != nil { | ||
| klog.Fatalf("Failed to connect to fulfillment-service: %v", err) | ||
| } |
There was a problem hiding this comment.
[low] logic-error
Log says 'connected' but grpc.NewClient is lazy -- no TCP handshake occurs until the first RPC. Since stubs are used, the connection is never established.
Suggested fix: Change log to say 'configured' instead of 'connected'.
| } | ||
| } | ||
|
|
||
| func dialFulfillment(endpoint string, insecureSkipVerify bool, tokenFile string) (*grpc.ClientConn, error) { |
There was a problem hiding this comment.
[low] test-inadequate
dialFulfillment and fileTokenSource have no unit tests. fileTokenSource.Token() is straightforward to test.
Suggested fix: Add unit tests for fileTokenSource.Token() covering valid token, missing file, empty file, trailing newlines.
| @@ -72,6 +86,42 @@ func main() { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] authentication-bypass
When --fulfillment-endpoint is set but --fulfillment-token-file is not provided, dialFulfillment() establishes a connection with TLS but no authentication credentials, with no warning.
Suggested fix: Add a log warning when token file is empty but endpoint is set.
| # onto each tenant cluster during cluster setup. | ||
| fulfillment: | ||
| # gRPC endpoint of the fulfillment-service (e.g. "fulfillment-api.osac.svc:8000"). | ||
| # Leave empty to use in-memory stubs. |
There was a problem hiding this comment.
[low] tls-verification-bypass
No warning log emitted when --grpc-insecure is active. Default is safe (false) and the flag name is self-documenting.
Suggested fix: Add a warning log when --grpc-insecure is true.
| // and ControlPlaneClient once the Volume API is implemented. | ||
| conn, err := dialFulfillment(*fulfillmentEndpoint, *grpcInsecure, *fulfillmentTokenFile) | ||
| if err != nil { | ||
| klog.Fatalf("Failed to connect to fulfillment-service: %v", err) |
There was a problem hiding this comment.
[low] secrets-in-logs
Bearer token held in memory as plain string via fileTokenSource. If gRPC debug/trace logging is enabled, token metadata could be exposed in logs.
| klog.Infof("No fulfillment endpoint configured, using in-memory stubs") | ||
| volumeClient = fulfillment.NewVolumeStub("default-backend", "nfs") | ||
| controlPlaneClient = &fulfillment.ControlPlaneStub{} | ||
|
|
There was a problem hiding this comment.
[low] architecture-coherence
TLS setup uses experimentalcredentials.NewTLSWithALPNDisabled (matching osac-operator) but diverges from metering-service by not supporting custom CA certificates.
| @@ -72,6 +86,42 @@ func main() { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] architecture-coherence
fileTokenSource duplicates a pattern from fulfillment-service (with modification-time caching) and metering-service. Could eventually be extracted to a shared package.
| {{- end }} | ||
| {{- if .Values.controller.fulfillmentEndpoint }} | ||
| - "--fulfillment-endpoint={{ .Values.controller.fulfillmentEndpoint }}" | ||
| {{- with .Values.controller.fulfillment }} |
There was a problem hiding this comment.
[low] template-idiom-consistency
with .Values.controller.fulfillment scoping in args section is inconsistent with if .Values.X used elsewhere. Volume conditionals use full paths without with.
Suggested fix: Use either with or full paths consistently for fulfillment-related conditionals.
| } | ||
| } | ||
|
|
||
| func dialFulfillment(endpoint string, insecureSkipVerify bool, tokenFile string) (*grpc.ClientConn, error) { |
There was a problem hiding this comment.
[low] code-organization
dialFulfillment and fileTokenSource in main.go is consistent with existing parseVendorSockets, but fileTokenSource has reuse potential and could live in pkg/.
5 participants
Summary
Wire the gRPC connection to the fulfillment-service in the CSI driver. When
--fulfillment-endpointis set,dialFulfillment()establishes a TLS connection with optional file-based bearer token auth. The connection is established but stubs are still used forVolumeClientandControlPlaneClientuntil the Volume API is implemented server-side (OSAC-2872).Only
osac-csi-driver/files changed. No operator, fulfillment-service, or other component changes.What's added
--fulfillment-token-fileand--grpc-insecureCLI flagsdialFulfillment()with TLS (ALPN-disabled for OpenShift router compat,MinVersion: tls.VersionTLS12),fileTokenSourcefor rotated token authcontroller.fulfillment.endpoint,controller.fulfillment.credentials.secretName/.key,controller.fulfillment.tls.insecureSkipVerify/etc/osac-csi/credentials/— CSI driver runs on tenant clusters, AAP provisions the SecretTest plan
go build ./osac-csi-driver/...— buildshelm lint— passeshelm template— all value combinations render correctly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes