Skip to content
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

Add native Azure Blob support #598

Merged
merged 10 commits into from
Mar 10, 2022
Merged

Add native Azure Blob support #598

merged 10 commits into from
Mar 10, 2022

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Mar 1, 2022

This PR introduces an Azure Blob BucketProvider implementation,
capable of fetching objects from public and private "container"
buckets.

The supported credential types are:

  • ClientSecret with tenantId, clientId and clientSecret Secret
    data fields.
  • ClientCertificate with tenantId, clientId and clientSecret Secret
    data (with optionally clientCertificatePassword).
  • ManagedIdentity with a clientId Secret data field.
  • SharedKey with accountKey Secret data field, the account name is
    extracted from the endpoint URL specified on the object.
  • ChainedToken with:
    • Environment credentials
    • ManagedIdentity with AZURE_CLIENT_ID environment variable
      (when available)
    • ManagedIdentity with defaults

If no Secret is provided or the chain can not be established, the
Bucket is assumed to be public.

Successor of #513

// - azblob.SharedKeyCredential when a "accountName" and "accountKey" are
// found.
// - Client without credentials.
func buildServiceClient(obj *sourcev1.Bucket, secret *corev1.Secret) (_ azblob.ServiceClient, err error) {
Copy link
Member Author

@hiddeco hiddeco Mar 1, 2022

Choose a reason for hiding this comment

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

@laozc can you please take a look and see if this does not cover anything your version did?

Reason for this change is that I had a look at the current Azure libraries out there, and this seems to be the one with the brightest future and maintenance expectations. Given this is a new dependency introduction, this has my preference over one we eventually need to replace (or semi-copy/borrow from another project).

Copy link
Contributor

@laozc laozc Mar 3, 2022

Choose a reason for hiding this comment

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

Hi @hiddeco

I'm still verifying this PR in Azure environments.
I think it would be better to use the following order during authentication

  1. Check for client secret first to use Service Principal password login
  2. Check for client certificate to use Service Principal certificate login
  3. Use Client ID with Managed Identity login
  4. Use Resource ID with Managed Identity login
  5. Use Storage Account key based auth login
  6. No auth

This may ensure it works as expected when multiple Managed Identites bound on the same VM node for 3) and 4).

func tokenCredentialFromSecret(secret *corev1.Secret) (azcore.TokenCredential, error) {
	tenantID, hasTenantID := secret.Data[tenantIDField]
	clientID, hasClientID := secret.Data[clientIDField]
	clientSecret, hasClientSecret := secret.Data[clientSecretField]
	clientCertificate, hasClientCertificate := secret.Data[clientCertificateField]
	clientCertificatePassword, _ := secret.Data[clientCertificatePasswordField]
	resourceID, hasResourceID := secret.Data[resourceIDField]

	if hasTenantID && hasClientID {
		if hasClientSecret && string(clientSecret) != "" {
			return azidentity.NewClientSecretCredential(string(tenantID), string(clientID), string(clientSecret), nil)
		}
		if hasClientCertificate && string(clientCertificate) != "" {
			certs, key, err := azidentity.ParseCertificates(clientCertificate, clientCertificatePassword)
			if err != nil {
				return nil, fmt.Errorf("failed to parse client certificates: %w", err)
			}
			return azidentity.NewClientCertificateCredential(string(tenantID), string(clientID), certs, key, nil)
		}
	}

	if hasClientID {
		return azidentity.NewManagedIdentityCredential(&azidentity.ManagedIdentityCredentialOptions{
			ID: azidentity.ClientID(clientID)})
	} else if hasResourceID {
		return azidentity.NewManagedIdentityCredential(&azidentity.ManagedIdentityCredentialOptions{
			ID: azidentity.ResourceID(resourceID)})
	}

	return nil, nil
}

Let me get back to you when I have all these scenarios verified.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up, I will take your suggestions into account and document the reasoning in-code. Please let me know if anything else pops up, and I'll try to address it swiftly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the code to your suggestion, you may also want to provide some input on #598 (review) and #598 (comment).

