Skip to content

WIP Azure IPI - Remove need for Subscription Admin Consent permission#2855

Closed
hassenius wants to merge 1 commit into
openshift:masterfrom
hassenius:azure-nonadmin
Closed

WIP Azure IPI - Remove need for Subscription Admin Consent permission#2855
hassenius wants to merge 1 commit into
openshift:masterfrom
hassenius:azure-nonadmin

Conversation

@hassenius

Copy link
Copy Markdown

@fabianofranz @jhixson74 I will probably need some help to get this one over the line.

Background

Initial proposal to allow deployments to Azure using IPI method without the need for Admin Consent, which is going to be an issue for a substantial portion of Azure accounts (including myself).
This is the subject of several issues, most specifically #2357

Essentially the only thing in the original Azure IPI approach that drove the need for Admin Consent was the terraform code to create the User Assigned Identity. However, in my experience it's very very difficult to obtain service principals with excessive access, not to mention unfeathered access -- particularly in a production environment.

Proposal

This PR allows a 2 phase deployment flow where a subscription admin (or anyone with the lesser User Administrator role) can create a Resource Group and a User Assigned Identity with Contributor role assigned to that Resource Group.
In Terraform terms this translates to

resource "azurerm_resource_group" "rg" {
  name     = var.resource_group_name
  location = var.location
}

resource "azurerm_user_assigned_identity" "main" {
  resource_group_name = azurerm_resource_group.rg.name
  location            = azurerm_resource_group.rg.location

  name = var.identity_name
}

resource "azurerm_role_assignment" "main" {
  scope                = azurerm_resource_group.rg.id
  role_definition_name = "Contributor"
  principal_id         = azurerm_user_assigned_identity.main.principal_id
}

If utilising the feature added in #2441 the Contributor or Network Contributor role should also be added to the resource group where the Vnet/Subnets are located.

For the second, installation, face the resource group and identity is then handed to the openshift installer via these two entries in install-config.yaml

platform:
  azure:
    resourceGroupName: my_precreated_resource_group
    userAssignedIdentity: ocpdeployer

Current state of this commit

The terraform builds the network, boot and master nodes successfully with the identity specified in install-config.yaml, but the installer eventually fails with this message:

23:09 $ ../bin/openshift-install create cluster
INFO Credentials loaded from file "/Users/hans/.azure/osServicePrincipal.json" 
INFO Consuming Install Config from target directory 
INFO Creating infrastructure resources...         

