azure: don't use managed identity on ARO #4843

m1kola · 2021-04-12T15:05:24Z

Adds a new aro build tag which is going to be used by ARO when vendoring the installer
Disables managed identity for ARO

openshift-ci-robot · 2021-04-12T15:05:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2021-04-12T15:05:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign wking after the PR has been reviewed.
You can assign the PR to them by writing /assign @wking 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:

OWNERS

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

staebler · 2021-04-12T15:57:34Z

This comes in timely for us, @m1kola. We need this same logic for Azure Stack, which does not have support for user-provided managed identities.

m1kola · 2021-04-12T16:12:10Z

@staebler Is there anything I should take into account while working on this PR? Maybe you want me to change something in it (like naming)?

Or should I go ahead and do what we need for ARO and later you refactor the codebase to accommodate Azure Stack?

staebler · 2021-04-12T16:39:28Z

@staebler Is there anything I should take into account while working on this PR? Maybe you want me to change something in it (like naming)?

Or should I go ahead and do what we need for ARO and later you refactor the codebase to accommodate Azure Stack?

I haven't dug into the PR yet to offer any opinion on naming and such. My inclination is to have you do what you need to ARO and we based the Azure Stack work around that, making any tweaks that we need.

patrickdillon · 2021-04-12T16:48:19Z

/assign

m1kola · 2021-04-12T16:54:40Z

My inclination is to have you do what you need to ARO and we based the Azure Stack work around that, making any tweaks that we need.

Sounds good to me.

Will look into CI failures next (tomorrow hopefully). Doesn't look like they are related to PR.

/retest

patrickdillon · 2021-04-13T01:25:37Z

/retest
/test e2e-azure
/test e2e-azure-upi
/test e2e-azure-shared-vpc

patrickdillon

I took an initial look and left some general comments.

As @staebler already mentioned, we need the logic for providing the clientID & secret in the cloud provider config for Azure Stack Hub support, so this PR is very helpful.

As my comment says, we already have logic for creating the secret and secret reader, so we should try to move the logic in this PR out of the cloud provider.

Have you considered any of the changes that would be required from the Terraform code, for example where we create user-assigned roles: https://github.com/openshift/installer/blob/master/data/data/azure/main.tf#L151-L170?

patrickdillon · 2021-04-13T01:43:31Z

pkg/types/azure/platform_aro.go

@@ -0,0 +1,7 @@
+// +build aro


nit: aro.go seems a better filename than platform_aro.go IMHO

I would break these changes in pkg/types into a separate commit.

nit: aro.go seems a better filename than platform_aro.go IMHO

platform_aro.go is going to be built only with build tag aro and it updates the value of the variable defined in platform.go. I think platform_aro.go makes the link less cryptic.

Also it mimics the way OKD is planning to gate their code in this PR (see pkg/types/installconfig_okd.go).

I would break these changes in pkg/types into a separate commit.

+1. Will do that.

I decided to split the build tag bit into a separate PR: #4874

Clarification: I'm happy for the build tag to be merged as part of this PR. But if there is still feedback related to managed identity - I suggest that we merge the build tag as part of #4874 as I have few more things that depend on it.

pkg/asset/manifests/cloudproviderconfig.go

m1kola

@patrickdillon thanks for the review. I'm looking into updating the PR. I left some responses to your comments below.

Have you considered any of the changes that would be required from the Terraform code, for example where we create user-assigned roles: https://github.com/openshift/installer/blob/master/data/data/azure/main.tf#L151-L170?

On ARO we do not use terraform code, so my plan is to introduce the changes we need for ARO and try not to break rest of the installer in the process :)

m1kola · 2021-04-13T16:54:48Z

pkg/types/azure/platform_aro.go

@@ -0,0 +1,7 @@
+// +build aro


nit: aro.go seems a better filename than platform_aro.go IMHO