@hiddeco hiddeco added area/bucket Bucket related issues and pull requests enhancement New feature or request labels Mar 1, 2022
pkg/azure/blob.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 13 times, most recently from 8920396 to c8f22b4 Compare March 3, 2022 11:51
Co-authored-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 3 times, most recently from ccfb4db to 4434365 Compare March 3, 2022 12:10
hiddeco and others added 3 commits March 3, 2022 13:20
This commit introduces an Azure Blob BucketProvider implementation,
capable of fetching from objects from public and private "container"
buckets.

The supported credential types are:

- ManagedIdentity with a `resourceId` Secret data field.
- ManagedIdentity with a `clientId` Secret data field.
- ClientSecret with `tenantId`, `clientId` and `clientSecret` Secret
  data fields.
- SharedKey with `accountKey` Secret data field, the Account Name is
  extracted from the endpoint URL specified on the object.

If no Secret is provided, the Bucket is assumed to be public.

Co-authored-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit allows for a Secret to be configured with `tenantId`,
`clientId` and `clientCertificate` data fields (with optionally
`clientCertificatePassword`) to authenticate using TLS.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 2 times, most recently from 179aa95 to 44a166e Compare March 3, 2022 12:50
Tests are configured in such a way that they only run for `main`.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch from 44a166e to d55a759 Compare March 3, 2022 13:03
@hiddeco
Copy link
Member Author

hiddeco commented Mar 3, 2022

@hiddeco hiddeco marked this pull request as ready for review March 3, 2022 13:04
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 2 times, most recently from 7940044 to 696dcc7 Compare March 4, 2022 15:00
@hiddeco
Copy link
Member Author

hiddeco commented Mar 4, 2022

@laozc please see the current state of the PR and let me know if this is acceptable. I will work on updating the tests in the meantime.

@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 5 times, most recently from f3eb2fd to 166e185 Compare March 4, 2022 17:38
pkg/azure/blob.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 2 times, most recently from eca70e4 to 7bc42a5 Compare March 8, 2022 09:17
@hiddeco hiddeco requested a review from stefanprodan March 8, 2022 10:10
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch 2 times, most recently from 1346696 to ce4e108 Compare March 8, 2022 10:46
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @hiddeco and @laozc

PS> We'll need a followup in #599 with Azure specific docs

@hiddeco
Copy link
Member Author

hiddeco commented Mar 8, 2022

@stefanprodan documented in 687af2f

pkg/azure/blob.go Outdated Show resolved Hide resolved
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

hiddeco added 3 commits March 8, 2022 14:48
- Use octal syntax for permissions.
- Fix typo.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Based on recommendations from Microsoft, change the order valid
authentication options are taken into account. Mainly to ensure it works
as expected when multiple Managed Identities are bound on the same VM
node.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This supports the fields as documented in the AKS documentation:
https://docs.microsoft.com/en-us/azure/aks/kubernetes-service-principal?tabs=azure-cli#manually-create-a-service-principal

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch from ce4e108 to 61f756f Compare March 8, 2022 13:57
hiddeco added 2 commits March 8, 2022 14:57
- `authorityHost` and `clientCertificateSendChain` can now be set where
  applicable.
- AZ CLI fields have been removed.
- Fallback to `ChainedTokenCredential` with `EnvironmentCredential` and
  `ManagedIdentityCredential` with defaults if no Secret is given.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This ensures the Managed Identity authentication works with multiple
identities assigned to a single node.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the azure-blob-bucket-provider branch from 61f756f to ccb65c7 Compare March 8, 2022 13:58
@hiddeco
Copy link
Member Author

hiddeco commented Mar 9, 2022

@laozc are you still running tests, or do you think this can be merged as is?

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM

Really good stuff @hiddeco @laozc!

@laozc
Copy link
Contributor

laozc commented Mar 10, 2022

@hiddeco Please go head with the merge.
It's great to see the support landed in flux.

@hiddeco hiddeco merged commit ccadce6 into main Mar 10, 2022
@hiddeco hiddeco deleted the azure-blob-bucket-provider branch March 10, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bucket Bucket related issues and pull requests enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants