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

port to new azure SDK #532

Merged
merged 8 commits into from
Jul 2, 2024
Merged

port to new azure SDK #532

merged 8 commits into from
Jul 2, 2024

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented May 14, 2024

  • use new Azure SDK
  • drop support for azure credentials and azure profiles JSON files
  • drop obsolete commands in ore (for image replication, unreplication and sharing)
  • deduplicate ore azure commands for image creation and blob uploading
  • drop flags for azure auth (azure credentials location, azure profile location and use identity)
  • drop the use of storage shared keys, storage accounts are created without the keys now

--

  • Changelog entries added in the respective changelog/ directory (user-facing change, bug fix, security fix, update)

@krnowak krnowak force-pushed the krnowak/azure-new-stuff branch 3 times, most recently from 66d4786 to ad42e53 Compare June 19, 2024 15:08
@krnowak krnowak force-pushed the krnowak/azure-new-stuff branch 3 times, most recently from cb8a773 to 9f85c83 Compare June 25, 2024 13:07
@jepio
Copy link
Member

jepio commented Jun 27, 2024

Github really fails to handle this many changes :/

I wanted to leave a comment: can you modify the storage account creation AccountCreateParameters to directly deploy AllowSharedKeyAccess: false.

@krnowak krnowak force-pushed the krnowak/azure-new-stuff branch from 9f85c83 to 481ec57 Compare June 27, 2024 11:17
krnowak added 7 commits June 27, 2024 14:42
This commit replaces old resource clients with their counterparts from
the new SDK. Most of the changes made were rather straightforward. The
larger changes were made in storage account handling and blob
uploading. We stopped generating shared keys for storage accounts, so
for uploading we expect the authenticated user to have an RBAC
permission that allows creating containers and upload blobs already
assigned. This can be done by, for example, assigning "Storage Blob
Data Owner" role at a subscription level. Uploading the blobs is now
delegated to the azure-vhd-utils package.

Another thing that has changed is authentication to Azure - the whole
thing is replaced with a default authentication method provided by the
azidentity package. So support for azure profile and azure credentials
JSON files is dropped. More about the default authentication method
can be read on azidentity documentation website:

https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity#section-readme

Some functionality that was unused and was difficult to port (image
sharing, replication and unreplication, some OS image stuff) was
dropped.
The obsolete options were related to authentication.
The obsolete options were mostly about authentication. The obsolete
commands were mostly about dropped functionality from Azure API (image
sharing, replication and unreplication). Commands for creating images
and uploading blobs are deduplicated. The code needed also some
adaptation to the API changes.
Obsolete options were mostly about authentication. The adaptations
were mostly about dropping support for storage keys and some of the
API being changed.
@krnowak krnowak force-pushed the krnowak/azure-new-stuff branch from 481ec57 to 0bc5aaf Compare June 27, 2024 12:48
@krnowak
Copy link
Member Author

krnowak commented Jun 27, 2024

Github really fails to handle this many changes :/

I have split the work into separate commits, maybe will be a tad easier to review and for github to display stuff. The whole vendor stuff is now in a separate commit too. I also dropped fiddling with RBAC, so that shrunk the vendored code too (not pulling the msgraph stuff any more).

I wanted to leave a comment: can you modify the storage account creation AccountCreateParameters to directly deploy AllowSharedKeyAccess: false.

Done.

@krnowak krnowak requested a review from a team June 27, 2024 13:02
@krnowak
Copy link
Member Author

krnowak commented Jun 27, 2024

Tests:

Will need small changes in scripts on all branches (to drop obsolete --azure-identity flags).


return false, nil
// log-level=NONE only affects the log file - stdout is unaffected
cmd := exec.Command(azcopy, "cp", "--blob-type=PageBlob", "--log-level=NONE", sourceURL, dstSAS)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we doing this? There is no lib for azcopy?

Copy link
Member

Choose a reason for hiding this comment

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

there is no lib for azcopy, and the the SDK CopyBlob function copies all 30GB of a sparse VHD instead of just the populated 1GB. But I think this code path is unused since we switched to the container pipeline (we don't copyblobs anymore, we always upload a vhd).

Copy link
Member Author

Choose a reason for hiding this comment

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

azcopy is doing similar thing to the tryCopyPageBlob function, but with goroutines and throttling and stuff. What I wasn't sure about is whether to keep the code that invokes azcopy.

@tormath1
Copy link
Contributor

It's a bit dense to review but if the CI is green it's already a good sign.

Will need small changes in scripts on all branches (to drop obsolete --azure-identity flags).

We don't need to forget about backporting this change to maintenance branches.

The dropped commands do not seem to be used in the Flatcar org.

@krnowak krnowak marked this pull request as ready for review June 28, 2024 08:55
@krnowak
Copy link
Member Author

krnowak commented Jun 28, 2024

It's a bit dense to review but if the CI is green it's already a good sign.

Yeah, sorry about that. It's hard to split it into more commits, though.

Will need small changes in scripts on all branches (to drop obsolete --azure-identity flags).

We don't need to forget about backporting this change to maintenance branches.

I have a PR (flatcar/scripts#2063) for it. I think I'll just amend the PRs that will be created in scripts repo after merging this PR.

The dropped commands do not seem to be used in the Flatcar org.

Right, I was asking about them on matrix some time ago.

@jepio
Copy link
Member

jepio commented Jul 2, 2024

can this be merged?

@tormath1
Copy link
Contributor

tormath1 commented Jul 2, 2024

@jepio yeah, as discussed I'm fine with merging in this state

@krnowak krnowak merged commit 3321b58 into flatcar-master Jul 2, 2024
2 checks passed
@krnowak krnowak deleted the krnowak/azure-new-stuff branch July 2, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants