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.

Sign up for GitHub

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

Jump to bottom

Switch to new Azure SDK and refactor Azure implementation #3

Merged
merged 1 commit into from
Aug 9, 2022
Merged

Switch to new Azure SDK and refactor Azure implementation #3

merged 1 commit into from
Aug 9, 2022

Conversation

phillebaba
Copy link
Contributor

@phillebaba phillebaba commented Mar 29, 2022 •
edited
Loading

This is a larger refactor of the Azure implementation which switches to the new Azure Go SDK. A lot of function names are the same its for the most part that interface that has changed to become more idiomatic Go. The main benefit of switching is that the new SDK does not require ADAL and instead fully supports azidentity which will remove some less than ideal code to get MSI working. I took the liberty to do some refactoring while I am at it in the hope that it will be easier for future developers to understand.

Fixes thanos-io/thanos/issues/5172

@phillebaba phillebaba marked this pull request as draft March 29, 2022 16:11
@phillebaba
Copy link
Contributor Author

Currently I need to fix the transport configuration in the new client to make use of tls certs etc. I will start testing my fork after that is completed. Having some sort of conformance tests would be nice but I may just have to resort to create a test tenant in my Thanos deployment.

The current Azure implementation works just fine so it is important that we don't break any existing features. Would appreciate any help with testing the changes.

@metalmatze
Copy link
Contributor

Please make sure to signoff your commits with --signoff. To fix the current one use: git rebase HEAD~1 --signoff. Thanks!

@phillebaba phillebaba marked this pull request as ready for review June 17, 2022 20:59
@phillebaba phillebaba mentioned this pull request Jun 17, 2022
Closed
2 tasks
@phillebaba
Copy link
Contributor Author

E2E tests now pass.

@phillebaba
Copy link
Contributor Author

Biggest issue right now is that MSI Resource and User Assigned ID is an either or choice.
https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#NewManagedIdentityCredential

Dont think this will affect functionality but it will affect the config validation.

@phillebaba phillebaba force-pushed the update/azure-sdk branch 3 times, most recently from c9346da to 73f94a2 Compare June 17, 2022 22:18
@phillebaba phillebaba mentioned this pull request Jul 4, 2022
Merged
2 tasks
@phillebaba
Copy link
Contributor Author

So I have verified that MSI resource no longer has to be set when configuring access to the storage account with Azure. It is automatically calculated and can not be configured.

The current blocker right now seems to be the requirement on Go 1.18 but there seems to be progress on that end also getting Thanos upgraded.

@bwplotka
Copy link
Member

bwplotka commented Jul 5, 2022

  • Do we have ticket for Go 1.18 upgrade somewhere we can link to track this?
  • Also we sill needs tests here, which is not trivial cc @kakkoyun who mentioned has some time for it

@kakkoyun
Copy link
Member

kakkoyun commented Jul 8, 2022

@phillebaba @bwplotka go1.18 is not strictly required for thanos-io/objstore. Where does this requirement come from?

@phillebaba
Copy link
Contributor Author

The requirement comes from the new Azure Go SDK. It requires Go 1.18, if i remember correctly it is because some authentication package uses generics.

@kakkoyun
Copy link
Member

kakkoyun commented Jul 8, 2022

@phillebaba Ok, got it. For whatever reason, I thought the other way around. Thanks for clarifying. I think after we release an initial version of this package, we can bump the required version and have these changes.

I'm working on the CI tests now for the repo.

@phillebaba phillebaba force-pushed the update/azure-sdk branch 11 times, most recently from 78a3c8f to 236cad3 Compare July 27, 2022 10:06
@phillebaba phillebaba force-pushed the update/azure-sdk branch 2 times, most recently from 41538f4 to 93d9bae Compare July 27, 2022 10:41
@phillebaba
Copy link
Contributor Author

@kakkoyun so I have now rebased this branch and fixed some of the unit tests. I have also deployed a build of this fork in a test environment and verified that things seems to still work. I just cant figure out why CodeQL is failing.

Looking through the changes it seems that there is no real breaking changes other than the msi_resource configuration. It cannot be set any more as the SDK will extrapolate this value. Removing the parameter will cause panics in existing Thanos deployments which set this value as it is no longer a configurable parameter.

On top of this I had to upgrade the Go version to 1.18 which can cause issues for both Thanos and other depending projects.

I think it would be good to start reviewing this PR.

@bwplotka
Copy link
Member

Thanks, I was just looking at this, will check.

The CodeQL might have problem with different Go version?

image

Retried just in case.

@phillebaba
Copy link
Contributor Author

phillebaba commented Jul 27, 2022 •
edited
Loading

@bwplotka I created #16 to upgrade to Go 1.18, might make this a bit easier to discuss and debug keeping the changes separate.

@phillebaba phillebaba mentioned this pull request Jul 27, 2022
Merged
@phillebaba
Copy link
Contributor Author

@bwplotka I fixed the issue. I needed to set the specific Go version for CodeQL.
github/codeql-action#1059 (comment)

bwplotka
bwplotka approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

💪🏽 Thanks, LGTM

I wish we had e2e tested this, but we don't have an Azure account for Thanos use I believe.

Ticket for this: #10

Happy to merge this after rebase (:

Makefile Outdated Show resolved Hide resolved
providers/azure/helpers.go Outdated Show resolved Hide resolved
@phillebaba phillebaba force-pushed the update/azure-sdk branch 2 times, most recently from c281d63 to 9387cdf Compare August 8, 2022 11:19
@phillebaba
Switch to new Azure SDK and refactor Azure implementation
022755a 
Signed-off-by: Philip Laine <philip.laine@gmail.com>
bwplotka
bwplotka approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing, thanks 💪🏽

And thanks for caring about compatibility, good job!

providers/azure/azure.go
Endpoint string `yaml:"endpoint"`
MaxRetries int `yaml:"max_retries"`
// Deprecated: Is automatically set by the Azure SDK.
MSIResource string `yaml:"msi_resource"`
Copy link
Member

Choose a reason for hiding this comment

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

I think we planned to log warning when someone provides it? Let's do that in next PR if needed (:

@bwplotka bwplotka merged commit 8ef1f21 into thanos-io:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

client for Azure should handle permission errors
4 participants
@phillebaba @metalmatze @bwplotka @kakkoyun