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 setting Azure cloud provider from Kubernetes secrets instread of local configure file #78242

Merged
merged 6 commits into from Jun 1, 2019

Conversation

@feiskyer
Copy link
Member

commented May 23, 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

What this PR does / why we need it:

Refer Azure/container-compute-upstream#27. This PR allows configure Azure cloud provider from Kubernetes secrets.

To support this, a new option cloudConfigType is added. Supported values are:

  • file: The cloud provider configuration is read from cloud-config file.
  • secret: the cloud provider configuration must be overridden by a secret.
  • merge: the cloud provider configuration can be optionally overridden by a secret when it is set explicitly in the secret, this is default value.

Note that the secret is a serialized version of azure.json file with key cloud-config. And the secret name is azure-cloud-provider.

Since Azure cloud provider would read Kubernetes secrets, the following RBAC should also be configured:

---
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRole
metadata:
  labels:
    kubernetes.io/cluster-service: "true"
  name: system:azure-cloud-provider-secret-getter
rules:
- apiGroups: [""]
  resources: ["secrets"]
  resourceNames: ["azure-cloud-provider"]
  verbs:
  - get
---
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRoleBinding
metadata:
  labels:
    kubernetes.io/cluster-service: "true"
  name: system:azure-cloud-provider-secret-getter
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: system:azure-cloud-provider-secret-getter
subjects:
- kind: ServiceAccount
  name: azure-cloud-provider
  namespace: kube-system

Which issue(s) this PR fixes:

Fixes Azure/container-compute-upstream#27

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Azure cloud provider could now be configured by Kubernetes secrets and a new option `cloudConfigType` is introduced, whose candicate values are `file`, `secret` and `merge` (default is `merge`).

action required:

Since Azure cloud provider would read Kubernetes secrets, the following RBAC should be configured:

  apiVersion: rbac.authorization.k8s.io/v1beta1
  kind: ClusterRole
  metadata:
    labels:
      kubernetes.io/cluster-service: "true"
    name: system:azure-cloud-provider-secret-getter
  rules:
  - apiGroups: [""]
    resources: ["secrets"]
    resourceNames: ["azure-cloud-provider"]
    verbs:
    - get
  ---
  apiVersion: rbac.authorization.k8s.io/v1beta1
  kind: ClusterRoleBinding
  metadata:
    labels:
      kubernetes.io/cluster-service: "true"
    name: system:azure-cloud-provider-secret-getter
  roleRef:
    apiGroup: rbac.authorization.k8s.io
    kind: ClusterRole
    name: system:azure-cloud-provider-secret-getter
  subjects:
  - kind: ServiceAccount
    name: azure-cloud-provider
    namespace: kube-system

/sig azure
/kind feature
/priority important-soon

/assign @khenidak @andyzhangx

@andyzhangx
Copy link
Member

left a comment

one question about the code logic, we have two path

init --> NewCloud --> initializeCloudFromConfig(fromSecret: false)
Cloud.Initialize --> initializeCloudFromSecret --> initializeCloudFromConfig(fromSecret: true)

so init would be invoked first and Cloud.Initialize could be invoked second, and finally it will goto initializeCloudFromConfig, right?

@feiskyer feiskyer force-pushed the feiskyer:configurable-provider branch from cde137a to 609f8cf May 24, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels May 24, 2019

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

/retest

@andyzhangx
Copy link
Member

left a comment

one question about the code logic, we have two path

init --> NewCloud --> initializeCloudFromConfig(fromSecret: false)
Cloud.Initialize --> initializeCloudFromSecret --> initializeCloudFromConfig(fromSecret: true)

so init would be invoked first and Cloud.Initialize could be invoked second, and finally it will goto initializeCloudFromConfig, right?

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

so init would be invoked first and Cloud.Initialize could be invoked second, and finally it will goto initializeCloudFromConfig, right?

Both would go to initializeCloudFromConfig, the difference is Kubelet won't invoke Initialize, while controller-manager would do. And for controller-manager, we want NewCloud won't error out, and initialize from secret again. But when secret initializing failed, controller-manager would fail with an error.

@andyzhangx

This comment has been minimized.

Copy link
Member

commented May 24, 2019

so init would be invoked first and Cloud.Initialize could be invoked second, and finally it will goto initializeCloudFromConfig, right?

Both would go to initializeCloudFromConfig, the difference is Kubelet won't invoke Initialize, while controller-manager would do. And for controller-manager, we want NewCloud won't error out, and initialize from secret again. But when secret initializing failed, controller-manager would fail with an error.

There are other drivers like CSI driver would also leverage this azure cloud provider lib, it uses NewCloud: https://github.com/kubernetes-sigs/azuredisk-csi-driver/blob/master/pkg/azuredisk/azure.go#L45
can we add a switch in NewCloud, let it read from secret also? by default, fromSecret could be false in NewCloud

@feiskyer feiskyer force-pushed the feiskyer:configurable-provider branch from 609f8cf to d127ef5 May 24, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/hold

So sorry for the last minute hold, but I think the kubelet checks are worth raising a flag for. Can we do this change without the kubelet checks?

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@andrewsykim thanks for reviewing. Agreed, the Kubelet check should be changed in alternative ways. will address these comments before v1.15 release

@andrewsykim

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/hold cancel

Thanks @feiskyer

@andrewsykim

This comment has been minimized.

Copy link
Member

commented May 30, 2019

heads up @claurence, we might have some follow-up PRs come in for this one

@andrewsykim

This comment has been minimized.

Copy link
Member

commented May 31, 2019

/hold

@feiskyer addressing comments given the code freeze extension

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

@andrewsykim Addressed comments. PTAL

/hold cancel

@justaugustus

This comment has been minimized.

Copy link
Member

commented May 31, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 31, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, feiskyer, justaugustus

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

@andrewsykim
Copy link
Member

left a comment

lgtm, thanks for iterating @feiskyer!

@k8s-ci-robot k8s-ci-robot merged commit bb7898b into kubernetes:master Jun 1, 2019

21 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-node-e2e-containerd 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 Jun 1, 2019

@feiskyer feiskyer deleted the feiskyer:configurable-provider branch Jun 3, 2019

mboersma added a commit to mboersma/aks-engine that referenced this pull request Jun 6, 2019
@mboersma mboersma referenced this pull request Jun 6, 2019
2 of 5 tasks complete
@jackfrancis

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@feiskyer will these capabilities ever be backported into pre-1.15 k8s or is this only applicable for 1.15 and greater?

@andyzhangx

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@feiskyer will these capabilities ever be backported into pre-1.15 k8s or is this only applicable for 1.15 and greater?

It won't be backported, only applicable for 1.15 and greater

jackfrancis added a commit to Azure/aks-engine that referenced this pull request Jun 8, 2019
feat: add support for Kubernetes 1.15.0-beta.2 (#1438)
* feat: add support for Kubernetes 1.15.0-beta.2

See https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG-1.15.md#changelog-since-v1150-beta1

* chore: add cluster role and binding for Azure secret getter

See kubernetes/kubernetes#78242
@feiskyer

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

@jackfrancis Yep, the feature is only available for v1.15 and greater

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.