hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.31k stars 9.49k forks source link

plan output being flooded by `use_microsoft_graph` deprecation warnings #31118

Closed Flasheh closed 2 years ago

Flasheh commented 2 years ago

Terraform Version

Terraform v1.2.0
on linux_amd64
+ provider [registry.terraform.io/gavinbunney/kubectl](http://registry.terraform.io/gavinbunney/kubectl) v1.14.0
+ provider [registry.terraform.io/hashicorp/azuread](http://registry.terraform.io/hashicorp/azuread) v2.22.0
+ provider [registry.terraform.io/hashicorp/azurerm](http://registry.terraform.io/hashicorp/azurerm) v3.7.0
+ provider [registry.terraform.io/hashicorp/helm](http://registry.terraform.io/hashicorp/helm) v2.5.1
+ provider [registry.terraform.io/hashicorp/kubernetes](http://registry.terraform.io/hashicorp/kubernetes) v2.11.0
+ provider [registry.terraform.io/hashicorp/random](http://registry.terraform.io/hashicorp/random) v3.2.0
+ provider [registry.terraform.io/hashicorp/time](http://registry.terraform.io/hashicorp/time) v0.7.2

Terraform Configuration Files

Not sure what configuration should be relevant here. Here is my terraform configuration block. Backend config is passed from file on cli,

terraform {
  required_version = ">=1.0.0, <2.0.0"

  required_providers {
    azurerm = {
      # https://github.com/terraform-providers/terraform-provider-azurerm
      source  = "hashicorp/azurerm"
      version = ">=3.0.0, <4.0.0"
    }

    .....
  }

  backend "azurerm" {
  }
}

provider "azurerm" {
  features {}
}

Issue

I recently upgraded my terraform to 1.2.0 and upgraded azurerm in all my states from v2.xx to v3.xx. Since then my plan output is being flooded with deprecation outputs for use_microsoft_graph as this is now defaulted to true. A single warning should be fine I suppose but something is spawning multiple of these and I am getting 5-10 warnings in my plan output

The warning: Warning: "use_microsoft_graph": [DEPRECATED] This field now defaults to 'true' and will be removed in v1.3 of Terraform Core due to the deprecation of ADAL by Microsoft.

Expected Behavior

I expect at most 1 deprecation warnings for use_microsoft_graph

Actual Behavior

I am getting multiple of these deprecation warnings, the amount varies per state and configuration

Steps to Reproduce

Run a terraform plan with terraform version 1.2.0 and using azurerm provider/backend

apparentlymart commented 2 years ago

Hi @Flasheh! Thanks for reporting this.

Do you think that what you are seeing is one warning for the backend configuration and one warning for the provider configuration? Those are two entirely separate components, so they do their own validation and consequent warning messages, and so I think one warning each is the minimum possible.

However, if you are seeing multiple warnings just from the Azure backend itself, ignoring the warnings from the provider, then that does seem like a definite bug.

Flasheh commented 2 years ago

I was thinking something similar at first, however the amount of warnings does not correlate with the amount of configuration blocks.

state 1:

state 2:

So there must be other components outputting these warnings

Flasheh commented 2 years ago

Could terraform_remote_state objects generate this type of warning? thats the only thing that comes to mind. But then the amount of warnings still does not add up. In my previous examples state 1 has 2 remote_state objects and state 2 has 4

apparentlymart commented 2 years ago

terraform_remote_state also configures the backend in the same way, so indeed it seems plausible to me that some of them are coming from there.

The Azure provider team maintains all of these components, so I'll leave this for them to respond now that we have a bunch of info in this thread about how your configuration is shaped. I'm not sure what will be possible to do here, but hopefully we can at least make sure that there is only one warning per configuration block that uses the deprecated argument.

tombuildsstuff commented 2 years ago

@Flasheh unfortunately I think this is more of a Core UX issue than something specific to the Backend unfortunately.

The reasoning for this warning is that this feature-flag is a temporary addition in 1.1, which has been flipped on in 1.2, and will be removed in 1.3 - whilst we don't envisage that the feature behind this change (switching to use MSAL rather than ADAL) will cause issues for users, as an abundance of caution we figured it was worth calling this out here incase there's some setups where this switch could be an issue.

Within the Azure Backend the use_microsoft_graph field is a boolean (which has an implicit default value of false) - however we're explicitly setting this to true if it's unspecified - and unfortunately I believe at this point in time there's no way for us to detect when a user has explicitly configured this field or not. This means that unfortunately this will be output every time, but since this is a breaking change we're removing in v1.3 (which users may be configuring, but we're unfortunately unable to detect) - whilst not a great UX we figured this was worth highlighting the deprecation.

Given that context - you could argue that this is a Terraform Core UX issue, rather than specific to the Backend - since Terraform Core should be able to determine when multiple (aliased) instances of a provider are present and returning the same deprecation - but I can also see the argument not to do that.

Since we're unable to determine whether a user has explicitly configured this field or not, I don't think there's much we can do here with regards to conditionally outputting this unfortunately (however if I'm wrong @apparentlymart let me know and we can take a look 😅) - instead I'm thinking it could be worth temporarily having an environment variable which muted this deprecation for the 1.2 release?

From a technical standpoint that'd be something along the lines of:

Deprecated: func() string {
  if v, ok := os.GetEnv("ARM_DISABLE_MSAL_DEPRECATION_NOTICE"); ok && v != "" {
    return ""
  }

  return "the deprecation notice"
}

That would allow users to be aware of this deprecation (since they'd have to set this environment variable to mute the deprecation), which I think would solve the problem from the Backend side.

With regards Terraform Core/CLI potentially condensing the same warning down, that'd be a feature enhancement at any rate, but perhaps @apparentlymart has some other suggestions there?

WDYT?

Flasheh commented 2 years ago

Thanks for the detailed response @tombuildsstuff

From a user perspective condensing these warnings to a single one seems like the proper way to solve this issue. I'm assuming it would catch similar cases in the future. I don't mind a warning even if I am aware of the deprecation but it obviously shouldn't output 10 of them.

With regards to this specific case, there is no broken functionality, it is only an eyesore in the output so I can live with it until the next minor release. A temporary toggle isn't necessary for me personally. Though it would be a nice-to-have if no other solution can be provided.

apparentlymart commented 2 years ago

Thanks for that extra context, @tombuildsstuff. I hadn't understood from the initial writeup that the backend is producing that error warning even if the argument isn't set at all; that does indeed seem unfortunate.

Looking at the backend implementation, I see that it's still using our built-in version of what eventually became the legacy SDKv2 for provider development, and so it's subject to the same design flaws as we often see in provider land where the SDK can't reliably distinguish between an argument being explicitly set to the zero value false or omitted altogether.

With that said though, since it looks like you're just using the built-in Deprecated feature here, rather than custom logic in the "configure" function, it does seem strange to me that it wouldn't already be handling this properly, in the same way as we'd expect for a deprecated argument in a provider configuration. Perhaps there was a later fix to the SDK made after we split it into its own repository that we could backport into legacy/helper/schema in order to make it behave correctly, and emit the warning only if use_microsoft_graph is explicitly set?

(We could probably avoid this problem entirely if we moved the backend away from using the legacy SDK, but that seems like a pretty heavy lift for a warning message that's destined to go away in the next minor release anyway, and would presumably make it harder to backport future changes from the Azure provider which is presumably also still using SDKv2, and thus has a similar sort of schema structure as the backend.)


The other thing I realize looking at this again with fresh eyes today is that we already have logic in Terraform CLI that, by default at least, tries to consolidate many similar warnings together into a single warning message. It seems like that isn't working for this particular message, but I'm not clear on why that is. I'm not equipped with either the credentials or knowledge to try this out for myself to see how this looks, but if one of you can share what these multipple warning messages actually look like I'd love to try to understand why the warning consolidation logic isn't working here and see if we can adjust the warnings to make it better match the grouping heuristic.

That would then at least reduce it to only a single message at the UI layer, with Terraform CLI annotating that message with the number of "similar messages elsewhere" it detected in the set of warning diagnostics.

apparentlymart commented 2 years ago

I did some initial digging into both SDKv2 and the older version of it we have embedded in Terraform to support these not-yet-updated backends. It looks like both of them handle the Deprecated schema field in the same way aside from the fact that SDKv2 was subsequently updated to properly support diagnostics, rather than just returning a legacy string error:

In both cases there's a conditional branch further up which returns early if the argument isn't set at all, so it isn't clear to me why we'd see this warning in cases where use_microsoft_graph is left entirely unset.

@tombuildsstuff do you see the equivalent argument in the Azure provider behaving in the same way? By which I mean: generating the warning even if the user didn't actually set the argument. I'm trying to determine if this is some oddity that has since been fixed in SDKv2 (and so hopefully we can find the fix and backport it) or if both of these helper/schema implementations have the same bug anyway, and thus I'm chasing a red herring here.

Flasheh commented 2 years ago

@apparentlymart

but if one of you can share what these multiple warning messages actually look like

You're looking for just the output without any debug info? Here's a real world example. (with anything identifying xxx'd out) https://gist.github.com/Flasheh/a8e6771937f09f2752b412e8ab45925a

tombuildsstuff commented 2 years ago

@tombuildsstuff do you see the equivalent argument in the Azure provider behaving in the same way? By which I mean: generating the warning even if the user didn't actually set the argument. I'm trying to determine if this is some oddity that has since been fixed in SDKv2 (and so hopefully we can find the fix and backport it) or if both of these helper/schema implementations have the same bug anyway, and thus I'm chasing a red herring here.

@apparentlymart from memory we also get these in SDKv2 as well (at least within the Provider block). Since we use a different Beta method in Provider-land (that is, the use of Environment Variables to opt-into the Beta of 3.0, containing this change, in 2.x) - we ended up using the 3.0 Beta toggle for the MS Graph switch, which has side-stepped this problem.

I'd agree longer-term there's probably value in switching this from the legacy SDK -> new SDK - but that'd still leave the underlying problem insofar as Terraform Core isn't correctly de-duping these duplicate deprecations?

If the easiest way to workaround this is to add the environment variable defined above to mute the deprecation then let me know and we can certainly add that, but it sounds like there's a couple of different bugs at play here (1. the legacy SDK used by the provider giving this implied default value & 2. Terraform Core not de-duplicating deprecations) which could warrant splitting this issue in two, I guess?

apparentlymart commented 2 years ago

Indeed, it does seem like there are two bugs to address here, though I would hesitate to split this just yet unless we move ahead with solving only one of them in response to this issue, which we will see.

I think the most immediate bug is that apparently the helper/schema package is emitting a deprecation warning for an argument that isn't actually set in the configuration, which seems to me to defeat the point of that deprecation mechanism: responding to the warning by doing what it suggests should always quiet the warning.

Separately we seem to have something "off" in Terraform CLI's warning consolidation heuristic, which seems a little less important if we can make the warning quietable, but if the work on this issue leads to us understanding why that is then it would be nice to fix that at the same time, though not required.

For the immediate concern here it seems like we will want to either fix the helper/schema bug described above or to work around it by implementing the warning in a different way. I suspect that this might be the only current example of us using the helper/schema deprecation mechanism in a legacy backend, and if that's true we can at least not worry about breaking any other backend in the process of fixing this bug.

Noel-Jones commented 2 years ago

I've read through this with interest since I have just come across the warning 3 times with no indication of where the offending field is set. That would surely be useful to know? I have never set the field so it does not appear anywhere in my code. So how do I remove it? I have just now for the first time upgraded from Terraform 1.1.8 to 1.2.2. I am guessing I need to set the field to true so that the state file is updated and then remove it?

Flasheh commented 2 years ago

@Noel-Jones

This feature can be set in the azurerm backend configuration, see here.

Prior to v1.2 this defaulted false, since v1.2 the default changed to true. And from v1.3 it will be removed. If you're using v1.2 there is no way to suppress the warnings AFAIK. So you'll either have to stick with it until v1.3 or downgrade <v1.2.

WaitingForGuacamole commented 2 years ago

Just a thought here - deprecation warnings to an end user should primarily target usage by an end user. If I don't use that property anywhere in my Terraform code, this message should be suppressable - tell me once, and then give me a switch to turn it off.

jack1902 commented 2 years ago

whilst it doesn't remove the warnings, something I have liked using to reduce the overall noise as a "stop gap" between <1.2 and 1.3 is adding -compact-warnings to terraform plan / terraform apply commands as it reduces it down to single lines rather than multiple.

Still has the warning but is significantly less verbose when there are multiple warnings (I'm using remote state in one of my terraform bits due to complexities with getting things up in azure)

timkowal commented 2 years ago

This should not appear at all, or be labeled "Information", or a "Notice" for awareness, unless the field is actually present.

AvtsVivek commented 2 years ago

FYI I raised a similar issue on SO here

crizzs commented 2 years ago

Same issue faced. Couldn't find the offending line on the resources. This warning ought to switch off.

apparentlymart commented 2 years ago

The forthcoming v1.3.0 release includes the end of the deprecation period for Azure AD Graph and so this option (including the deprecation warning) will be removed entirely in that version, and so I'm going to close this issue.

v1.3.0 is currently in its release candidate phase, so as long as we don't see any reports of critical problems from those who are testing the release candidate the v1.3.0 final release will be available soon. Once it's released, you can upgrade to v1.3.0 to quiet this warning.

Thanks!

github-actions[bot] commented 1 year ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.