platform_aro.go is going to be built only with build tag aro and it updates the value of the variable defined in platform.go. I think platform_aro.go makes the link less cryptic.

Also it mimics the way OKD is planning to gate their code in this PR (see pkg/types/installconfig_okd.go).

I would break these changes in pkg/types into a separate commit.

+1. Will do that.

pkg/asset/manifests/cloudproviderconfig.go

patrickdillon

I am on PTO today so just took a quick look. I can try to help with this more tomrrow or later this week.

pkg/asset/manifests/openshift.go

m1kola · 2021-04-20T14:11:19Z

@patrickdillon @staebler please take another look.

I updated naming and replaced yaml mrashaling with templates as requested. I left generation in pkg/asset/manifests/openshift.go for now, but I'm happy to move it back to pkg/asset/manifests/cloudproviderconfig.go as in the original version of this PR, if you think it is more appropriate place for it.

m1kola · 2021-04-20T14:24:51Z

pkg/asset/manifests/openshift.go

@@ -69,6 +69,9 @@ func (o *Openshift) Dependencies() []asset.Asset {
 		&openshift.PrivateClusterOutbound{},
 		&openshift.BaremetalConfig{},
 		new(rhcos.Image),
+		&openshift.AzureCloudProviderSecret{},


@patrickdillon an issue with the appraoch of using templates is this: we have to declare templates as dependencies and store will call Generate() on them even if we don't need them (e.g. non-ARO builds).

What do you think? Is it something we should worry about?

We can probably use build tags and create two extra files in this package:

One with // +build aro where we include them as dependencies

One with // +build !aro where we do not include them

But I don't want to rely on build tags too much in this case:

It will make the code harder to follow

It will probably be unnecessary once you implement Azure Stack Hub support

If we're going with the template approach, I was thinking to just add them to the existing templates. Here's a simplified example: master...patrickdillon:azure-merge-cloud-config We would need to add a boolean field to AzureCredsSecretData to switch this behavior on or off depending on ASH/ARO.

Also, not sure if it is valid to have the values of the cloud provider config secret be base64 encoded. I assume for the existing code, the cloud credential operator knows to base64 decode the values, but I assume that is up to the client. I don't see anything in the azure docs about this. If we don't know, we can test and see.

I would like to get @staebler's feedback about whether this is the right direction with templates. To summarize the context: In the case of ARO & ASH we need to create a secret (with the creds) , role, and role binding. I see a few options:

Use the template approach, like in my example above

Generate all 3 resources in the cloud provider config, which was the original state of this PR

Some combination of the above

Generate the resource manifests somewhere else entirely

I'm pretty neutral on these options with a slight leaning toward option 1.

@patrickdillon thanks for the feedback. Replied to some of the comments.

If we're going with the template approach, I was thinking to just add them to the existing templates. Here's a simplified example: master...patrickdillon:azure-merge-cloud-config We would need to add a boolean field to AzureCredsSecretData to switch this behavior on or off depending on ASH/ARO.

I was thinking about reusing data/data/manifests/openshift/cloud-creds-secret.yaml.template, but it is already overloaded with logic and I didn't want to make it worse. Happy to do this if you think it is a better appraoch.

Also, not sure if it is valid to have the values of the cloud provider config secret be base64 encoded. I assume for the existing code, the cloud credential operator knows to base64 decode the values, but I assume that is up to the client. I don't see anything in the azure docs about this. If we don't know, we can test and see.

We must encode values into base64 manually here because:

We add them into Secret's data:

The values for all keys in the data field have to be base64-encoded strings.

We use templates. Normaly when marshaling into json/yaml from a go struct it will be done implicitly due to the fact that data is defined as follows:

Data map[string][]byte `json:"data,omitempty" protobuf:"bytes,2,rep,name=data"`

And []byte in go marshaled as base64-encoded string.

Array and slice values encode as JSON arrays, except that []byte encodes as a base64-encoded string, and a nil slice encodes as the null JSON value.

On the receiving end, the cloud provider doesn't have to decode the value explicilty here: go will do it behind the scenes.

I was thinking about reusing data/data/manifests/openshift/cloud-creds-secret.yaml.template, but it is already overloaded with logic and I didn't want to make it worse. Happy to do this if you think it is a better appraoch.

As I look at this more closely I do think this current approach is on the right track. I think the approach should very closely follow the pattern here:
https://github.com/openshift/installer/blob/master/pkg/asset/manifests/openshift.go#L253-L259

we have to declare templates as dependencies and store will call Generate() on them even if we don't need them (e.g. non-ARO builds).

So yes this is fine to have a dependency generated even when it is not used. The above example is only for private clusters, but is generated for all clusters. But you should consolidate all of these Azure Cloud Provider dependencies into a single asset, which in the above example would be equivalent to private-cluster-outbound-service.go. Note that the one asset could have many templates; so you could either have a separate template for each resource or combine the three resources in a single template.

If this is unclear or your have questions let me know. I think this is the right drection!

Also, thanks for the detailed, awesome explanation of why secret values are b64 encoded. I didn't know that!

@patrickdillon I left 3 template files, but consolidated them into one assset. Now the naming is little bit tricky: I called it AzureCloudProviderSecret, but it contains the role, the role binding as well as the secret which might be a little bit confusing. Suggestions are welcome :)

I'm also happy to put all yamls into one file, if you like. Having in separate templates/manifests is a tiny bit better for visibility, IMO. But I have no strong prefrences.

I think this is looking good. Have you tested this? I hope to test it soon with Azure Stack.

I called it AzureCloudProviderSecret... Suggestions are welcome :)

Maybe AzureMergedCloudConfig?

@patrickdillon

Have you tested this?

I did openshift-install create manifests with aro build tag and manifests look good. I alos did it with a binary compiled without aro build tag and relevant manifests were not present in the output directory, as expected.

And I'm pretty confident that it will work with ARO because I tested it with older version of this PR (we did not change outputs significantly in recent changes). I'll ARO integration tomorrow, but it should not block this PR.

Maybe AzureMergedCloudConfig?

I was wonder if we can better reflect the fact that it is not only secret, but role and role binding as well.
Let's probably keep it as AzureCloudProviderSecret as it consists of resource name azure-cloud-provider and resource type secret. Resource name is forced on us by cloud provider.

I'm also happy to put all yamls into one file, if you like. Having in separate templates/manifests is a tiny bit better for visibility, IMO. But I have no strong prefrences.

Leave it as one file for each resource. The CVO has (or had) issues with multiple resources in a single manifest when there are problems applying some of the resources.

We must encode values into base64 manually here because:

We add them into Secret's data:

Use stringData instead of data. Then you don't need to do the base-64 encoding. The API server will do the base-64 encoding for you.

I didn't know that stringData merges into data.

@staebler I updated the PR to use stringData. Note that everything else in pkg/asset/manifests/openshift.go explicitly encodes into base64. I'm happy to revert back to explicit encoding for consistency, if you wish.

* Adds a new aro build tag which is going to be used by ARO when vendoring the installer * Adds IsARO() method to gate ARO-only modifications

pkg/asset/manifests/openshift.go

At the moment OCP on Azure uses MSI for kubelets and controllers and one or more service principals for operators. For now on ARO, simplify to all components using the user-provided SP. Later, we'll reinstate a separate managed identity at least for worker kubelets. PR: openshift#4843

At the moment OCP on Azure uses MSI for kubelets and controllers and one or more service principals for operators. For now on ARO, simplify to all components using the user-provided SP. Later, we'll reinstate a separate managed identity at least for worker kubelets. PR: github.com/openshift/pull/4843

At the moment OCP on Azure uses MSI for kubelets and controllers and one or more service principals for operators. For now on ARO, simplify to all components using the user-provided SP. Later, we'll reinstate a separate managed identity at least for worker kubelets. PR: openshift#4843

