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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: system:azure-cloud-provider-secret-getter
namespace: kube-system
rules:
- apiGroups:
- ""
resourceNames:
- azure-cloud-provider
resources:
- secrets
verbs:
- get
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: system:azure-cloud-provider-secret-getter
namespace: kube-system
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: system:azure-cloud-provider-secret-getter
subjects:
- kind: ServiceAccount
name: azure-cloud-provider
namespace: kube-system
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: v1
kind: Secret
metadata:
name: azure-cloud-provider
namespace: kube-system
stringData:
cloud-config: |
{{.CloudConfig | indent 4}}
7 changes: 6 additions & 1 deletion pkg/asset/machines/azure/machines.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ func provider(platform *azure.Platform, mpool *azure.MachinePool, osImage string
publicLB = ""
}

managedIdentity := fmt.Sprintf("%s-identity", clusterID)
if platform.IsARO() {
managedIdentity = ""
}

return &azureprovider.AzureMachineProviderSpec{
TypeMeta: metav1.TypeMeta{
APIVersion: "azureproviderconfig.openshift.io/v1beta1",
Expand All @@ -122,7 +127,7 @@ func provider(platform *azure.Platform, mpool *azure.MachinePool, osImage string
},
Zone: az,
Subnet: subnet,
ManagedIdentity: fmt.Sprintf("%s-identity", clusterID),
ManagedIdentity: managedIdentity,
Vnet: virtualNetwork,
ResourceGroup: rg,
NetworkResourceGroup: networkResourceGroup,
Expand Down
8 changes: 7 additions & 1 deletion pkg/asset/manifests/azure/cloudproviderconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,23 @@ type CloudProviderConfig struct {
NetworkSecurityGroupName string
VirtualNetworkName string
SubnetName string
ARO bool
}

// JSON generates the cloud provider json config for the azure platform.
// managed resource names are matching the convention defined by capz
func (params CloudProviderConfig) JSON() (string, error) {
useManagedIdentityExtension := true
if params.ARO {
useManagedIdentityExtension = false
}

config := config{
authConfig: authConfig{
Cloud: params.CloudName.Name(),
TenantID: params.TenantID,
SubscriptionID: params.SubscriptionID,
UseManagedIdentityExtension: true,
UseManagedIdentityExtension: useManagedIdentityExtension,
// The cloud provider needs the clientID which is only known after terraform has run.
// When left empty, the existing managed identity on the VM will be used.
// By leaving it empty, we don't have to create the identity before running the installer.
Expand Down
1 change: 1 addition & 0 deletions pkg/asset/manifests/cloudproviderconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ func (cpc *CloudProviderConfig) Generate(dependencies asset.Parents) error {
NetworkSecurityGroupName: nsg,
VirtualNetworkName: vnet,
SubnetName: subnet,
ARO: installConfig.Config.Azure.IsARO(),
}.JSON()
if err != nil {
return errors.Wrap(err, "could not create cloud provider config")
Expand Down
32 changes: 32 additions & 0 deletions pkg/asset/manifests/openshift.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (o *Openshift) Dependencies() []asset.Asset {
&openshift.PrivateClusterOutbound{},
&openshift.BaremetalConfig{},
new(rhcos.Image),
&openshift.AzureCloudProviderSecret{},
Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. One with // +build aro where we include them as dependencies
  2. 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:

  1. It will make the code harder to follow
  2. It will probably be unnecessary once you implement Azure Stack Hub support

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Use the template approach, like in my example above
  2. Generate all 3 resources in the cloud provider config, which was the original state of this PR
  3. Some combination of the above
  4. Generate the resource manifests somewhere else entirely

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
}

Expand Down Expand Up @@ -258,6 +259,37 @@ func (o *Openshift) Generate(dependencies asset.Parents) error {
assetData["99_private-cluster-outbound-service.yaml"] = applyTemplateData(privateClusterOutbound.Files()[0].Data, templateData)
}

if platform == azuretypes.Name && installConfig.Config.Azure.IsARO() {
// config is used to created compatible secret to trigger azure cloud
// controller config merge behaviour
// https://github.com/openshift/origin/blob/90c050f5afb4c52ace82b15e126efe98fa798d88/vendor/k8s.io/legacy-cloud-providers/azure/azure_config.go#L83
session, err := installConfig.Azure.Session()
if err != nil {
return err
}
config := struct {
AADClientID string `json:"aadClientId" yaml:"aadClientId"`
AADClientSecret string `json:"aadClientSecret" yaml:"aadClientSecret"`
}{
AADClientID: session.Credentials.ClientID,
AADClientSecret: session.Credentials.ClientSecret,
}

b, err := yaml.Marshal(config)
if err != nil {
return err
}

azureCloudProviderSecret := &openshift.AzureCloudProviderSecret{}
dependencies.Get(azureCloudProviderSecret)
for _, f := range azureCloudProviderSecret.Files() {
name := strings.TrimSuffix(filepath.Base(f.Filename), ".template")
assetData[name] = applyTemplateData(f.Data, map[string]string{
"CloudConfig": string(b),
})
}
}

o.FileList = []*asset.File{}
for name, data := range assetData {
if len(data) == 0 {
Expand Down
1 change: 1 addition & 0 deletions pkg/asset/targets/targets.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ var (
&openshift.CloudCredsSecret{},
&openshift.KubeadminPasswordSecret{},
&openshift.RoleCloudCredsSecretReader{},
&openshift.AzureCloudProviderSecret{},
}

// IgnitionConfigs are the ignition-configs targeted assets.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package openshift

import (
"os"
"path/filepath"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/templates/content"
)

const (
azureCloudProviderSecretFileName = "99_azure-cloud-provider-secret.yaml.template"
azureCloudProviderSecretGetterRoleBindingFileName = "99_azure-cloud-provider-secret-getter-rolebinding.yaml"
azureCloudProviderSecretGetterRoleFileName = "99_azure-cloud-provider-secret-getter-role.yaml"
)

var _ asset.WritableAsset = (*AzureCloudProviderSecret)(nil)

// AzureCloudProviderSecret is the variable to represent contents of corresponding files
type AzureCloudProviderSecret struct {
FileList []*asset.File
}

// Dependencies returns all of the dependencies directly needed by the asset
func (t *AzureCloudProviderSecret) Dependencies() []asset.Asset {
return []asset.Asset{}
}

// Name returns the human-friendly name of the asset.
func (t *AzureCloudProviderSecret) Name() string {
return "AzureCloudProviderSecret"
}

// Generate generates the actual files by this asset
func (t *AzureCloudProviderSecret) Generate(parents asset.Parents) error {
t.FileList = []*asset.File{}

for _, fileName := range []string{
azureCloudProviderSecretFileName,
azureCloudProviderSecretGetterRoleBindingFileName,
azureCloudProviderSecretGetterRoleFileName,
} {
data, err := content.GetOpenshiftTemplate(fileName)
if err != nil {
return err
}
t.FileList = append(t.FileList, &asset.File{
Filename: filepath.Join(content.TemplateDir, fileName),
Data: []byte(data),
})
}
return nil
}

// Files returns the files generated by the asset.
func (t *AzureCloudProviderSecret) Files() []*asset.File {
return t.FileList
}

// Load returns the asset from disk.
func (t *AzureCloudProviderSecret) Load(f asset.FileFetcher) (bool, error) {
t.FileList = []*asset.File{}

for _, fileName := range []string{
azureCloudProviderSecretFileName,
azureCloudProviderSecretGetterRoleBindingFileName,
azureCloudProviderSecretGetterRoleFileName,
} {
file, err := f.FetchByName(filepath.Join(content.TemplateDir, fileName))
if err != nil {
if os.IsNotExist(err) {
return false, nil
}
return false, err
}
t.FileList = append(t.FileList, file)
}

return true, nil
}
8 changes: 8 additions & 0 deletions pkg/types/azure/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import (
"strings"
)

// aro is a setting to enable aro-only modifications
var aro bool

// OutboundType is a strategy for how egress from cluster is achieved.
// +kubebuilder:validation:Enum="";Loadbalancer;UserDefinedRouting
type OutboundType string
Expand Down Expand Up @@ -115,3 +118,8 @@ func (p *Platform) ClusterResourceGroupName(infraID string) string {
}
return fmt.Sprintf("%s-rg", infraID)
}

// IsARO returns true if ARO-only modifications are enabled
func (p *Platform) IsARO() bool {
return aro
}
7 changes: 7 additions & 0 deletions pkg/types/azure/platform_aro.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build aro
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


package azure

func init() {
aro = true
}