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: set DisableKubeletCloudCredentialProviders=false for versions without OOT credential provider (1.29) #456

Merged
merged 6 commits into from
Aug 22, 2024
Merged

fix: set DisableKubeletCloudCredentialProviders=false for versions without OOT credential provider (1.29) #456

merged 6 commits into from
Aug 22, 2024

Conversation

Bryce-Soghigian
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian commented Aug 14, 2024 •
edited
Loading

Fixes #411

Description
https://github.com/Azure/karpenter-provider-azure/blob/main/pkg/providers/imagefamily/bootstrap/aksbootstrap.go#L425

// CredentialProviderURL returns the URL for OOT credential provider,
// or an empty string if OOT provider is not to be used
func CredentialProviderURL(kubernetesVersion, arch string) string {
  minorVersion := semver.MustParse(kubernetesVersion).Minor
  if minorVersion < 30 { 
    return ""
  }

Looks like from this code for out of tree provider we default to not including the settings for out of tree provider if CredentialProviderURL is empty.

DisableKubeletCloudCredentialProviders false Alpha 1.23 1.28
DisableKubeletCloudCredentialProviders true Beta 1.29 –

We don't have the logic conditionally enabled for 1.29, so auth pull will not work for that specific kubernetes version.

This can be easily fixed by

A) Switching 1.29 to use out of tree credential provider.(Have karpenter pass in the rest of the required OOT Provider kubelet flags.)
B) Adding the default to false for the feature gate for 1.29 clusters.

To have consistency with the provisioning of the AKS Resource Provider nodes, we will choose option B.

How was this change tested?

  • make az-all
  • make presubmit
  • az e2etest acr

Does this change impact docs?

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

Release Note

fixes acr pull on 1.29

@Bryce-Soghigian Bryce-Soghigian changed the title fix: disabling kubelet flag DisableKubeletCloudCredentialProviders for versions without OOT credential provider fix: disabling kubelet flag DisableKubeletCloudCredentialProviders for versions without OOT credential provider(1.29) Aug 14, 2024
@coveralls
Copy link

coveralls commented Aug 14, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10502765047

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 97.697%
Files with Coverage Reduction New Missed Lines %
pkg/cache/unavailableofferings.go 2 95.45%
Totals Coverage Status
Change from base Build 10502691706: -0.005%
Covered Lines: 36311
Relevant Lines: 37167
💛 - Coveralls

@Bryce-Soghigian Bryce-Soghigian marked this pull request as ready for review August 14, 2024 02:22
@Bryce-Soghigian
Copy link
Collaborator Author

~/dev/focus/karpenter-provider-azure (bsoghigian/disable-credential-providers*) » k get nodes sillygoose@Bryces-MacBook-Pro
NAME STATUS ROLES AGE VERSION
aks-nodepool1-34775126-vmss000000 Ready 14h v1.29.0
aks-nodepool1-34775126-vmss000001 Ready 14h v1.29.0
aks-nodepool1-34775126-vmss000002 Ready 14h v1.29.0

< Exit [AfterEach] TOP-LEVEL - /Users/sillygoose/dev/focus/karpenter-provider-azure/test/suites/acr/suite_test.go:43 @ 08/14/24 09:35:22.275 (41ms)
• [357.830 seconds]

Ran 1 of 1 Specs in 358.025 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAcr (358.03s)
PASS
ok github.com/Azure/karpenter-provider-azure/test/suites/acr 358.562s
kubectl taint nodes CriticalAddonsOnly=true:NoSchedule- --all
node/aks-nodepool1-34775126-vmss000000 untainted
node/aks-nodepool1-34775126-vmss000001 untainted
node/aks-nodepool1-34775126-vmss000002 untainted

Here is the proof of karpenter e2e successfully passing for these scenarios on a 1.29 cluster. One thing to check is just double check that az-import doesnt also inherit other traits of the image(possibly importing the pause container image for the acr image pull test may also inherit unauthorized pulls or something?)

@tallaxes tallaxes changed the title fix: disabling kubelet flag DisableKubeletCloudCredentialProviders for versions without OOT credential provider(1.29) fix: set DisableKubeletCloudCredentialProviders=false for versions without OOT credential provider (1.29) Aug 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 Aug 16, 2024
@Bryce-Soghigian Bryce-Soghigian merged commit a43b994 into main Aug 22, 2024
10 of 11 checks passed
@Bryce-Soghigian Bryce-Soghigian deleted the bsoghigian/disable-credential-providers branch August 22, 2024 06:29
Bryce-Soghigian added a commit that referenced this pull request Sep 12, 2024
@Bryce-Soghigian @tallaxes
fix: set DisableKubeletCloudCredentialProviders=false for versions …
1960e65 
…without OOT credential provider (1.29) (#456)

* fix: DisableKubeletCloudCredentialProviders should default to false for 1.29

* fix: adding 1.31 credential provider into switch

* test: testing 1.31 credential provider url

* fix: ci

---------

Co-authored-by: Alex Leites <18728999+tallaxes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallaxes tallaxes tallaxes approved these changes

Assignees

@Bryce-Soghigian Bryce-Soghigian

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
3 participants
@Bryce-Soghigian @coveralls @tallaxes