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

Default Azure subscription ID if not provided using instance metadata #4708

Closed
wants to merge 1 commit into from
Closed

Default Azure subscription ID if not provided using instance metadata #4708

wants to merge 1 commit into from

Conversation

whites11
Copy link

@whites11 whites11 commented Feb 23, 2022

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind bug

What this PR does / why we need it:

In CA version 1.21, on Azure, it was possible to omit the Subscription ID completely.
On startup, cluster autoscaler used to detect it using Azure Instance Metadata Service (IMDS).

It used to do so using a service in legacy-cloud-providers.

The initialization code for such service is as follows:

metadataService, err := providerazure.NewInstanceMetadataService(metadataURL)

In 1.21 this worked just fine.
In 1.22 this behaviour was broken, and then fixed with this PR (still unreleased, :()

In 1.23/master the feature was removed completely.
This PR aims at adding it back.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Restore possibility to use IMDS with managed identities to authenticate cluster-autoscaler with Azure API.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: whites11
To complete the pull request process, please assign marwanad after the PR has been reviewed.
You can assign the PR to them by writing /assign @marwanad in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -22,6 +22,7 @@ import (
"fmt"
"io"
"io/ioutil"
"k8s.io/legacy-cloud-providers/azure/cache"
Copy link
Member

Choose a reason for hiding this comment

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

we probably would want to avoid referencing the legacy-cloud-provider path.

There's another PR in flight here: #4689 so if it addresses the issue, we can close this one in favor of the other one.

Copy link
Author

Choose a reason for hiding this comment

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

Oh I missed that one! Good, I'll close mine then.
Thanks

@whites11 whites11 closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants