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

fix(bootstrap): OOT credential provider support in 1.30+ #429

Merged
merged 10 commits into from
Jul 17, 2024
Merged

fix(bootstrap): OOT credential provider support in 1.30+ #429

merged 10 commits into from
Jul 17, 2024

Conversation

Bryce-Soghigian
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian commented Jul 10, 2024 •
edited by tallaxes
Loading

Description

This fixes OOT credential provider support in AKS 1.30+:

  • Uses correct download URL
  • Pins down provider patch version
  • Respects machine architecture

OOT credential provider should be supported in 1.29 as well, but this PR opts to not enable it just yet. We are likely to enable it some time later, after some more testing.

How was this change tested?

  • make presumbit
  • Local manual testing of bootstrap and image pull on 1.30.0; another test with faked detection of 1.30.2
  • Retested on 1.28.9
  • Local E2E Utilization tests (passed)
  • CI E2E tests (most passed, Integration passed after retry, need to review for flakiness)

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

Bryce-Soghigian
Bryce-Soghigian commented Jul 10, 2024
View reviewed changes
Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

/test

@coveralls
Copy link

coveralls commented Jul 16, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9969045472

Details

  • 13 of 16 (81.25%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 97.696%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/providers/imagefamily/bootstrap/aksbootstrap.go 12 15 80.0%
Files with Coverage Reduction New Missed Lines %
pkg/cache/unavailableofferings.go 2 95.45%
Totals Coverage Status
Change from base Build 9966791669: -0.01%
Covered Lines: 36292
Relevant Lines: 37148
💛 - Coveralls

@tallaxes tallaxes changed the title test: 1.30.0 fix: OOT credential provider support in 1.30+ Jul 16, 2024
@tallaxes tallaxes added kind/bug Categorizes issue or PR as related to a bug. area/bootstrap Issues or PRs related to bootstrap labels Jul 16, 2024
@tallaxes tallaxes self-assigned this Jul 16, 2024
tallaxes
tallaxes reviewed Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

/test

@tallaxes tallaxes assigned charliedmcb and unassigned charliedmcb Jul 16, 2024
@tallaxes
Copy link
Collaborator

@Bryce-Soghigian, please take a look as well, can't add you as reviewer since you initiated the PR

Bryce-Soghigian
Bryce-Soghigian commented Jul 16, 2024
View reviewed changes
Copy link
Collaborator Author

@Bryce-Soghigian Bryce-Soghigian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, I don't love how manual adding new selection for the binary URL is, but I can't think of a better way thats simpler.

charliedmcb
charliedmcb approved these changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@charliedmcb charliedmcb left a comment

Choose a reason for hiding this comment

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

Approved. However, have a few questions/comments.
 
In the PR description I just see how tested with "make presubmit". Feels worth testing as self hosted manually, or making sure we get successful e2e tests?

Also, left one clarify question on how the versioning actually works, and picking up of bug fixes/CVEs.

pkg/providers/imagefamily/bootstrap/aksbootstrap.go Show resolved Hide resolved
@tallaxes
Copy link
Collaborator

In the PR description I just see how tested with "make presubmit". Feels worth testing as self hosted manually, or making sure we get successful e2e tests?

Yes, doing more testing; will update the list

tallaxes
tallaxes reviewed Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

/test

@tallaxes tallaxes changed the title fix: OOT credential provider support in 1.30+ fix(bootstrap): OOT credential provider support in 1.30+ Jul 17, 2024
tallaxes
tallaxes reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

/test

@tallaxes
Copy link
Collaborator

Most E2E tests passing in CI as well (intergration passed after retry, need to review for flakiness): https://github.com/Azure/karpenter-provider-azure/actions/runs/9966510663

@Bryce-Soghigian
Copy link
Collaborator Author

Lets get this in then?

@tallaxes tallaxes merged commit 46b4276 into main Jul 17, 2024
9 of 10 checks passed
@tallaxes tallaxes deleted the bsoghigian/chore-adding-missed-cherry-pick branch July 17, 2024 18:11
Bryce-Soghigian added a commit that referenced this pull request Sep 12, 2024
@Bryce-Soghigian @tallaxes
fix(bootstrap): OOT credential provider support in 1.30+ (#429)
9c31f06 
* test: 1.30.0

* fix: credential provider URL

* test: associated tests

* chore: remove unused function

* fix: minor unrelated log fix

* chore: undo AKS version choice

---------

Co-authored-by: tallaxes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallaxes tallaxes tallaxes left review comments

@charliedmcb charliedmcb charliedmcb approved these changes

Assignees

@tallaxes tallaxes

Labels
area/bootstrap Issues or PRs related to bootstrap kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bryce-Soghigian @coveralls @tallaxes @charliedmcb