Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ tools/bin
*~
.vscode
.envrc
.env
.DS_Store

.kube
Expand Down
4 changes: 4 additions & 0 deletions control-plane-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/openshift/hypershift/dnsresolver"
etcdbackup "github.com/openshift/hypershift/etcd-backup"
etcddefrag "github.com/openshift/hypershift/etcd-defrag"
etcdupload "github.com/openshift/hypershift/etcd-upload"
ignitionserver "github.com/openshift/hypershift/ignition-server/cmd"
kasbootstrap "github.com/openshift/hypershift/kas-bootstrap"
konnectivityhttpsproxy "github.com/openshift/hypershift/konnectivity-https-proxy"
Expand Down Expand Up @@ -97,6 +98,8 @@ func commandFor(name string) *cobra.Command {
cmd = tokenminter.NewStartCommand()
case "etcd-defrag-controller":
cmd = etcddefrag.NewStartCommand()
case "etcd-upload":
cmd = etcdupload.NewStartCommand()
case "sync-fg-configmap":
cmd = syncfgconfigmap.NewRunCommand()
case "sync-global-pullsecret":
Expand Down Expand Up @@ -156,6 +159,7 @@ func defaultCommand() *cobra.Command {
cmd.AddCommand(dnsresolver.NewCommand())
cmd.AddCommand(etcdbackup.NewStartCommand())
cmd.AddCommand(etcdbackup.NewFetchCertsCommand())
cmd.AddCommand(etcdupload.NewStartCommand())
cmd.AddCommand(kasbootstrap.NewRunCommand())
cmd.AddCommand(syncfgconfigmap.NewRunCommand())
cmd.AddCommand(syncglobalpullsecret.NewRunCommand())
Expand Down
178 changes: 178 additions & 0 deletions etcd-upload/azure_uploader.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
package etcdupload

import (
"context"
"encoding/json"
"fmt"
"os"

"github.com/Azure/azure-sdk-for-go/sdk/azcore"
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob"
"github.com/Azure/msi-dataplane/pkg/dataplane"
)

const (
// AuthTypeClientSecret uses a JSON file with client ID, secret, and tenant ID.
AuthTypeClientSecret = "client-secret"
// AuthTypeManagedIdentity uses msi-dataplane with a certificate file mounted
// via CSI SecretProviderClass (ARO HCP).
AuthTypeManagedIdentity = "managed-identity"
)

// AzureBlobUploader uploads etcd snapshots to Azure Blob Storage.
type AzureBlobUploader struct {
container string
storageAccount string
encryptionScope string
client AzureBlobUploadAPI
}

// AzureBlobUploadAPI defines the Azure Blob client interface used by the uploader.
type AzureBlobUploadAPI interface {
UploadFile(ctx context.Context, containerName string, blobName string, file *os.File, o *azblob.UploadFileOptions) (azblob.UploadFileResponse, error)
}

// azureCredentialsFile represents the Azure credentials JSON file format.
type azureCredentialsFile struct {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jparrill @tony-schndr - what credential is this in ARO HCP? Like where does it come from? How is it plumbed here? Can you help me understand the architecture flow a bit better please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, from my side:

  • This PR delivers the etcd-upload CLI subcommand - the credential plumbing is not wired yet. That's the controller's responsibility (CNTRLPLANE-2678), which will create the backup Jobs and mount the appropriate credentials.

  • For ARO HCP, we know the CPO itself authenticates via msi-dataplane.NewUserAssignedIdentityCredential with a certificate mounted through a SecretProviderClass + CSI driver. The etcd-upload command currently supports ClientSecretCredential (via JSON file) and DefaultAzureCredential as fallback - neither covers the ARO HCP auth path. This is a known gap that will need to be addressed when implementing the controller:

    1. A managed (I assume new because should be provided by ARO ppl) identity for the etcd-upload Job
    2. A SecretProviderClass for the Job pod to mount the cert from Key Vault
    3. Either adapting azure_uploader.go to support msi-dataplane credentials (in this PR, adding support to managed identities as auth-type)
  • For self-managed Azure, the CPO currently has no Azure credentials at all, so this is a broader gap that predates this work.

@tony-schndr tony-schndr Mar 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understood in the proposal since HO was running the job in the HO namespace. Therefore upload could use the managed identity that is attached to HO. We would add RBAC so that HO can access the storage account. @jparrill is my understanding correct?

ref: https://github.com/jparrill/enhancements/blob/eb0772d8ee369e83bbad6aa1d7710e6ce8145b34/enhancements/hypershift/etcd-backup-crd-for-oadp-integration.md?plain=1#L79-L82

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your understanding is correct. The Job runs in the HO namespace, so it could leverage the HO's identity. We've just added --auth-type managed-identity support in this PR, which uses msi-dataplane.NewUserAssignedIdentityCredential to consume the certificate mounted via SecretProviderClass — so the etcd-upload binary is ready for this path.

However, the HO identity today does not have storage permissions in either platform, so we'll need to extend them as part of the controller work (CNTRLPLANE-2678):

ARO HCP (Azure Blob Storage): We'd need to assign Storage Blob Data Contributor role to the HO's managed identity, scoped to the target storage account/container.

ROSA HCP (S3): The HO's OIDC S3 credentials only cover OIDC bucket operations. We'd need to add S3 permissions for the backup bucket: s3:PutObject, s3:CreateMultipartUpload, s3:UploadPart, s3:CompleteMultipartUpload, s3:AbortMultipartUpload (required by the transfer manager for multipart uploads), plus kms:GenerateDataKey and kms:Decrypt if using SSE-KMS.

Whether we extend the existing HO identity or create a dedicated one for etcd backups is a decision for CNTRLPLANE-2678.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore upload could use the managed identity that is attached to HO

@tony-schndr @jparrill there is no such managed identity today, is that correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for the etcd backup job in ARO HCP, we'd need to create a new managed identity with Storage Blob Data Contributor role + a SecretProviderClass to mount the credential, following the same pattern as we do with CPO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tony-schndr WDTY?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, I don't see a problem with adding the managed identity.

SubscriptionID string `json:"subscriptionId"`
TenantID string `json:"tenantId"`
ClientID string `json:"clientId"`
ClientSecret string `json:"clientSecret"`
}

// NewAzureBlobUploader creates a new AzureBlobUploader.
// authType controls how credentials are loaded:
// - "client-secret": reads a JSON file with clientId/clientSecret/tenantId (default)
// - "managed-identity": uses msi-dataplane with a certificate file from CSI mount (ARO HCP)
//
// If credentialsFile is empty, falls back to DefaultAzureCredential regardless of authType.
func NewAzureBlobUploader(ctx context.Context, container, storageAccount, credentialsFile, encryptionScope, authType string) (*AzureBlobUploader, error) {
if container == "" {
return nil, fmt.Errorf("--container is required for AzureBlob storage type")
}
if storageAccount == "" {
return nil, fmt.Errorf("--storage-account is required for AzureBlob storage type")
}

cred, err := newAzureCredential(ctx, credentialsFile, authType)
if err != nil {
return nil, fmt.Errorf("failed to load Azure credentials: %w", err)
}

serviceURL := fmt.Sprintf("https://%s.blob.core.windows.net", storageAccount)
client, err := azblob.NewClient(serviceURL, cred, nil)
if err != nil {
return nil, fmt.Errorf("failed to create Azure Blob client: %w", err)
}

return &AzureBlobUploader{
container: container,
storageAccount: storageAccount,
encryptionScope: encryptionScope,
client: client,
}, nil
}

// Upload uploads a snapshot file to Azure Blob Storage with conditional write and optional CMK encryption.
func (u *AzureBlobUploader) Upload(ctx context.Context, snapshotPath string, key string) (*UploadResult, error) {
f, err := os.Open(snapshotPath)
if err != nil {
return nil, fmt.Errorf("failed to open snapshot file %q: %w", snapshotPath, err)
}
defer f.Close()

opts := &azblob.UploadFileOptions{
BlockSize: 4 * 1024 * 1024, // 4 MiB blocks
AccessConditions: &blob.AccessConditions{
ModifiedAccessConditions: &blob.ModifiedAccessConditions{
IfNoneMatch: etagPtr(azcore.ETagAny),
},
},
}

if u.encryptionScope != "" {
opts.CPKScopeInfo = &blob.CPKScopeInfo{
EncryptionScope: &u.encryptionScope,
}
}

if _, err := u.client.UploadFile(ctx, u.container, key, f, opts); err != nil {
return nil, fmt.Errorf("failed to upload to Azure Blob %s/%s: %w", u.container, key, err)
}

url := fmt.Sprintf("https://%s.blob.core.windows.net/%s/%s", u.storageAccount, u.container, key)
return &UploadResult{URL: url}, nil
}

// newAzureCredential returns a TokenCredential based on the provided credentials file and auth type.
// If credentialsFile is empty, it uses DefaultAzureCredential regardless of authType.
// If credentialsFile is provided:
// - authType "client-secret": reads a JSON file with clientId/clientSecret/tenantId
// - authType "managed-identity": uses msi-dataplane to load a certificate credential (ARO HCP)
func newAzureCredential(ctx context.Context, credentialsFile, authType string) (azcore.TokenCredential, error) {
if credentialsFile == "" {
cred, err := azidentity.NewDefaultAzureCredential(nil)
if err != nil {
return nil, fmt.Errorf("failed to create default Azure credential: %w", err)
}
return cred, nil
}

switch authType {
case AuthTypeManagedIdentity:
return newManagedIdentityCredential(ctx, credentialsFile)
case AuthTypeClientSecret, "":
return newClientSecretCredential(credentialsFile)
default:
return nil, fmt.Errorf("unsupported auth type: %q (must be %q or %q)", authType, AuthTypeClientSecret, AuthTypeManagedIdentity)
}
}

// newClientSecretCredential reads a JSON file with clientId/clientSecret/tenantId
// and returns a ClientSecretCredential.
func newClientSecretCredential(credentialsFile string) (azcore.TokenCredential, error) {
data, err := os.ReadFile(credentialsFile)
if err != nil {
return nil, fmt.Errorf("failed to read credentials file %q: %w", credentialsFile, err)
}

var creds azureCredentialsFile
if err := json.Unmarshal(data, &creds); err != nil {
return nil, fmt.Errorf("failed to parse credentials file: %w", err)
}

credential, err := azidentity.NewClientSecretCredential(creds.TenantID, creds.ClientID, creds.ClientSecret, nil)
if err != nil {
return nil, fmt.Errorf("failed to create Azure credential: %w", err)
}

return credential, nil
}

// newManagedIdentityCredential loads a UserAssignedIdentityCredentials file
// (certificate-based, mounted via CSI SecretProviderClass) and returns
// a TokenCredential using the msi-dataplane library. This is the auth path
// used by ARO HCP.
func newManagedIdentityCredential(ctx context.Context, credentialsFile string) (azcore.TokenCredential, error) {
cred, err := dataplane.NewUserAssignedIdentityCredential(ctx, credentialsFile)
if err != nil {
return nil, fmt.Errorf("failed to create managed identity credential from %q: %w", credentialsFile, err)
}
return cred, nil
}

func etagPtr(e azcore.ETag) *azcore.ETag {
return &e
}

// newAzureBlobUploaderWithClient creates an AzureBlobUploader with a provided client (for testing).
func newAzureBlobUploaderWithClient(container, storageAccount, encryptionScope string, client AzureBlobUploadAPI) *AzureBlobUploader {
return &AzureBlobUploader{
container: container,
storageAccount: storageAccount,
encryptionScope: encryptionScope,
client: client,
}
}
171 changes: 171 additions & 0 deletions etcd-upload/azure_uploader_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
package etcdupload

import (
"context"
"encoding/json"
"fmt"
"os"
"path/filepath"
"testing"

. "github.com/onsi/gomega"

"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
)

type mockAzureBlobClient struct {
uploadFileFn func(ctx context.Context, containerName string, blobName string, file *os.File, o *azblob.UploadFileOptions) (azblob.UploadFileResponse, error)
lastOpts *azblob.UploadFileOptions
}

func (m *mockAzureBlobClient) UploadFile(ctx context.Context, containerName string, blobName string, file *os.File, o *azblob.UploadFileOptions) (azblob.UploadFileResponse, error) {
m.lastOpts = o
if m.uploadFileFn != nil {
return m.uploadFileFn(ctx, containerName, blobName, file, o)
}
return azblob.UploadFileResponse{}, nil
}

func TestAzureBlobUploader(t *testing.T) {
t.Run("When uploading successfully it should return the correct Azure Blob URL", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{}
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", "", mock)
snapshotPath := createTempSnapshot(t)

result, err := uploader.Upload(context.Background(), snapshotPath, "backups/12345.db")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(result.URL).To(Equal("https://mystorageaccount.blob.core.windows.net/my-container/backups/12345.db"))
})

t.Run("When uploading it should set IfNoneMatch for conditional write", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{}
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", "", mock)
snapshotPath := createTempSnapshot(t)

_, err := uploader.Upload(context.Background(), snapshotPath, "backups/12345.db")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(mock.lastOpts.AccessConditions).ToNot(BeNil())
g.Expect(mock.lastOpts.AccessConditions.ModifiedAccessConditions).ToNot(BeNil())
g.Expect(mock.lastOpts.AccessConditions.ModifiedAccessConditions.IfNoneMatch).ToNot(BeNil())
})

t.Run("When encryption scope is provided it should set CPKScopeInfo", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{}
encryptionScope := "my-encryption-scope"
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", encryptionScope, mock)
snapshotPath := createTempSnapshot(t)

_, err := uploader.Upload(context.Background(), snapshotPath, "backups/12345.db")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(mock.lastOpts.CPKScopeInfo).ToNot(BeNil())
g.Expect(mock.lastOpts.CPKScopeInfo.EncryptionScope).ToNot(BeNil())
g.Expect(*mock.lastOpts.CPKScopeInfo.EncryptionScope).To(Equal(encryptionScope))
})