ERROR                                              
ERROR Error: Code="OSProvisioningTimedOut" Message="OS Provisioning for VM 'hkdevtest-p6zp4-master-2' did not finish in the allotted time. The VM may still finish provisioning successfully. Please check provisioning state later. Also, make sure the image has been properly prepared (generalized).\r\n * Instructions for Windows: https://azure.microsoft.com/documentation/articles/virtual-machines-windows-upload-image/ \r\n * Instructions for Linux: https://azure.microsoft.com/documentation/articles/virtual-machines-linux-capture-image/ " 
ERROR                                              
ERROR   on ../../../../../../../../private/var/folders/51/_07vdgj96pj1hc2lyhf7l8640000gn/T/openshift-install-335924962/master/master.tf line 37, in resource "azurerm_virtual_machine" "master": 
ERROR   37: resource "azurerm_virtual_machine" "master" { 
ERROR                                              
ERROR                                              
ERROR                                              
ERROR Error: Code="OSProvisioningTimedOut" Message="OS Provisioning for VM 'hkdevtest-p6zp4-master-1' did not finish in the allotted time. The VM may still finish provisioning successfully. Please check provisioning state later. Also, make sure the image has been properly prepared (generalized).\r\n * Instructions for Windows: https://azure.microsoft.com/documentation/articles/virtual-machines-windows-upload-image/ \r\n * Instructions for Linux: https://azure.microsoft.com/documentation/articles/virtual-machines-linux-capture-image/ " 
ERROR                                              
ERROR   on ../../../../../../../../private/var/folders/51/_07vdgj96pj1hc2lyhf7l8640000gn/T/openshift-install-335924962/master/master.tf line 37, in resource "azurerm_virtual_machine" "master": 
ERROR   37: resource "azurerm_virtual_machine" "master" { 
ERROR                                              
ERROR                                              
ERROR                                              
ERROR Error: Code="OSProvisioningTimedOut" Message="OS Provisioning for VM 'hkdevtest-p6zp4-master-0' did not finish in the allotted time. The VM may still finish provisioning successfully. Please check provisioning state later. Also, make sure the image has been properly prepared (generalized).\r\n * Instructions for Windows: https://azure.microsoft.com/documentation/articles/virtual-machines-windows-upload-image/ \r\n * Instructions for Linux: https://azure.microsoft.com/documentation/articles/virtual-machines-linux-capture-image/ " 
ERROR                                              
ERROR   on ../../../../../../../../private/var/folders/51/_07vdgj96pj1hc2lyhf7l8640000gn/T/openshift-install-335924962/master/master.tf line 37, in resource "azurerm_virtual_machine" "master": 
ERROR   37: resource "azurerm_virtual_machine" "master" { 
ERROR                                              
ERROR                                              
FATAL failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply using Terraform 

I am not able to troubleshoot why. From the boot node I can see the process stopping on this step in bootkube.sh

until bootkube_podman_run \
		--rm \
		--name etcdctl \
		--env ETCDCTL_API=3 \
		--volume /opt/openshift/tls:/opt/openshift/tls:ro,z \
		--entrypoint etcdctl \
		"${MACHINE_CONFIG_ETCD_IMAGE}" \
		--dial-timeout=10m \
		--cacert=/opt/openshift/tls/etcd-ca-bundle.crt \
		--cert=/opt/openshift/tls/etcd-client.crt \
		--key=/opt/openshift/tls/etcd-client.key \
		--endpoints="${ETCD_ENDPOINTS}" \
		endpoint health
do
	echo "etcdctl failed. Retrying in 5 seconds..."
	sleep 5
done

and I notice when I try ping the master nodes I get a lot of timeouts, so it seems the master nodes are rebooting a lot

--- 10.0.0.5 ping statistics ---
317 packets transmitted, 7 received, 97.7918% packet loss, time 782ms

Since I cannot create an environment without this code added, I'm not able to tell if this is because of this code, or a problem elsewhere in the codebase.

 ../bin/openshift-install version
../bin/openshift-install unreleased-master-2286-g24eebc9b2507d8d14e7cf1ce26ed556e1c484a1a-dirty
built from commit 24eebc9b2507d8d14e7cf1ce26ed556e1c484a1a
release image registry.svc.ci.openshift.org/origin/release:4.3

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2019
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

Hi @hassenius. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 20, 2019
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details 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

@hassenius

Copy link
Copy Markdown
Author

Worth noting, I haven't done anything with the destroy action, but initial thought is that an appropriate approach might be to but some indicator in the metadata.json file during installation to indicate that the cluster was created into an existing resource group so that the destroy action just cleans up the DNS resources and prints a message informing that the resource group and all it's resources need to be deleted manually?

ComputeSubnet string `json:"computeSubnet,omitempty"`

// ResourceGroupName specifies an existing resource group to deploy to
ResourceGroupName string `json:"resourceGroupName,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not acceptable for IPI installations. this increases the burden on the operators and installer as we needs another way to cleanup.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the case of Azure, this is really just a single action of deleting the resource group. This will delete everything else (except the DNS entries).
If it is desired to be able to be able to offer cleanup, we can pass the ResourceGroupName to the metadata.json file, and then in the destroy action check for this value. If the value is not there fallback to the naming assumption.

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/hold

any solution that depends on providing a resource group for cluster is high likely hood of being rejected.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2019
@hassenius

Copy link
Copy Markdown
Author

So, I would argue that, at least in my experience working with other Kubernetes vendors on Azure, for a majority of organisations will prefer a tightly scoped option, where they provide a resource group, network and a user with appropriate access to these only.
In my previous engagements it's even been difficult to obtain Contributor role to the Network resource groups (Network Contributor or custom roles being the preferred option).

@abhinavdahiya

Copy link
Copy Markdown
Contributor

Essentially the only thing in the original Azure IPI approach that drove the need for Admin Consent was the terraform code to create the User Assigned Identity

That's not the reason for Admin Consent requirement. Users with User Access Administrator role in the subsciptions can can create/manage managed-identities.

The actual reason is that cluster operators need credentials to communicate with Azure API. and OpenShift has the cred-minter cluster operator which creates new service principal and provides the credentials for that to the operators, making sure these new ones only have required permissions.

So this change doesn't solve the problem.

@hassenius

Copy link
Copy Markdown
Author

Bummer.

As per the design principles of the credential minter: https://github.com/openshift/cloud-credential-operator it should by design still be possible to supply credentials that are limited to the resource group scope (if my understanding is right); with the drawback that the individual components will have credentials that are too broadly scoped.
On the plus side though, the total blast radius is contained to the resource group, rather than potentially affecting the whole subscription which seems would be the case if the service principal with admin consent leaks.

As mentioned, in my organisation there is no way I can deploy OpenShift 4 using the IPI method with the current Admin Consent requirement. I believe there are many organisations using Azure in the same situation.

@abhinavdahiya

Copy link
Copy Markdown
Contributor

Closing in favor of generating consensus in #2357 before we work up the implementation..

/close

@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@abhinavdahiya: Closed this PR.

Details

In response to this:

Closing in favor of generating consensus in #2357 before we work up the implementation..

/close

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@hassenius: PR needs rebase.

Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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