Azure Stack cloud provider config #5042
Conversation
|
/test e2e-azure |
|
/test e2e-azure |
patrickdillon
force-pushed
the
azurestack-cloud-provider-config
branch
from
d210770
to
fc58baf
Compare
openshift-ci
bot
added
the
needs-rebase
The AzureStack cloud provider config differs from that of public Azure. This provides the appropriate values for the following keys when using ASH: useManagedIdentityExtension should be false useInstanceMetadata should be false loadBalancerSku should be basic
This is a temporary addition to add the client credentials to the cloud provider config to support bootstrapping the kubelet with the legacy cloud provider. Once the Azure out-of-tree provider has been implemented we can utilize a merged cloud provider config similar to ARO so that the client credentials are saved in a secret rather than in plaintext on the nodes. From my reading of the legacy provider in the kubelet, the merged config is not supported when bootstrapping (hence the need for this commit). The call for bootstrapping is made here: https://github.com/openshift/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/azure/azure.go#L360 But that code never calls getConfigFromSecret, which creates the merged config: https://github.com/openshift/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/azure/azure_config.go#L66 Instead, getConfigFromSecret is called from Initialize: https://github.com/openshift/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/azure/azure.go#L675 which appears to only be called from the kube-controller-manager: https://github.com/openshift/kubernetes/blob/master/cmd/kube-controller-manager/app/controllermanager.go#L601 This is not a problem for Public Azure because it uses managed identity, which is not supported in Azure Stack.
patrickdillon
force-pushed
the
azurestack-cloud-provider-config
branch
from
fc58baf
to
91b5b56
Compare
openshift-ci
bot
removed
the
needs-rebase
|
/lgtm |
|
@patrickdillon, maybe we can add ARM endpoint to cloud config? Actually, i think we can skip part with dumping endpoints config onto config-map and just put arm endpoint into cloud config. |
ARM endpoint is in the infrastructure object: Can you pull it from there? But thanks for the link. This is a good idea and I will add it to the auth section. |
Will have to mutate cloud-config in operator. No other way to pass this thing to ccm. Would be waaay more easy and convenient to put it there in installer. |
Adds a field to the Azure cloud provider config for the ARM endpoint and populates the value in the case of Azure Stack cloud.
|
/lgtm Thanks a lot! |
|
/test e2e-azure |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
openshift-ci
bot
added
the
approved
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
These commits add specific cloud provider configuration for Azure Stack Hub.
be4635c - Adds generic support, separating configuration that differs from public Azure.
d210770 - Is a temporary commit to add credentials to the cloud provider config. Once bootstrap support for the cloud controller manager is added we should revert this commit and keep credentials in a secret, which is not presently supported as explained in the commit message.
Merging d210770 would allow us to make progress with fully automated IPI install and reverting later would be simple enough. I can create a revert PR now with a basic PoC of how to use the merge config based on ARO.