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

Add ignore client error errors parameter #271

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phillebaba
Copy link
Contributor

There are situations when resources are conditionally created, meaning that the provider is not actually used even if it is present in the Terraform HCL. This is common if someone is using conditional modules that are only imported if a feature is enabled. The current provider init method creates a client object with the given PAT. The issue with doing this is that when the client is created it will verify the PAT and return an error if it is invalid.

So if a Azure DevOps resource is present but has a count of 0 the provider will still require that a valid PAT is present. Without being able to ignore this error it becomes impossible to create a generic module that includes resources from this provider as it will always require the PAT.

This behavior is not present in other providers which defer the validation of the token to when any of the CRUD functions are called on a function, allowing for the module design described above.

This change will not break any existing deployments as it defaults to false, the normal behavior, so it is an opt in feature.

@xuzhang3
Copy link
Collaborator

@phillebaba Add a custom configuration to ignore the initiation error is not good. If you don't want ADO provider, remove it from your configuration. When initiation error occurs, user should handle the error not ignore it. The ignore configuration may mislead the users and took more time to find out what is wrong.

@phillebaba
Copy link
Contributor Author

Well the issue is that at times I do not have the option to remove the provider. Basically if I include a resource from the AzDO provider which has a count of zero, Terraform will run the init command for the provider. If I omit the provider config it will fail as the PAT is empty and therefor invalid.

This will not work for the Azure DevOps provider.

resource "azuredevops_project" "project" {
  count = 0

  name       = "Project Name"
  description        = "Project Description"
}

This issue only exists for the AzDO provider as it runs client validation when the provider is initiated. All other providers will not error until it actually has to perform a CRUD action on one of the resources if the credential is invalid.

@xuzhang3
Copy link
Collaborator

As this is workaround in your scenario. Can you set the AZDO_ORG_SERVICE_URL and AZDO_PERSONAL_ACCESS_TOKEN with dummy values? This should works for you.

@phillebaba
Copy link
Contributor Author

No that does not work as as when the client is created it will verify that the token is valid.

This will error if the token is invalid.

client, err := client.GetAzdoClient(d.Get("personal_access_token").(string), d.Get("org_service_url").(string), terraformVersion)

@xuzhang3
Copy link
Collaborator

Hi @phillebaba Unless you have azuredevops configurations, otherwise the ADO client won't be initialized(tested with terraform v0.13 and v0.12).

@phillebaba
Copy link
Contributor Author

I think the easiest way to explain this is to first show the use case I have and then compare this provider with the GitHub provider. This module here can be conditionally enabled by a boolean variable. If the module is enabled with will include Azure DevOps resources. If it not enabled the count of the module will be zero and none of the resources in the module will be used.
https://github.com/XenitAB/terraform-modules/blob/c46858c04f7e18012806d33581188b9e940064de/modules/kubernetes/aks-core/modules.tf#L58

If we simplify the problem what we are doing is basically declaring a resource with zero instances. Doing so with a simple GitHub provider would look like this.

terraform {
  required_providers {
    github = {
      source = "integrations/github"
      version = "4.3.0"
    }
  }
}

resource "github_repository" "example" {
  count = 0

  name        = "example"
  description = "My awesome codebase"
  visibility  = "public"
  template {
    owner = "github"
    repository = "terraform-module-template"
  }
}

Running plan will result as expected in no changes in the plan.

$ terraform init
$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.


------------------------------------------------------------------------

No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.

If we now do the same with a simple Azure DevOps example we would expect the same result right?

terraform {
  required_providers {
    azuredevops = {
      source = "microsoft/azuredevops"
      version = "0.1.1"
    }
  }
}

resource "azuredevops_project" "project" {
  count = 0
  name       = "Project Name"
  description        = "Project Description"
}

Running plan with this example will result in an error.

$ terraform init
$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.


------------------------------------------------------------------------

Error: the personal access token is required

  on <empty> line 0:
  (source code not available)

Adding a fake PAT and ORG url will result in a different error.

Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.


