Added Azure bundle publisher#7030
Conversation
sabre1041
requested review from
MarcosDY,
amartinezfayo,
evan2645,
rturner3 and
sorindumitru
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new SPIRE server BundlePublisher plugin (azure_blob) that publishes the trust bundle to Azure Blob Storage with multiple auth options.
Changes:
- Introduces the Azure Blob bundle publisher implementation and Azure SDK client wrapper.
- Registers the new built-in plugin in the server catalog and adds Azure SDK dependencies.
- Adds comprehensive unit tests and end-user documentation for configuration/auth.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/server/plugin/bundlepublisher/azureblob/azureblob.go | Implements the BundlePublisher + config parsing/auth selection logic. |
| pkg/server/plugin/bundlepublisher/azureblob/client.go | Adds a thin interface/wrapper for azblob.Client to aid testing/injection. |
| pkg/server/plugin/bundlepublisher/azureblob/azureblob_test.go | Adds unit tests for configuration/auth paths, publishing, and refresh hint behavior. |
| pkg/server/catalog/bundlepublisher.go | Registers azure_blob as a built-in bundle publisher. |
| go.mod | Adds github.com/Azure/azure-sdk-for-go/sdk/storage/azblob dependency. |
| go.sum | Updates dependency checksums for new/updated Azure SDK modules. |
| doc/plugin_server_bundlepublisher_azure_blob.md | Documents the new plugin, configuration options, and auth methods. |
| p.blobClient = blobClient | ||
|
|
||
| p.setConfig(newConfig) | ||
| p.setBundle(nil) | ||
| return &configv1.ConfigureResponse{}, nil |
There was a problem hiding this comment.
This is a pre-existing pattern in the aws_s3 and gcp_cloudstorage publishers, where the client field is assigned in Configure and read in PublishBundle without a lock. It only became concurrently reachable after dynamic reconfiguration was added, since Configure previously ran once at initialization. I'll open a separate PR to fix it in the other two plugins, so addressing it here is optional and either way is fine with me.
There was a problem hiding this comment.
Opened #7082 to fix this in the aws_s3 and gcp_cloudstorage publishers.
| formatter := bundleformat.NewFormatter(bundleToPublish) | ||
| bundleBytes, err := formatter.Format(config.bundleFormat) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "could not format bundle: %v", err.Error()) |
There was a problem hiding this comment.
No change needed. This matches the identical format error in aws_s3 and gcp_cloudstorage, and %v on the resulting string is equivalent, so changing only this plugin would introduce an inconsistency for no behavioral gain.
| | Configuration | Description | Required | Default | | ||
| |----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------|----------------------| | ||
| | storage_account_name | The name of the Azure Storage account. | Yes. | | | ||
| | storage_account_key | The Azure Storage account access key for shared key authentication. | Required only when using shared key authentication. | | |
There was a problem hiding this comment.
This one is worth addressing. Clarifying in the description and sample that storage_account_key expects the storage account access key, and not a connection string or SAS token, would help avoid a real misconfiguration.
| BundlePublisher "azure_blob" { | ||
| plugin_data { | ||
| storage_account_name = "mystorageaccount" | ||
| storage_account_key = "my-storage-account-key" |
sabre1041
force-pushed
the
azure-bundle-publisher
branch
from
69fe1f9 to
b25e93e
Compare
Signed-off-by: Andrew Block <andy.block@gmail.com>
sabre1041
force-pushed
the
azure-bundle-publisher
branch
from
b25e93e to
bbe0586
Compare
| p.log.Warn(note) | ||
| } | ||
|
|
||
| serviceEndpoint := newConfig.ServiceEndpoint |
There was a problem hiding this comment.
can we put some validation on serviceEndpoint here as done here ?
There was a problem hiding this comment.
Thanks for flagging this. I agree some validation here would be good. One thing to keep in mind is that the aws_s3 check uses url.ParseRequestURI because its endpoint is a full URL, whereas service_endpoint here is a bare host suffix (default blob.core.windows.net), so that exact check would reject the valid default. It would be better to validate the host form, or validate the constructed account URL, and to do it in buildConfig so that spire-server validate catches it too.
| return nil, status.Errorf(codes.Internal, "could not format bundle: %v", err.Error()) | ||
| } | ||
|
|
||
| _, err = p.blobClient.UploadBuffer(ctx, config.ContainerName, config.BlobName, bundleBytes, nil) |
There was a problem hiding this comment.
we should pass appropriate UploadBufferOptions with HTTPHeaders containing the correct content-type based on config.bundleFormat. Right now UploadBufferOptions is passed as nil, which means the blob is uploaded with Azure's default content type (application/octet-stream).
There was a problem hiding this comment.
Thanks for raising this. Passing nil does leave the blob stored as application/octet-stream. Worth noting that neither aws_s3 nor gcp_cloudstorage set a content type today, so I would treat this as an optional improvement rather than something needed for parity with the existing publishers. If we do want correct content types (json for spiffe and jwks, a pem type for pem), I think it would be best to apply it consistently across all three publishers in a separate change.
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @sabre1041 for this contribution!
|
|
||
| var blobClient blobStorage | ||
|
|
||
| switch { |
There was a problem hiding this comment.
It seems that the auth-field validation here only runs in Configure, so the Validate RPC that backs spire-server validate won't catch those misconfigurations. So a config that sets both storage_account_key and the client-secret fields, or only a partial service principal, would validate as OK and then fail at startup.
I think we should move the pure validation into buildConfig via status.ReportError (the way aws_s3 validates its endpoint there) and keep only the credential and client creation in Configure.
| @@ -0,0 +1,104 @@ | |||
| # Server plugin: BundlePublisher "azure_blob" | |||
There was a problem hiding this comment.
This new plugin needs an entry in the BundlePublisher table in doc/spire_server.mdto to keep the built-in plugin index updated.
|
Thanks for the review. Based on the reviews thus far, please share any actions that I would be able to take for this contribution. Happy to address any area that may need enhancement in order to resolve any identified deficiencies. |
Pull Request check list
Affected functionality
Extend existing
BundlePublishercapability by adding support for publishing the bundle to Azure BlobDescription of change
New BundlePublisher for integrating with Azure Blob. Similar capability to existing AWS and GCP bundle publisher
Which issue this PR fixes
N/A