feat: add confidential compute support to SDL#312
Conversation
|
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:
WalkthroughAdds Trusted Execution Environment (TEE) support across schema, SDL parsing/validation, manifest generation, and provider attestation APIs; includes YAML fixtures, unit tests, protobuf contract additions, and build-tooling updates. ChangesTEE Support and Attestation Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
github-actions
Bot
added
C:go/manifest
C:proto/provider
C:go/provider
C:go/sdl
Type: Build
labels
![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
🧹 Nitpick comments (1)
go/provider/client/client.go (1)
924-926: ⚡ Quick winUse
ClientResponseErrorfor non-OK responses for API consistency.At Line 924, this method returns a plain formatted error, while other HTTP methods return typed
ClientResponseErrorvia shared handling. This makes downstream error handling inconsistent.Suggested diff
- if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("attestation quote failed (status %d): %s", resp.StatusCode, string(body)) - } + if err := createClientResponseErrorIfNotOK(resp, bytes.NewBuffer(body)); err != nil { + return nil, 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 `@go/provider/client/client.go` around lines 924 - 926, Replace the plain fmt.Errorf return in the non-OK branch with a typed ClientResponseError to match the package's API error handling: when resp.StatusCode != http.StatusOK build and return a ClientResponseError populated with the HTTP status code and response body (use the existing resp and body variables) and include a clear message like "attestation quote failed" so callers can assert on the concrete ClientResponseError type instead of a plain error.
🤖 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 `@make/setup-cache.mk`:
- Line 155: The target rule for $(GOLANGCI_LINT_VERSION_FILE) incorrectly lists
$(AP_DEVCACHE) as a prerequisite but the cache target defined in this Makefile
is $(AKASH_DEVCACHE); update the prerequisite to use $(AKASH_DEVCACHE) so the
cache is properly initialized. Locate the rule named
"$(GOLANGCI_LINT_VERSION_FILE): $(SEMVER) $(AP_DEVCACHE)" and replace the
$(AP_DEVCACHE) token with $(AKASH_DEVCACHE) to match the existing cache target
and ensure the dependency is executed.
---
Nitpick comments:
In `@go/provider/client/client.go`:
- Around line 924-926: Replace the plain fmt.Errorf return in the non-OK branch
with a typed ClientResponseError to match the package's API error handling: when
resp.StatusCode != http.StatusOK build and return a ClientResponseError
populated with the HTTP status code and response body (use the existing resp and
body variables) and include a clear message like "attestation quote failed" so
callers can assert on the concrete ClientResponseError type instead of a plain
error.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86c1e270-5e7b-47fe-bc4b-dd9a2fe9eacd
⛔ Files ignored due to path filters (1)
go/manifest/v2beta3/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
Makefilebuf.yamlgo/provider/client/client.gogo/provider/client/path.gogo/sdl/_testdata/v2.1-confidential-compute-cpu-only.yamlgo/sdl/_testdata/v2.1-confidential-compute-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-no-attestation.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx.yamlgo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/sdl-input.schema.yamlgo/sdl/v2.gogo/sdl/v2_1_test.gomake/setup-cache.mkproto/provider/akash/manifest/v2beta3/service.proto
cloud-j-luna
force-pushed
the
feature/confidential-compute
branch
from
2b83c04 to
43a8af1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@proto/provider/akash/manifest/v2beta3/service.proto`:
- Around line 49-54: The proto's attestation boolean in TEEParams cannot express
a true default at the wire level (proto3 zero value is false), so fix by
changing the field to a wrapper type or by ensuring producers set it explicitly:
replace "bool attestation = 2" with "google.protobuf.BoolValue attestation = 2"
(and import google/protobuf/wrappers.proto) so an omitted field is
distinguishable, and update any Go/SDL builders (the TEEParams construction
code) to explicitly set attestation to true when omitted; alternatively, if you
prefer not to change the wire format, update documentation and all
producers/builders to always assign attestation = true in TEEParams creation
(reference symbols: attestation, TEEParams, service.proto).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b90c7160-7679-4804-9c99-ffec49068fad
⛔ Files ignored due to path filters (2)
go/manifest/v2beta3/service.pb.gois excluded by!**/*.pb.gogo/node/types/resources/v1beta4/tee.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
Makefilego/node/deployment/v1beta4/groupspec.gogo/provider/client/client.gogo/provider/client/path.gogo/sdl/_testdata/v2.1-confidential-compute-cpu-only.yamlgo/sdl/_testdata/v2.1-confidential-compute-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-no-attestation.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx.yamlgo/sdl/_testdata/v2.1-tee-snp-gpu.yamlgo/sdl/_testdata/v2.1-tee-snp.yamlgo/sdl/_testdata/v2.1-tee-tdx-no-attestation.yamlgo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/sdl-input.schema.yamlgo/sdl/tee.gogo/sdl/v2.gogo/sdl/v2_1_test.gomake/setup-cache.mkproto/node/akash/base/resources/v1beta4/tee.protoproto/provider/akash/manifest/v2beta3/service.proto
✅ Files skipped from review due to trivial changes (3)
- go/sdl/_testdata/v2.1-tee-snp.yaml
- go/node/deployment/v1beta4/groupspec.go
- go/sdl/_testdata/v2.1-confidential-compute-cpu-only.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- go/sdl/_testdata/v2.1-confidential-compute-tdx.yaml
- go/sdl/v2.go
- go/provider/client/path.go
- go/sdl/_testdata/v2.1-confidential-compute-no-attestation.yaml
- go/sdl/_testdata/v2.1-tee-tdx-no-attestation.yaml
- go/sdl/_testdata/v2.1-confidential-compute-gpu.yaml
- go/sdl/_testdata/v2.1-confidential-compute-tdx-gpu.yaml
- Makefile
- go/provider/client/client.go
- go/sdl/tee.go
- make/setup-cache.mk
- go/sdl/_testdata/v2.1-tee-snp-gpu.yaml
- go/sdl/sdl-input.schema.yaml
- go/sdl/groupBuilder_v2_1.go
- go/sdl/v2_1_test.go
- go/sdl/groupBuilder_v2.go
| // attestation controls whether the provider injects an attestation sidecar. | ||
| // Defaults to true. Set to false to bring your own attestation. | ||
| bool attestation = 2 [ | ||
| (gogoproto.jsontag) = "attestation", | ||
| (gogoproto.moretags) = "yaml:\"attestation\"" | ||
| ]; |
There was a problem hiding this comment.
attestation "defaults to true" cannot be represented at the wire level.
proto3 bool zero value is false, so an unset/omitted attestation decodes to false, not true. The Go SDL builder explicitly sets it to true when omitted, so manifests produced through this repo are safe. However, any consumer that constructs TEEParams directly (e.g. a non-Go client) will get attestation disabled by default — a security-relevant divergence from the documented contract for confidential compute. Ensure all producers set this field explicitly, or document that the default is enforced only by the producer.
🧰 Tools
🪛 Buf (1.69.0)
[error] 51-51: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 52-52: cannot find gogoproto.jsontag in this scope
(COMPILE)
[error] 53-53: cannot find gogoproto.moretags in this scope
(COMPILE)
[error] 54-54: cannot find gogoproto.nullable in this scope
(COMPILE)
[error] 52-52: cannot find gogoproto.jsontag in this scope
(COMPILE)
[error] 53-53: cannot find gogoproto.moretags in this scope
(COMPILE)
[error] 51-51: cannot find gogoproto.customname in this scope
(COMPILE)
[error] 52-52: cannot find gogoproto.jsontag in this scope
(COMPILE)
[error] 53-53: cannot find gogoproto.moretags in this scope
(COMPILE)
🤖 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 `@proto/provider/akash/manifest/v2beta3/service.proto` around lines 49 - 54,
The proto's attestation boolean in TEEParams cannot express a true default at
the wire level (proto3 zero value is false), so fix by changing the field to a
wrapper type or by ensuring producers set it explicitly: replace "bool
attestation = 2" with "google.protobuf.BoolValue attestation = 2" (and import
google/protobuf/wrappers.proto) so an omitted field is distinguishable, and
update any Go/SDL builders (the TEEParams construction code) to explicitly set
attestation to true when omitted; alternatively, if you prefer not to change the
wire format, update documentation and all producers/builders to always assign
attestation = true in TEEParams creation (reference symbols: attestation,
TEEParams, service.proto).
cloud-j-luna
force-pushed
the
feature/confidential-compute
branch
from
d5b8443 to
6d7c725
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
proto/provider/akash/manifest/v2beta3/service.proto (1)
49-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
attestation"defaults to true" is not representable at the wire level.A proto3
boolzero-value isfalse, so any producer that doesn't explicitly setattestation(e.g. a non-Go client constructingTEEParamsdirectly) will get attestation disabled, diverging from the documented default. The Go SDL builder sets it explicitly, so manifests from this repo are safe, but the default is producer-enforced only. Consider a wrapper type (google.protobuf.BoolValue) or clearly documenting that the default is enforced by producers.🤖 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 `@proto/provider/akash/manifest/v2beta3/service.proto` around lines 49 - 54, The attestation field currently declared as a proto3 bool (attestation = 2) cannot express a default of true at the wire level; change its type to google.protobuf.BoolValue and import google/protobuf/wrappers.proto so absence vs explicit false is distinguishable (i.e., replace "bool attestation = 2" with "google.protobuf.BoolValue attestation = 2" and update any gogoproto.jsontag/moretags as needed), then update any builders/usage (e.g., where TEEParams is constructed) to treat a nil wrapper as the default-true behavior or add clear comments documenting the new semantics.
🤖 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.
Duplicate comments:
In `@proto/provider/akash/manifest/v2beta3/service.proto`:
- Around line 49-54: The attestation field currently declared as a proto3 bool
(attestation = 2) cannot express a default of true at the wire level; change its
type to google.protobuf.BoolValue and import google/protobuf/wrappers.proto so
absence vs explicit false is distinguishable (i.e., replace "bool attestation =
2" with "google.protobuf.BoolValue attestation = 2" and update any
gogoproto.jsontag/moretags as needed), then update any builders/usage (e.g.,
where TEEParams is constructed) to treat a nil wrapper as the default-true
behavior or add clear comments documenting the new semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27187bf8-8df3-4511-ac7d-ee38a8c1f395
⛔ Files ignored due to path filters (2)
go/manifest/v2beta3/service.pb.gois excluded by!**/*.pb.gogo/node/types/resources/v1beta4/tee.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
Makefilego/node/deployment/v1beta4/groupspec.gogo/provider/client/client.gogo/provider/client/path.gogo/sdl/_testdata/v2.1-confidential-compute-cpu-only.yamlgo/sdl/_testdata/v2.1-confidential-compute-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-no-attestation.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx-gpu.yamlgo/sdl/_testdata/v2.1-confidential-compute-tdx.yamlgo/sdl/_testdata/v2.1-tee-snp-gpu.yamlgo/sdl/_testdata/v2.1-tee-snp.yamlgo/sdl/_testdata/v2.1-tee-tdx-no-attestation.yamlgo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/sdl-input.schema.yamlgo/sdl/tee.gogo/sdl/v2.gogo/sdl/v2_1_test.gomake/setup-cache.mkproto/node/akash/base/resources/v1beta4/tee.protoproto/provider/akash/manifest/v2beta3/service.proto
✅ Files skipped from review due to trivial changes (5)
- go/sdl/_testdata/v2.1-tee-snp.yaml
- go/sdl/_testdata/v2.1-confidential-compute-no-attestation.yaml
- go/sdl/_testdata/v2.1-confidential-compute-tdx.yaml
- go/sdl/v2.go
- go/node/deployment/v1beta4/groupspec.go
🚧 Files skipped from review as they are similar to previous changes (13)
- Makefile
- go/sdl/_testdata/v2.1-tee-snp-gpu.yaml
- go/sdl/_testdata/v2.1-confidential-compute-tdx-gpu.yaml
- go/sdl/_testdata/v2.1-confidential-compute-cpu-only.yaml
- go/sdl/groupBuilder_v2_1.go
- go/provider/client/path.go
- go/provider/client/client.go
- go/sdl/_testdata/v2.1-confidential-compute-gpu.yaml
- go/sdl/sdl-input.schema.yaml
- go/sdl/v2_1_test.go
- go/sdl/_testdata/v2.1-tee-tdx-no-attestation.yaml
- go/sdl/tee.go
- make/setup-cache.mk
|
|
||
| // v2ResourceTEE defines the TEE (Trusted Execution Environment) configuration | ||
| // for a compute resource. Follows the same pattern as v2ResourceStorage. | ||
| type v2ResourceTEE struct { |
There was a problem hiding this comment.
correct for params is v2TEEParams
| } | ||
|
|
||
| // IsGPUTEEType returns true if the TEE type requires GPU confidential compute. | ||
| func IsGPUTEEType(t string) bool { |
There was a problem hiding this comment.
how do we know at SDL build time if GPU has TEE? what if provider has it disabled?
2 participants
📝 Description
This PR adds support to Confidential Compute to SDL and provider client.
🔧 Purpose of the Change
✅ Checklist