------------------------------------------------------------------------

Error: TF400813: The user '' is not authorized to access this resource.

  on main.tf line 10, in provider "azuredevops":
  10: provider "azuredevops" {

@xuzhang3
Copy link
Collaborator

Hi @phillebaba I can set the dummy values and run plan&apply . How do you set the dummy values? I can reproduce your error when I remove the count = 0.
Test script:

terraform {
  //  required_version = ">= 0.13"
  required_providers {
    azuredevops = {
      source = "microsoft/azuredevops"
      version = "=0.1.0"
    }

    azurerm = {
      source = "hashicorp/azurerm"
      version = "=2.41.0"
    }
  }
}

provider "azurerm" {
  features {}
  version = "=2.41.0"
}

resource "azuredevops_project" "project" {
  count = 0
  name       = "Project Name"
  description        = "Project Description"
}

Env values:

AZDO_ORG_SERVICE_URL=https://dev.azure.com/xuzhang87  
AZDO_PERSONAL_ACCESS_TOKEN=zo5fg3gf2z672s2q3xpu

Result :
image

@phillebaba
Copy link
Contributor Author

So this error seems to be dependent on the org settings. My guess is that there is some minimal public permissions that have to be enabled in the org.

Could you try using export AZDO_ORG_SERVICE_URL=https://dev.azure.com/xenitab, because it would only work if I had a valid PAT set.

@xuzhang3
Copy link
Collaborator

Hi @phillebaba You are right, the PAT need a minimal public permissions, can you generate a minimal permission PAT? My understanding is the PAT must be a real token and have access to the ORG(any permission should be OK).
Ignore the init error is not the best choice , lazy initialization is better.

@phillebaba
Copy link
Contributor Author

The issue in my case is that I am importing a module which I am disabling with an option. Meaning I have no use for a PAT, and the provider should not force me to have one if I am not using any of the resource or datasources. This is not an unusual thing to do in Terraform, to have optional resources. In all other cases it works fine as the provider does not require any configuration if resources are not used. This is only this provider that behaves in this manner, so I don't understand why anyone would want this provider to behave in the exact opposite way to all other providers that are available?

@arlobelshee
Copy link

Please accept this PR. I need it for a different reason: unit testing.

I want to write tests for my ADO config code. They run against a plan output. When run on CI, I cannot have a PAT - that would be a security hole in our model. With any other provider, I just run terraform plan -refresh=false to generate my plan - I want it to always run against a state of my choosing anyway, regardless of what might exist in production.

This provider fails because it requires a valid PAT and attempts to log in, even though I'm attempting to explicitly disable any API usage. End result: I can't unit test any code that uses this provider, though I can with all other providers.

Please apply this PR, or make another one that completely disables all use of the client.

@arlobelshee
Copy link

Hi @phillebaba You are right, the PAT need a minimal public permissions, can you generate a minimal permission PAT? My understanding is the PAT must be a real token and have access to the ORG(any permission should be OK). Ignore the init error is not the best choice , lazy initialization is better.

I agree that lazy init is optimal. But it needs to be fully lazy: don't even try to connect until you know you need to make an API call. And you shouldn't need such a call for terraform init, or terraform plan -refresh-false. You only need to check credentials at all when you actually want to read or set state on real resources.

@slushysnowman
Copy link

Yeah this is actually an issue, I've never encountered it with any other provider.

We have this exact same use case, we want to do a for_each on a module that contains azuredevops resources. But we have cases where the for_each will do nothing (by intention) as there are some cases where we CANNOT easily pass a valid PAT into Terraform.

If the azuredevops provider is not being used, then the 'config' should not be validated in any way, this is how pretty much all other providers I've encountered work.

@slushysnowman
Copy link

@phillebaba did you find a way around this or did you give up?

@phillebaba
Copy link
Contributor Author

I have created a new Terraform provider to fill my needs with git, so I am migrating away from this Terraform provider.

https://github.com/XenitAB/terraform-provider-git

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants