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.

Sign up for GitHub

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

Jump to bottom

Add support for azure-keyvault-secrets-provider AKS addon #14308

Merged
merged 16 commits into from
Dec 7, 2021
Merged

Add support for azure-keyvault-secrets-provider AKS addon #14308

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bbc531d
Add support for CSI driver
Nov 22, 2021
9c6f010
Support for rotationPollInterval
Nov 22, 2021
d0f2058
Add test for AKV provider addon
Nov 22, 2021
3f2d553
Added validation for secret rotation interval
Nov 23, 2021
5faf107
Fixed state file
Nov 23, 2021
f2fa9b7
Added data source
Nov 23, 2021
e8ed0d0
Added docs for AKV provider
Nov 23, 2021
3136409
Format tests
Nov 23, 2021
21aec33
Address review comments
Nov 24, 2021
e5bb366
Use better logic for defaults. Use existing validation
Nov 24, 2021
e0d1570
Support exporting identity for AKV provider
Nov 24, 2021
75f78e6
Fix acceptance test
Nov 30, 2021
9a67bb1
Fix docs, rename identity block
Dec 1, 2021
09c7ab5
Fix order of properties
Dec 3, 2021
ad15dde
Rename identity block to secret_identity
Dec 6, 2021
aa19a84
default enableSecretRotation in expand func
stephybun Dec 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 127 additions & 30 deletions internal/services/containers/kubernetes_addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/Azure/azure-sdk-for-go/services/containerservice/mgmt/2021-08-01/containerservice"
"github.com/Azure/go-autorest/autorest/azure"
commonValidate "github.com/hashicorp/terraform-provider-azurerm/helpers/validate"
containerValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/containers/validate"
laparse "github.com/hashicorp/terraform-provider-azurerm/internal/services/loganalytics/parse"
logAnalyticsValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/loganalytics/validate"
applicationGatewayValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/network/validate"
Expand All @@ -18,13 +19,14 @@ import (

const (
// note: the casing on these keys is important
aciConnectorKey = "aciConnectorLinux"
azurePolicyKey = "azurepolicy"
kubernetesDashboardKey = "kubeDashboard"
httpApplicationRoutingKey = "httpApplicationRouting"
omsAgentKey = "omsagent"
ingressApplicationGatewayKey = "ingressApplicationGateway"
openServiceMeshKey = "openServiceMesh"
aciConnectorKey = "aciConnectorLinux"
azurePolicyKey = "azurepolicy"
kubernetesDashboardKey = "kubeDashboard"
httpApplicationRoutingKey = "httpApplicationRouting"
omsAgentKey = "omsagent"
ingressApplicationGatewayKey = "ingressApplicationGateway"
openServiceMeshKey = "openServiceMesh"
azureKeyvaultSecretsProviderKey = "azureKeyvaultSecretsProvider"
)

// The AKS API hard-codes which add-ons are supported in which environment
Expand All @@ -34,16 +36,18 @@ const (
// omitted from this list an addon/environment combination will be supported
var unsupportedAddonsForEnvironment = map[string][]string{
azure.ChinaCloud.Name: {
aciConnectorKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5510
httpApplicationRoutingKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5960
kubernetesDashboardKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/7487
openServiceMeshKey, // Preview features are not supported in Azure China
aciConnectorKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5510
httpApplicationRoutingKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5960
kubernetesDashboardKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/7487
openServiceMeshKey, // Preview features are not supported in Azure China
azureKeyvaultSecretsProviderKey, // Preview features are not supported in Azure China
},
azure.USGovernmentCloud.Name: {
azurePolicyKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/6702
httpApplicationRoutingKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5960
kubernetesDashboardKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/7136
openServiceMeshKey, // Preview features are not supported in Azure Government
azurePolicyKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/6702
httpApplicationRoutingKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/5960
kubernetesDashboardKey, // https://github.com/hashicorp/terraform-provider-azurerm/issues/7136
openServiceMeshKey, // Preview features are not supported in Azure Government
azureKeyvaultSecretsProviderKey, // Preview features are not supported in Azure China
},
}

Expand Down Expand Up @@ -235,6 +239,50 @@ func schemaKubernetesAddOnProfiles() *pluginsdk.Schema {
},
},
},
"azure_keyvault_secrets_provider": {
Type: pluginsdk.TypeList,
MaxItems: 1,
Optional: true,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"enabled": {
stephybun marked this conversation as resolved.
Show resolved Hide resolved
Type: pluginsdk.TypeBool,
Required: true,
},
Comment on lines +248 to +251
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than exposing this field - we can take the presence of the parent block to mean enabled=true (and it's false if it's omitted)

Copy link
Contributor

Choose a reason for hiding this comment

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

@tombuildsstuff, @stephybun asked a similar question earlier: #14308 (comment)

This wouldn't match with the other addon blocks which have their own enabled status. Plus I worry if we have existing clusters that have it manually enabled but the Terraform isn't updated, it may try to disable them on subsequent applies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw your below comment about consistency with other blocks. I'm still worried how this will play out as more addons are created and manually applied during previews. I realize that with a diff or preview, we should catch these possible changes, but if it is being automated it may not be.

"secret_rotation_enabled": {
Type: pluginsdk.TypeBool,
Default: false,
Optional: true,
},
"secret_rotation_interval": {
Type: pluginsdk.TypeString,
Optional: true,
Default: "2m",
ValidateFunc: containerValidate.Duration,
},
"azure_keyvault_secrets_provider_identity": {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this one in the initial review, since this block is already called azure_keyvault_secrets_provider could we rename this to just identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I was just following the convention with the other addons, where we have ingress_application_gateway_identity and oms_agent_identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the convention might be there to prevent confusion?

Looking at the AKS module docs, it might get a bit confusing if there were multiple The identity block exports the following: sections, which I guess is why each identity has its own name?

Copy link
Member

Choose a reason for hiding this comment

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

If the schemas for each identity block were different I would agree, but they're all identical so I don't really see an issue if they all point to the same block in the docs. To be sure I will clarify internally whether there is any other reason for that naming convention. If not it should be renamed and the other two properties will also need to be renamed and deprecated (in a separate PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could also be that the AKS cluster itself already exports an identity block, which does have a different schema from the addon identity blocks?

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 it would add confusion with the AKS identity and kubelet_identity blocks like you said. Maybe something more generic for the addons (in a future PR) like addon_identity? It's always going to be the same with a user_assigned_managed_identity anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the background on this is each identity block currently has a distinct name because of how these are represented in the docs - as such I'd agree this probably should be secret_identity here.

To call out the comment above too, we'll be making the add-on blocks consistent with the rest of the provider where possible in a future release, that is, having the presence of the block mean enabled = true and the omission of the block meaning enabled = false. This is required since there's a tri-state in the Azure API currently which needs resolving (e.g. the block being omitted in the API, or enabled=false mean the same thing - and then there's enabled=true - but we need to consolidate these into Terraform.

Type: pluginsdk.TypeList,
Computed: true,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"client_id": {
Type: pluginsdk.TypeString,
Computed: true,
},
"object_id": {
Type: pluginsdk.TypeString,
Computed: true,
},
"user_assigned_identity_id": {
Type: pluginsdk.TypeString,
Computed: true,
},
},
},
},
},
},
},
},
},
}
Expand All @@ -246,13 +294,14 @@ func expandKubernetesAddOnProfiles(input []interface{}, env azure.Environment) (
}

