betr-io / terraform-provider-mssql

Terraform provider for Microsoft SQL Server
https://registry.terraform.io/providers/betr-io/mssql/latest
MIT License
35 stars 28 forks source link

Why the "server" block instead of using provider authentication? #50

Open karlschriek opened 1 year ago

karlschriek commented 1 year ago

I was wondering what the reasoning is for logging into the SQL Server via the "server" block within the resource specs? This is the first time that I have seen a terraform provider do it like this and was wondering if there is a specific reason for this.

To be clear what I mean, instead of passing the authentication information via the resource itself, I would have expected to instead initialise the provider with it, thereby establishing a session that is used by all resources thereafter. I.e., something that looks like this:

terraform {
  required_providers {
    mssql = {
      source  = "betr-io/mssql"
      version = "0.2.5"
    }
  }
}

provider "mssql" {
  host = "example-sql-server.database.windows.net"
  azure_login {
    tenant_id     = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
    client_id     = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
    client_secret = "terriblySecretSecret"
  }
}

resource "mssql_login" "example" {
  login_name = "testlogin"
  password = "verysecret"
}

resource "mssql_login" "example2" {
  login_name = "testlogin2"
  password = "veryverysecret"
}
karlschriek commented 1 year ago

Authenticating via the provider configuration would also make this issue go away: https://github.com/betr-io/terraform-provider-mssql/issues/49

MarcinGrinberg commented 1 year ago

We've stumbled upon this issue when our service principal secret expired. It would be great if this behavior could be changed as at the moment I need to go through all of our terraform state files and fix client_secret field.

If the provider configuration were used then there would be no need to even keep credentials in a state file. WIn-win.

karlschriek commented 1 year ago

I've created a fork where I implemented this. It broke the acceptance tests though, and I haven't managed to get them working (also not really something that is a priority for us right now, so I have limited time to look into it). I could put in a PR with the changes though if someone wants to help to get it across the line.

On Wed, 16 Nov 2022 at 10:54, Marcin Grinberg @.***> wrote:

We've stumbled upon this issue when our service principal secret expired. It would be great if this behavior could be changed as at the moment I need to go through all of our terraform state files and fix client_secret field.

If the provider configuration were used then there would be no need to even keep credentials in a state file. WIn-win.

— Reply to this email directly, view it on GitHub https://github.com/betr-io/terraform-provider-mssql/issues/50#issuecomment-1316715244, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGBE4ODQX5IBCC47A3ORKV3WISVOTANCNFSM6AAAAAARZJ4FHE . You are receiving this because you authored the thread.Message ID: @.***>

magne commented 1 year ago

There is a (good?) reason for this behavior.

The design aims to expose as few secrets as possible, and it will not change (dramatically). As a side-effect it makes it easy to manage multiple databases with a single provider. If the credentials were kept in the provider you would need a provider per database. But this is just a happy consequence, not a reason for this design.

I initially created the provider with credentials provided in the provider block. But unlike most providers (e.g. azurerm or aws), this provider is used to create resources inside a resource created by another provider, usually in the same terraform file(s). And since you can't use attributes exposed by other resources, you ultimately have to specify any credentials in the input variables. I wanted to create the credentials in my terraform scripts, never exposing them outside of the terraform state (which is kept in a pretty locked down azure storage account) and in a key vault. This way the only credentials needed outside the scripts are the credentials used to run the scripts.

I agree that updating the state when a SP secret expires is a PITA, but it is a consequence of not having access to updated attribute values in the planning phase (at least I have not been able to find a way to access this information) (at least I think this was the problem; it's been a while).

One possible solution to #49 might be to allow temporary override of the credentials (possibly from environment variables).

PavelPikat commented 1 year ago

@magne

If the credentials were kept in the provider you would need a provider per database.

What's wrong with that? This is expected, and in line with many other providers

And since you can't use attributes exposed by other resources, you ultimately have to specify any credentials in the input variables

Yes you absolutely can use an output of a resource as input for the provider. E.g. output from SQL Server resource as input to this provider. This is how we use Kubernetes provider, for example (which is also managing resource created by another provider btw) - we pass output from azurerm_kubernetes_cluster as input to kubernetes provider. This is very common in a lot of providers.

I wanted to create the credentials in my terraform scripts, never exposing them outside of the terraform state (which is kept in a pretty locked down azure storage account) and in a key vault. This way the only credentials needed outside the scripts are the credentials used to run the scripts.

You don't have to expose provider's credentials to your pipeline or scripts. You can still keep it internal to your Terraform configuration. If you use Key Vault, then you could just use data resource to read a KV secret and pass it to the provider.

magne commented 1 year ago

If the credentials were kept in the provider you would need a provider per database.

What's wrong with that? This is expected, and in line with many other providers

As I stated, this is not a reason for the current design, just a consequence.

As for using output of resources as input to the provider block, the terraform documentation for the latest version (v1.3.x) states:

You can use expressions in the values of these configuration arguments, but can only reference values that are known before the configuration is applied. This means you can safely reference input variables, but not attributes exported by resources (with an exception for resource arguments that are specified directly in the configuration).

If you can show me an example to the contrary, I will look into it.

And your last point is totally valid, given of course that you can access output variables of resources in provider blocks.

PavelPikat commented 1 year ago

If you can show me an example to the contrary, I will look into it.

I can share our configuration for deploying AKS clusters and configuring them with Helm charts and Kubernetes resources (like namespaces, RBAC etc) all in one go.

provider "azurerm" {
  features {}
}

provider "helm" {
  kubernetes {
    host                   = module.eu1-aks-devtest-1.host
    username               = module.eu1-aks-devtest-1.kube_admin_config_username
    password               = module.eu1-aks-devtest-1.kube_admin_config_password
    client_certificate     = module.eu1-aks-devtest-1.base64_client_certificate
    client_key             = module.eu1-aks-devtest-1.base64_client_key
    cluster_ca_certificate = module.eu1-aks-devtest-1.base64_cluster_ca_certificate
  }
}

provider "kubernetes" {
  host                   = module.eu1-aks-devtest-1.host
  username               = module.eu1-aks-devtest-1.kube_admin_config_username
  password               = module.eu1-aks-devtest-1.kube_admin_config_password
  client_certificate     = module.eu1-aks-devtest-1.base64_client_certificate
  client_key             = module.eu1-aks-devtest-1.base64_client_key
  cluster_ca_certificate = module.eu1-aks-devtest-1.base64_cluster_ca_certificate
}

# Uses azurerm provider to provision AKS
module "eu1-aks-devtest-1" {
  source          = "../../modules/aks"
  <other params to create AKS clusters>
}

# Uses helm and kubernetes providers to deploy nginx ingress to deployed AKS cluster
module "nginx-ingress" {
  source          = "../../modules/nginx_ingress"
  nginx_dns_label = module.eu1-aks-devtest-1.aks_cluster_name
}

Terraform will take care of dependency management and will only init helm provider after AKS cluster has been deployed, we can chain output from one resource as input to a provider.

You can imagine this working similarly with a SQL Server resource and this mssql provider.

magne commented 1 year ago

What's interesting here is where the module.eu1-aks-devtest-1.* attributes used in those providers come from. If they are output attributes from some resource in the ../../modules/aks module, that's really interesting.

jdelforno commented 1 year ago

I'd really like to see MSI as login as well, if possible. Execute the SQL commands using the MSI the agent is running on

PavelPikat commented 1 year ago

What's interesting here is where the module.eu1-aks-devtest-1.* attributes used in those providers come from. If they are output attributes from some resource in the ../../modules/aks module, that's really interesting.

@magne They are outputs from the module indeed :)

outputs.tf file

output "host" {
  value = azurerm_kubernetes_cluster.aks.kube_admin_config.0.host
}

output "base64_client_certificate" {
  value = base64decode(azurerm_kubernetes_cluster.aks.kube_admin_config.0.client_certificate)
}

output "base64_client_key" {
  value = base64decode(azurerm_kubernetes_cluster.aks.kube_admin_config.0.client_key)
}
magne commented 1 year ago

Yes, they are module outputs, but if you trace the values back through the resource graph, are they output attributes of a resource, or do they originate from variables (remember that resource input attributes are also present as output attributes, and as such are not truly output attributes).

PavelPikat commented 1 year ago

Yes, they are module outputs, but if you trace the values back through the resource graph, are they output attributes of a resource, or do they originate from variables (remember that resource input attributes are also present as output attributes, and as such are not truly output attributes).

They originate from the resources. E.g. the output will only return value after the resource has been provisioned. Terraform will wait with provisioning dependant resources that use helm/kubernetes providers because it knows it can't authenticate until the "parent" module returns output.

Theuno commented 1 year ago

Using an azuread_application_password resource results in the same behaviour. Rotating these on regular basis results in the same behaviour.

resource "time_rotating" "main-resource" {
  rotation_days = 30
}

resource "azuread_application_password" "main" {
  display_name = "GeneratedByInfrastructure"
  application_object_id = azuread_application.main.id
  rotate_when_changed = {
    rotation = time_rotating.main-resource.id
  }
  end_date_relative = "2160h"
}

The output of azuread_application_password.main.value can be used as password for the database connection. But this is not refreshed in the state after the rotation of the secret.

magne commented 1 year ago

Yes, they are module outputs, but are they true resource outputs. You will need to trace the values in the resource graph to see if they really are resource outputs (remember that resource inputs are also present in resource outputs, so you might have to trace an attribute through several resources to see if they originate as true outputs, or if they are, in fact, input variables).

karlschriek commented 1 year ago

You can use expressions in the values of these configuration arguments, but can only reference values that are known before the configuration is applied. This means you can safely reference input variables, but not attributes exported by resources (with an exception for resource arguments that are specified directly in the configuration).

This only becomes and issue if you create for example azurerm_sql_server in the same module as where you would use mssql_login because of the way terraform figures out the order of instantiating the providers. This is the case for pretty much any resource that you roll out which in turn has their own terraform provider you want to use.

A good example of this is to deploy azurerm_kubernetes_cluster and use the kubeconfig it outputs to instantiate a kubernetes or helm provider. As long as these two resources live in separate modules it isn't an issue. See for example https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs

There are also other patterns you can use for this though. For example, create azurerm_sql_server and write its admin creds to a key vault. In a separate terraform state, use a data resource to fetch those creds, instantiate the mssql provider with that and then create the mssql users.

By the way this is not true:

If the credentials were kept in the provider you would need a provider per database.

The only credentials that are kept in the provider are the ones you use in order to create additional users/logins. You could easily use different credentials here if you wanted, but most users will probably just initialize a single provider using the admin credentials, and then create all users/logins with that. If the admin credentials get rotated, you just initialize the provider with the new one. If nothing else changes, this then has no impact on the logins/users that you are managing. This is what we currently do with the fork we created off this project.