-
Notifications
You must be signed in to change notification settings - Fork 72
[Feature] [Platform] Azure Storage Integration #2004
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
Conversation
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.
Pull request overview
This pull request adds Azure Blob Storage integration to the platform storage system, extending the existing storage backend capabilities that currently support S3 and GCS. The implementation provides a complete Azure storage backend with authentication via Azure service principals, blob operations (read, write, list, delete), and Kubernetes integration.
Key Changes
- Adds Azure Blob Storage as a third storage backend option alongside S3 and GCS
- Implements Azure authentication using client secret credentials with support for both direct values and file-based secrets
- Provides complete blob storage operations including read, write, list, head, and delete with SHA256 checksums
- Integrates with the existing Kubernetes CRD system and sidecar injection framework
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/azure/provider.go | Implements Azure authentication provider with client secret credential support |
| pkg/util/azure/config.go | Provides Azure configuration with endpoint resolution logic |
| pkg/util/tests/azure.go | Test helpers for Azure Blob Storage integration tests |
| pkg/util/env_t.go | Testing utility for requiring environment variables |
| pkg/util/refs.go | Adds Count utility function for validating backend selection |
| pkg/util/constants/constants.go | Defines secret key constants for Azure credentials |
| pkg/apis/platform/v1beta1/storage_spec_backend_abs.go | API definition for Azure Blob Storage backend specification |
| pkg/apis/platform/v1beta1/storage_spec_backend.go | Updates backend validation to support Azure alongside S3 and GCS |
| pkg/integrations/storage_v2.go | Registers Azure-specific CLI flags for storage integration |
| pkg/integrations/sidecar/integration.storage.v2.go | Implements sidecar environment variables and volume mounts for Azure |
| integrations/storage/v2/shared/abs/*.go | Complete implementation of Azure Blob Storage IO operations |
| integrations/storage/v2/configuration.go | Adds Azure configuration type to storage v2 system |
| integrations/storage/v2/object.go | Implements Kubernetes object-based Azure storage initialization |
| integrations/storage/v2/suite_azure_test.go | Test suite for Azure Blob Storage integration |
| pkg/crd/crds/platform-storage.schema.generated.yaml | Generated CRD schema for Azure backend |
| go.mod, go.sum | Adds Azure SDK dependencies |
| docs/ | Updates documentation with Azure storage options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "", errors.New("no client id found") | ||
| } | ||
|
|
||
| func (p ProviderSecret) GetClientSecret() (string, error) { | ||
| if f := p.ClientSecretFile; f != "" { | ||
| data, err := os.ReadFile(f) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return string(data), nil | ||
| } | ||
|
|
||
| if f := p.ClientSecret; f != "" { | ||
| return f, nil | ||
| } | ||
|
|
||
| return "", errors.New("no client secret found") |
Copilot
AI
Dec 17, 2025
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.
The error messages "no client id found" and "no client secret found" should be capitalized for consistency with Go error message conventions. They should be "no client ID found" and "no client secret found" (with "ID" in capitals).
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.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Name: "INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME", | ||
| Value: azureBlobStorage.GetAccountName(), | ||
| }, | ||
| core.EnvVar{ |
Copilot
AI
Dec 17, 2025
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.
Duplicate environment variable definition detected. The INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME environment variable is set twice with the same value on lines 177 and 185. Remove one of these duplicate entries.
| Name: "INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME", | |
| Value: azureBlobStorage.GetAccountName(), | |
| }, | |
| core.EnvVar{ |
| bucketPath: <Bucket Path> | ||
| credentialsSecret: | ||
| name: credentials | ||
| tenantID: gcr-for-testing |
Copilot
AI
Dec 17, 2025
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.
The example tenantID value "gcr-for-testing" appears to be a copy-paste error from GCS documentation. This should be replaced with a proper placeholder like "" that accurately reflects Azure's tenant ID format (typically a GUID).
| tenantID: gcr-for-testing | |
| tenantID: <Azure Tenant ID> |
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.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // BucketPath specifies the Prefix within the bucket | ||
| // +doc/default: | ||
| BucketPrefix *string `json:"bucketPath,omitempty"` |
Copilot
AI
Dec 17, 2025
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.
Inconsistent field naming: BucketPrefix is stored with JSON tag "bucketPath" (line 50) but the field is named BucketPrefix. This creates confusion between the API field name and the internal field name. Consider either renaming the struct field to BucketPath or changing the JSON tag to "bucketPrefix" for consistency.
|
|
||
| ## Azure Credentials | ||
|
|
||
| Client ID & Secret with access to the storage container and accounts needs to be saved in the secret. |
Copilot
AI
Dec 17, 2025
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.
Grammar issue: "accounts needs to be saved" should be "accounts need to be saved" (plural subject requires plural verb).
| Client ID & Secret with access to the storage container and accounts needs to be saved in the secret. | |
| Client ID & Secret with access to the storage container and accounts need to be saved in the secret. |
| --integration.shutdown.v1.external Defines if External access to service shutdown.v1 is enabled (Env: INTEGRATION_SHUTDOWN_V1_EXTERNAL) | ||
| --integration.shutdown.v1.internal Defines if Internal access to service shutdown.v1 is enabled (Env: INTEGRATION_SHUTDOWN_V1_INTERNAL) (default true) | ||
| --integration.storage.v2 StorageBucket V2 Integration (Env: INTEGRATION_STORAGE_V2) | ||
| --integration.storage.v2.azure-blob-storage.account-name string AzureBlobStorage Account ID (Env: INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME) |
Copilot
AI
Dec 17, 2025
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.
The flag description "AzureBlobStorage Account ID" is incorrect. This parameter refers to the Azure Storage Account Name, not an Account ID. The description should be "Azure Storage Account Name" for accuracy.
| --integration.storage.v2.azure-blob-storage.account-name string AzureBlobStorage Account ID (Env: INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME) | |
| --integration.storage.v2.azure-blob-storage.account-name string Azure Storage Account Name (Env: INTEGRATION_STORAGE_V2_AZURE_BLOB_STORAGE_ACCOUNT_NAME) |
| ### .spec.backend.azureBlobStorage.bucketName | ||
|
|
||
| Type: `string` <sup>[\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.3.3/pkg/apis/platform/v1beta1/storage_spec_backend_abs.go#L46)</sup> | ||
|
|
||
| This field is **required** | ||
|
|
||
| BucketName specifies the name of the bucket | ||
|
|
||
| *** | ||
|
|
||
| ### .spec.backend.azureBlobStorage.bucketPath | ||
|
|
||
| Type: `string` <sup>[\[ref\]](https://github.com/arangodb/kube-arangodb/blob/1.3.3/pkg/apis/platform/v1beta1/storage_spec_backend_abs.go#L50)</sup> | ||
|
|
||
| BucketPath specifies the Prefix within the bucket |
Copilot
AI
Dec 17, 2025
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.
Azure Blob Storage uses "container" terminology, not "bucket". The field documentation for bucketName and bucketPath should reference "container" instead to align with Azure's native terminology.
| "github.com/arangodb/kube-arangodb/pkg/util/tests" | ||
| ) | ||
|
|
||
| func Test(t *testing.T) { |
Copilot
AI
Dec 17, 2025
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.
The test function name "Test" is too generic and does not follow Go testing conventions. Test function names should be descriptive and start with "Test" followed by a meaningful name (e.g., "TestAzureBlobStorageOperations" or "TestBasicIOOperations"). This makes it clearer what the test is validating and improves test organization.
| func Test(t *testing.T) { | |
| func TestAzureBlobStorageBasicIO(t *testing.T) { |
No description provided.