profiles := map[string]*containerservice.ManagedClusterAddonProfile{
aciConnectorKey: &disabled,
azurePolicyKey: &disabled,
kubernetesDashboardKey: &disabled,
httpApplicationRoutingKey: &disabled,
omsAgentKey: &disabled,
ingressApplicationGatewayKey: &disabled,
openServiceMeshKey: &disabled,
aciConnectorKey: &disabled,
azurePolicyKey: &disabled,
kubernetesDashboardKey: &disabled,
httpApplicationRoutingKey: &disabled,
omsAgentKey: &disabled,
ingressApplicationGatewayKey: &disabled,
openServiceMeshKey: &disabled,
azureKeyvaultSecretsProviderKey: &disabled,
}

if len(input) == 0 || input[0] == nil {
Expand Down Expand Up @@ -371,6 +420,27 @@ func expandKubernetesAddOnProfiles(input []interface{}, env azure.Environment) (

}

azureKeyvaultSecretsProvider := profile["azure_keyvault_secrets_provider"].([]interface{})
if len(azureKeyvaultSecretsProvider) > 0 && azureKeyvaultSecretsProvider[0] != nil {
value := azureKeyvaultSecretsProvider[0].(map[string]interface{})
config := make(map[string]*string)
enabled := value["enabled"].(bool)

if secretRotation, ok := value["secret_rotation_enabled"]; ok && secretRotation.(bool) {
config["enableSecretRotation"] = utils.String("true")
Copy link
Contributor

Choose a reason for hiding this comment

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

this'll also want a default value set of false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to match the provider's behaviour with what the CLI does. In that case, when disabling secret rotation (or just not enabling it), the field is completely omitted from the API response.

I can also set it to false, if that's preferable.

}

if rotationPollInterval, ok := value["secret_rotation_interval"]; ok && rotationPollInterval != "" {
config["rotationPollInterval"] = utils.String(rotationPollInterval.(string))
}

addonProfiles[azureKeyvaultSecretsProviderKey] = &containerservice.ManagedClusterAddonProfile{
Enabled: utils.Bool(enabled),
Config: config,
}

}

return filterUnsupportedKubernetesAddOns(addonProfiles, env)
}

Expand Down Expand Up @@ -544,20 +614,47 @@ func flattenKubernetesAddOnProfiles(profile map[string]*containerservice.Managed
})
}

azureKeyvaultSecretsProviders := make([]interface{}, 0)
if azureKeyvaultSecretsProvider := kubernetesAddonProfileLocate(profile, azureKeyvaultSecretsProviderKey); azureKeyvaultSecretsProvider != nil {
enabled := false
if enabledVal := azureKeyvaultSecretsProvider.Enabled; enabledVal != nil {
enabled = *enabledVal
}
enableSecretRotation := false
if v := kubernetesAddonProfilelocateInConfig(azureKeyvaultSecretsProvider.Config, "enableSecretRotation"); v != nil && v != utils.String("false") {
enableSecretRotation = true
}
rotationPollInterval := ""
if v := kubernetesAddonProfilelocateInConfig(azureKeyvaultSecretsProvider.Config, "rotationPollInterval"); v != nil {
rotationPollInterval = *v
}

azureKeyvaultSecretsProviderIdentity := flattenKubernetesClusterAddOnIdentityProfile(azureKeyvaultSecretsProvider.Identity)

azureKeyvaultSecretsProviders = append(azureKeyvaultSecretsProviders, map[string]interface{}{
"enabled": enabled,
"secret_rotation_enabled": enableSecretRotation,
"secret_rotation_interval": rotationPollInterval,
"azure_keyvault_secrets_provider_identity": azureKeyvaultSecretsProviderIdentity,
})

}

// this is a UX hack, since if the top level block isn't defined everything should be turned off
if len(aciConnectors) == 0 && len(azurePolicies) == 0 && len(httpApplicationRoutes) == 0 && len(kubeDashboards) == 0 && len(omsAgents) == 0 && len(ingressApplicationGateways) == 0 && len(openServiceMeshes) == 0 {
if len(aciConnectors) == 0 && len(azurePolicies) == 0 && len(httpApplicationRoutes) == 0 && len(kubeDashboards) == 0 && len(omsAgents) == 0 && len(ingressApplicationGateways) == 0 && len(openServiceMeshes) == 0 && len(azureKeyvaultSecretsProviders) == 0 {
return []interface{}{}
}

return []interface{}{
map[string]interface{}{
"aci_connector_linux": aciConnectors,
"azure_policy": azurePolicies,
"http_application_routing": httpApplicationRoutes,
"kube_dashboard": kubeDashboards,
"oms_agent": omsAgents,
"ingress_application_gateway": ingressApplicationGateways,
"open_service_mesh": openServiceMeshes,
"aci_connector_linux": aciConnectors,
"azure_policy": azurePolicies,
"http_application_routing": httpApplicationRoutes,
"kube_dashboard": kubeDashboards,
"oms_agent": omsAgents,
"ingress_application_gateway": ingressApplicationGateways,
"open_service_mesh": openServiceMeshes,
"azure_keyvault_secrets_provider": azureKeyvaultSecretsProviders,
},
}
}
Expand Down
99 changes: 88 additions & 11 deletions internal/services/containers/kubernetes_cluster_addons_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@ import (
)

var kubernetesAddOnTests = map[string]func(t *testing.T){
"addonProfileAciConnectorLinux": testAccKubernetesCluster_addonProfileAciConnectorLinux,
"addonProfileAciConnectorLinuxDisabled": testAccKubernetesCluster_addonProfileAciConnectorLinuxDisabled,
"addonProfileAzurePolicy": testAccKubernetesCluster_addonProfileAzurePolicy,
"addonProfileKubeDashboard": testAccKubernetesCluster_addonProfileKubeDashboard,
"addonProfileOMS": testAccKubernetesCluster_addonProfileOMS,
"addonProfileOMSToggle": testAccKubernetesCluster_addonProfileOMSToggle,
"addonProfileRouting": testAccKubernetesCluster_addonProfileRoutingToggle,
"addonProfileAppGatewayAppGatewayId": testAccKubernetesCluster_addonProfileIngressApplicationGateway_appGatewayId,
"addonProfileAppGatewaySubnetCIDR": testAccKubernetesCluster_addonProfileIngressApplicationGateway_subnetCIDR,
"addonProfileAppGatewaySubnetID": testAccKubernetesCluster_addonProfileIngressApplicationGateway_subnetId,
"addonProfileOpenServiceMesh": testAccKubernetesCluster_addonProfileOpenServiceMesh,
"addonProfileAciConnectorLinux": testAccKubernetesCluster_addonProfileAciConnectorLinux,
"addonProfileAciConnectorLinuxDisabled": testAccKubernetesCluster_addonProfileAciConnectorLinuxDisabled,
"addonProfileAzurePolicy": testAccKubernetesCluster_addonProfileAzurePolicy,
"addonProfileKubeDashboard": testAccKubernetesCluster_addonProfileKubeDashboard,
"addonProfileOMS": testAccKubernetesCluster_addonProfileOMS,
"addonProfileOMSToggle": testAccKubernetesCluster_addonProfileOMSToggle,
"addonProfileRouting": testAccKubernetesCluster_addonProfileRoutingToggle,
"addonProfileAppGatewayAppGatewayId": testAccKubernetesCluster_addonProfileIngressApplicationGateway_appGatewayId,
"addonProfileAppGatewaySubnetCIDR": testAccKubernetesCluster_addonProfileIngressApplicationGateway_subnetCIDR,
"addonProfileAppGatewaySubnetID": testAccKubernetesCluster_addonProfileIngressApplicationGateway_subnetId,
"addonProfileOpenServiceMesh": testAccKubernetesCluster_addonProfileOpenServiceMesh,
"addonProfileAzureKeyvaultSecretsProvider": TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider,
}

var addOnAppGatewaySubnetCIDR string = "10.241.0.0/16" // AKS will use 10.240.0.0/16 for the aks subnet so use 10.241.0.0/16 for the app gateway subnet
Expand Down Expand Up @@ -375,6 +376,36 @@ func testAccKubernetesCluster_addonProfileOpenServiceMesh(t *testing.T) {
})
}

func TestAccKubernetesCluster_addonProfileAzureKeyvaultSecretsProvider(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test")
r := KubernetesClusterResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
// Enable AzureKeyvaultSecretsProvider
Config: r.addonProfileAzureKeyvaultSecretsProviderConfig(data, true, true, "2m"),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.#").HasValue("1"),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.0.enabled").HasValue("true"),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.0.secret_rotation_enabled").HasValue("true"),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.0.secret_rotation_interval").HasValue("2m"),
),
},
data.ImportStep(),
{
// Disable AzureKeyvaultSecretsProvider
Config: r.addonProfileAzureKeyvaultSecretsProviderConfig(data, false, false, "2m"),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.#").HasValue("1"),
check.That(data.ResourceName).Key("addon_profile.0.azure_keyvault_secrets_provider.0.enabled").HasValue("false"),
),
},
data.ImportStep(),
})
}

func (KubernetesClusterResource) addonProfileAciConnectorLinuxConfig(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down Expand Up @@ -1173,3 +1204,49 @@ resource "azurerm_kubernetes_cluster" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, data.RandomInteger, enabled)
}

func (KubernetesClusterResource) addonProfileAzureKeyvaultSecretsProviderConfig(data acceptance.TestData, enabled bool, secretRotation bool, rotationInterval string) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-aks-%d"
location = "%s"
}

resource "azurerm_kubernetes_cluster" "test" {
name = "acctestaks%d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
dns_prefix = "acctestaks%d"

linux_profile {
admin_username = "acctestuser%d"

ssh_key {
key_data = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCqaZoyiz1qbdOQ8xEf6uEu1cCwYowo5FHtsBhqLoDnnp7KUTEBN+L2NxRIfQ781rxV6Iq5jSav6b2Q8z5KiseOlvKA/RF2wqU0UPYqQviQhLmW6THTpmrv/YkUCuzxDpsH7DUDhZcwySLKVVe0Qm3+5N2Ta6UYH3lsDf9R9wTP2K/+vAnflKebuypNlmocIvakFWoZda18FOmsOoIVXQ8HWFNCuw9ZCunMSN62QGamCe3dL5cXlkgHYv7ekJE15IA9aOJcM7e90oeTqo+7HTcWfdu0qQqPWY5ujyMw/llas8tsXY85LFqRnr3gJ02bAscjc477+X+j/gkpFoN1QEmt [email protected]"
}
}

default_node_pool {
name = "default"
node_count = 1
vm_size = "Standard_DS2_v2"
}

addon_profile {
azure_keyvault_secrets_provider {
enabled = %t
secret_rotation_enabled = %t
secret_rotation_interval = "%s"
}
}

identity {
type = "SystemAssigned"
}
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger, data.RandomInteger, enabled, secretRotation, rotationInterval)
}
Loading