patrickdillon · 2021-06-17T10:17:07Z

/lgtm
/approve

openshift-ci · 2021-06-17T10:17:15Z

[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:

~~OWNERS~~ [patrickdillon]

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

openshift-bot · 2021-06-19T04:23:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T04:36:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T06:34:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T09:03:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T09:27:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T15:28:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T17:41:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T18:41:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T19:41:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T21:05:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-19T22:53:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-06-20T00:35:27Z

@m1kola: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure-upi	`1402758`	link	`/test e2e-azure-upi`

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.

openshift-bot · 2021-06-20T01:09:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

At the moment OCP on Azure uses MSI for kubelets and controllers and one or more service principals for operators. For now on ARO, simplify to all components using the user-provided SP. Later, we'll reinstate a separate managed identity at least for worker kubelets. PR: openshift#4843

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 Apr 12, 2021

openshift-ci-robot requested review from fabianofranz and jhixson74 April 12, 2021 15:05

openshift-ci-robot assigned patrickdillon Apr 12, 2021

patrickdillon reviewed Apr 13, 2021

View reviewed changes

m1kola commented Apr 13, 2021

View reviewed changes

m1kola force-pushed the aro_disable_managed_identity branch 3 times, most recently from 32aedef to d76ce45 Compare April 19, 2021 08:45

m1kola marked this pull request as ready for review April 19, 2021 08:46

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2021

patrickdillon reviewed Apr 19, 2021

View reviewed changes

pkg/asset/manifests/openshift.go Outdated Show resolved Hide resolved

pkg/asset/manifests/openshift.go Outdated Show resolved Hide resolved

m1kola force-pushed the aro_disable_managed_identity branch 2 times, most recently from 2f474cb to 90b778a Compare April 20, 2021 14:02

m1kola requested a review from patrickdillon April 20, 2021 14:11

m1kola commented Apr 20, 2021

View reviewed changes

azure: Adds a gate for ARO-only modifications

dc6d6d0

* Adds a new aro build tag which is going to be used by ARO when vendoring the installer * Adds IsARO() method to gate ARO-only modifications

m1kola force-pushed the aro_disable_managed_identity branch from 90b778a to 888a21a Compare April 22, 2021 14:06

patrickdillon reviewed Apr 22, 2021

View reviewed changes

pkg/asset/manifests/openshift.go Outdated Show resolved Hide resolved

m1kola force-pushed the aro_disable_managed_identity branch from 888a21a to 8b2f210 Compare April 22, 2021 15:06

m1kola requested a review from patrickdillon April 22, 2021 15:59

m1kola requested review from staebler and patrickdillon June 4, 2021 08:59

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2021

openshift-merge-robot merged commit 63ac266 into openshift:master Jun 20, 2021

m1kola deleted the aro_disable_managed_identity branch June 21, 2021 09:29

m1kola mentioned this pull request Aug 20, 2021

Add subscription feature MTU3900 Azure/ARO-RP#1672

Merged

azure: don't use managed identity on ARO #4843

azure: don't use managed identity on ARO #4843

Conversation

m1kola commented Apr 12, 2021

openshift-ci-robot commented Apr 12, 2021

openshift-ci-robot commented Apr 12, 2021

staebler commented Apr 12, 2021

m1kola commented Apr 12, 2021

staebler commented Apr 12, 2021

patrickdillon commented Apr 12, 2021

m1kola commented Apr 12, 2021

patrickdillon commented Apr 13, 2021

patrickdillon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon left a comment

Choose a reason for hiding this comment

m1kola commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon Apr 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdillon commented Jun 17, 2021

openshift-ci bot commented Jun 17, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-bot commented Jun 19, 2021

openshift-ci bot commented Jun 20, 2021

openshift-bot commented Jun 20, 2021

patrickdillon Apr 22, 2021 •

edited