-
Notifications
You must be signed in to change notification settings - Fork 116
Contributing Vault Coding skills #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,256 @@ | ||
| --- | ||
| name: vault-architecture | ||
| description: Design and organize code in HashiCorp Vault. Use when designing new features, refactoring code, working with CE/EE splits, making API design decisions, understanding Vault's plugin architecture, or deciding where code should live (api/ vs sdk/ vs vault/). | ||
| compatibility: Requires Go 1.22+, access to Vault repository | ||
| --- | ||
|
|
||
| # Vault Architecture | ||
|
|
||
| ## Repository Structure | ||
|
|
||
| ### Public vs Internal | ||
|
|
||
| Only these packages are public (importable externally): | ||
| ``` | ||
| api/ # Vault API client | ||
| sdk/ # Plugin SDK | ||
| ``` | ||
|
|
||
| Everything in `vault/` is internal - never import directly. | ||
|
|
||
| ### Core Organization | ||
|
|
||
| ``` | ||
| vault/ # Core server (INTERNAL) | ||
| ├── logical/ # Backend interfaces | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no vault/logical directory. |
||
| ├── physical/ # Storage backends | ||
| └── audit/ # Audit backends | ||
|
|
||
| builtin/ # Built-in plugins | ||
| ├── logical/ # Secret engines | ||
| └── credential/ # Auth methods | ||
|
|
||
| command/ # CLI commands | ||
| http/ # HTTP API handlers | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be confusing because most of the HTTP API handlers that don't live here. This is more like the lower-level parts of our HTTP API and surrounding tooling, together with some of the less conventional HTTP handlers that don't use the SDK so much. |
||
| ``` | ||
|
|
||
| ## CE/EE Code Separation | ||
|
|
||
| Use build tags for compile-time separation: | ||
|
arnabkaycee marked this conversation as resolved.
|
||
|
|
||
| ### File Naming | ||
|
|
||
| ``` | ||
| feature.go # Shared (CE + EE) | ||
| feature_oss.go # Community Edition / Open Source only | ||
| feature_ent.go # Enterprise only | ||
| feature_test.go # Shared tests | ||
| feature_ent_test.go # Enterprise tests only | ||
| ``` | ||
|
|
||
| ### Build Tags | ||
|
|
||
| ```go | ||
| //go:build !enterprise | ||
| // CE-only code | ||
|
|
||
| //go:build enterprise | ||
| // EE-only code | ||
| ``` | ||
|
|
||
| ### Pattern: Interface-Based Separation | ||
|
|
||
| ```go | ||
| // feature.go - shared interface | ||
| type FeatureManager interface { | ||
| Process(ctx context.Context) error | ||
| GetCapabilities() []string | ||
| } | ||
|
|
||
| // feature_oss.go | ||
| //go:build !enterprise | ||
|
|
||
| func NewFeatureManager() FeatureManager { | ||
| return &ossManager{} // Basic implementation | ||
| } | ||
|
|
||
| // feature_ent.go | ||
| //go:build enterprise | ||
|
|
||
| func NewFeatureManager() FeatureManager { | ||
| return &entManager{ // Advanced implementation | ||
| replicator: NewReplicator(), | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Key rules**: | ||
| - Define interface in shared file | ||
| - Same function signatures in both files | ||
| - Use build tags, not runtime checks | ||
| - Test both editions: `make subtest` (CE), `make test` (EE) | ||
|
arnabkaycee marked this conversation as resolved.
|
||
|
|
||
| ## API Design | ||
|
|
||
| ### RESTful Endpoints | ||
|
|
||
| ``` | ||
| Create: POST /v1/resource | ||
| Read: GET /v1/resource/:id | ||
| Update: POST /v1/resource/:id | ||
| Delete: DELETE /v1/resource/:id | ||
| List: LIST /v1/resource # Note: LIST not GET | ||
|
arnabkaycee marked this conversation as resolved.
|
||
| ``` | ||
|
|
||
| ### Request/Response Pattern | ||
|
|
||
| ```go | ||
| func (b *backend) pathEntityCreate( | ||
| ctx context.Context, | ||
| req *logical.Request, | ||
| data *framework.FieldData, | ||
| ) (*logical.Response, error) { | ||
| // Parse | ||
| name := data.Get("name").(string) | ||
|
|
||
| // Validate | ||
| if err := validateName(name); err != nil { | ||
| return logical.ErrorResponse(err.Error()), nil | ||
| } | ||
|
|
||
| // Process | ||
| entity, err := b.createEntity(ctx, name) | ||
| if err != nil { | ||
| return nil, err // Internal error | ||
| } | ||
|
|
||
| // Return | ||
| return &logical.Response{ | ||
| Data: map[string]interface{}{ | ||
| "id": entity.ID, | ||
| }, | ||
| }, nil | ||
| } | ||
| ``` | ||
|
|
||
| **Error handling**: | ||
| - `logical.ErrorResponse()` for validation errors (user-facing) | ||
| - `return nil, err` for internal errors (logged, user sees generic message) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also frequently do both. I think that's when we want to ensure that the http status code is appropriate while also including a user-facing error message. |
||
|
|
||
| ## Plugin Architecture | ||
|
|
||
| ### Backend Interface | ||
|
|
||
| ```go | ||
| func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) { | ||
| b := &backend{} | ||
|
|
||
| b.Backend = &framework.Backend{ | ||
| BackendType: logical.TypeLogical, | ||
| Paths: []*framework.Path{ | ||
| b.pathCreate(), | ||
| b.pathRead(), | ||
| }, | ||
| } | ||
|
|
||
| if err := b.Setup(ctx, conf); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return b, nil | ||
| } | ||
| ``` | ||
|
|
||
| ## Storage Patterns | ||
|
|
||
| ```go | ||
| // Write | ||
| entry := &logical.StorageEntry{ | ||
| Key: "entity/" + id, | ||
| Value: marshaledData, | ||
| } | ||
| req.Storage.Put(ctx, entry) | ||
|
|
||
| // Read | ||
| entry, err := req.Storage.Get(ctx, "entity/"+id) | ||
|
|
||
| // Delete | ||
| req.Storage.Delete(ctx, "entity/"+id) | ||
|
|
||
| // List | ||
| keys, err := req.Storage.List(ctx, "entity/") | ||
| ``` | ||
|
|
||
| **Key naming**: | ||
| ``` | ||
| entity/<id> # Single entity | ||
| entity/<id>/alias/<alias_id> # Nested | ||
| config/ # Configuration | ||
| ``` | ||
|
|
||
| ## Decision Trees | ||
|
|
||
| ### Where Does Code Go? | ||
|
|
||
| ``` | ||
| Is it a public API? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's some ambiguity here around "API", since we define HTTP APIs that can live in backends or core, as well as Go APIs. And then to further confuse matters, the code in api/ is generally Go code that facilitates access to our HTTP APIs. |
||
| ├─ YES → api/ or sdk/ | ||
| │ ├─ External consumers need it → api/ | ||
| │ └─ Plugin developers need it → sdk/ | ||
| └─ NO → CE, EE, or both? | ||
| ├─ Both → .go file | ||
| ├─ CE only → _oss.go file | ||
|
arnabkaycee marked this conversation as resolved.
|
||
| └─ EE only → _ent.go file | ||
| ``` | ||
|
|
||
| **Default rule**: If external projects might need it, put in `api/` or `sdk/`. Otherwise, use `vault/`. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the vast majority of code we introduce isn't intended to be used by external projects as Go code, so I worry that the "might" is going to push in the wrong direction of defaulting toward exposing stuff publicly, when the default should be the reverse. We also have had a problem for years of |
||
|
|
||
| ### When to Split CE/EE? | ||
|
|
||
| Use CE/EE split when: | ||
| - Feature exists in both with different implementations | ||
| - EE adds significant capabilities | ||
| - Single codebase needed | ||
|
Comment on lines
+212
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No sure if I understand what these two mean. Can you ellaborate? |
||
|
|
||
| Don't split when: | ||
| - Feature identical in both | ||
| - Feature 100% EE-only (just use `_ent.go`) | ||
| - Difference is trivial | ||
|
|
||
| ## Adding New Components | ||
|
|
||
| ### New Secret Engine | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These don't belong in the vault repo nowadays, they should be in a standalone plugin repo. Same with auth methods. |
||
|
|
||
| 1. Create package in `builtin/logical/<name>/` | ||
| 2. Implement `logical.Backend` interface | ||
| 3. Define paths using `framework.PathAppend` | ||
| 4. Add CRUD operations | ||
| 5. Implement secret revocation | ||
| 6. Write tests + acceptance tests | ||
|
|
||
| ### New Auth Method | ||
|
|
||
| 1. Create package in `builtin/credential/<name>/` | ||
| 2. Implement `logical.Backend` interface | ||
| 3. Define authentication paths | ||
| 4. Implement token generation | ||
| 5. Add credential validation | ||
| 6. Write tests | ||
|
|
||
| ## Dependency Management | ||
|
|
||
| ```bash | ||
| # Add dependency | ||
| go get github.com/example/package@latest | ||
|
|
||
| # Update go.mod | ||
| make go-mod-tidy | ||
|
|
||
| # Verify tests | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Running tests should apply to every change so I don't know that we need to call it out here. And adding a new module shouldn't be able to break existing code. |
||
| make test TEST=./path/to/package | ||
| ``` | ||
|
|
||
| **Rules**: | ||
| - Pin exact versions | ||
|
arnabkaycee marked this conversation as resolved.
|
||
| - Minimize dependencies (smaller attack surface) | ||
| - Run full tests after any dep change | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only applies to external projects though.