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

Allow Kubelet to run with no Azure identity #77906

Merged
merged 1 commit into from May 16, 2019

Conversation

@feiskyer
Copy link
Member

commented May 15, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

/kind bug
/sig azure

What this PR does / why we need it:

Allow Kubelet to run with no Azure identity. useInstanceMetadata should be enabled and Kubelet would use IMDS to get node's information.

Which issue(s) this PR fixes:

Fixes #77309

Special notes for your reviewer:

#77851 is also required for no identity working.

Does this PR introduce a user-facing change?:

Kubelet could be run with no Azure identity now. A sample cloud provider configure is:  `{"vmType": "vmss", "useInstanceMetadata": true, "subscriptionId": "<subscriptionId>"}`
Allow Kubelet to run with no Azure identity
useInstanceMetadata should be enabled and Kubelet would use IMDS to get
node's information.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer

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

The pull request process is described 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

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

/priority important-soon

/hold

@andyzhangx When validating this with #77851, I find AzureDisk is not working as expected. The error message is:

Operation for "\"kubernetes.io/azure-disk//<pvc-resource-id>\"" failed. No retries permitted until 2019-05-15 07:37:44.323108693 +0000 UTC m=+297.614031734 (durationBeforeRetry 1m4s). Error: "MountVolume.WaitForAttach failed for volume \"pvc-dceb8b43-76e3-11e9-a1ab-000d3aa2e18b\" (UniqueName: \"kubernetes.io/azure-disk//<pvc-resource-id>\") pod \"sh\" (UID: \"dcd1c6b5-76e3-11e9-a1ab-000d3aa2e18b\") : azureDisk - WaitForAttach failed within timeout node (k8s-agentpool2-20421826-vmss000000) diskId:(k8s-vmss-dynamic-pvc-dceb8b43-76e3-11e9-a1ab-000d3aa2e18b) lun:(-1)"

As the error message indicated, the lun is -1, which is weird. Do you think this is related to #77483?

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

The above issue is fixed by #77912.

/hold cancel

if metadata.Compute.VMSize != "" {
return metadata.Compute.VMSize, nil
if !isLocalInstance {
if az.vmSet != nil {

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx May 15, 2019

Member

why do this code change? not use metadata.Compute.VMSize, nil directly?

This comment has been minimized.

Copy link
@feiskyer

feiskyer May 15, 2019

Author Member

when this is not local instance, and credentials not provided, we should report errors.

the original logic has been moved to L331.

@neolit123
Copy link
Member

left a comment

@feiskyer the change LGTM
/lgtm
/hold

but there is a good chance the release note with { .. } might break anago (the release tool)
possibly trim it to?

Kubelet could be run with no Azure identity now.

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Thanks, let me remove the {} and wrap the example in quotes ``.

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@neolit123 looked at the old release notes, {} is allowed within ```` quota, so changed the example within `` now.

@neolit123

This comment has been minimized.

Copy link
Member

commented May 16, 2019

ok, understood.
/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 5a233cc into kubernetes:master May 16, 2019

20 checks passed

cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

Provider Azure automation moved this from In progress to Done May 16, 2019

@feiskyer feiskyer deleted the feiskyer:az-no-authz branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.