t.Run("When no encryption scope is provided it should not set CPKScopeInfo", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{}
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", "", mock)
snapshotPath := createTempSnapshot(t)

_, err := uploader.Upload(context.Background(), snapshotPath, "backups/12345.db")
g.Expect(err).ToNot(HaveOccurred())
g.Expect(mock.lastOpts.CPKScopeInfo).To(BeNil())
})

t.Run("When blob already exists it should return condition not met error", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{
uploadFileFn: func(ctx context.Context, containerName string, blobName string, file *os.File, o *azblob.UploadFileOptions) (azblob.UploadFileResponse, error) {
return azblob.UploadFileResponse{}, fmt.Errorf("ConditionNotMet: The condition specified using HTTP conditional header(s) is not met")
},
}
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", "", mock)
snapshotPath := createTempSnapshot(t)

_, err := uploader.Upload(context.Background(), snapshotPath, "backups/12345.db")
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("ConditionNotMet"))
})

t.Run("When snapshot file does not exist it should return an error", func(t *testing.T) {
g := NewGomegaWithT(t)
mock := &mockAzureBlobClient{}
uploader := newAzureBlobUploaderWithClient("my-container", "mystorageaccount", "", mock)

_, err := uploader.Upload(context.Background(), "/nonexistent/snapshot.db", "backups/12345.db")
g.Expect(err).To(HaveOccurred())
})
}

func TestAzureCredential(t *testing.T) {
t.Run("When auth type is managed-identity it should attempt to load msi-dataplane credential", func(t *testing.T) {
g := NewGomegaWithT(t)

// Create a file with valid JSON structure but invalid certificate data,
// simulating what a CSI SecretProviderClass would mount from Azure Key Vault.
fakeCredentials := map[string]string{
"authenticationEndpoint": "https://login.microsoftonline.com",
"clientId": "fake-client-id",
"tenantId": "fake-tenant-id",
"clientSecret": "bm90LWEtdmFsaWQtY2VydA==", // base64("not-a-valid-cert")
"notBefore": "2026-01-01T00:00:00Z",
"notAfter": "2027-01-01T00:00:00Z",
}
data, err := json.Marshal(fakeCredentials)
g.Expect(err).ToNot(HaveOccurred())

credFile := filepath.Join(t.TempDir(), "managed-identity-creds.json")
g.Expect(os.WriteFile(credFile, data, 0644)).To(Succeed())

// msi-dataplane will parse the JSON but fail on the invalid certificate.
// This proves the managed-identity path is reached and the file is consumed.
_, err = newAzureCredential(context.Background(), credFile, AuthTypeManagedIdentity)
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("managed identity credential"))
})

t.Run("When auth type is managed-identity and credentials file is missing it should return an error", func(t *testing.T) {
g := NewGomegaWithT(t)

_, err := newAzureCredential(context.Background(), "/nonexistent/creds.json", AuthTypeManagedIdentity)
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("managed identity credential"))
})

t.Run("When auth type is managed-identity but credentials file is empty it should fall back to DefaultAzureCredential", func(t *testing.T) {
// When no credentials file is provided, authType is ignored and
// DefaultAzureCredential is used. This matches the behavior where
// the controller doesn't pass --credentials-file.
_, err := newAzureCredential(context.Background(), "", AuthTypeManagedIdentity)
// DefaultAzureCredential will fail in a test environment (no Azure identity),
// but the error should NOT mention "managed identity credential".
if err != nil {
g := NewGomegaWithT(t)
g.Expect(err.Error()).To(ContainSubstring("default Azure credential"))
}
})

t.Run("When auth type is client-secret it should parse client secret JSON", func(t *testing.T) {
g := NewGomegaWithT(t)

creds := azureCredentialsFile{
SubscriptionID: "sub-id",
TenantID: "tenant-id",
ClientID: "client-id",
ClientSecret: "client-secret",
}
data, err := json.Marshal(creds)
g.Expect(err).ToNot(HaveOccurred())

credFile := filepath.Join(t.TempDir(), "client-secret-creds.json")
g.Expect(os.WriteFile(credFile, data, 0644)).To(Succeed())

credential, err := newAzureCredential(context.Background(), credFile, AuthTypeClientSecret)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(credential).ToNot(BeNil())
})
}
Loading
Loading