pulumi / pulumi-azure-native

Azure Native Provider
Apache License 2.0
127 stars 34 forks source link

AzureTableStorageApplicationLogsConfigArgs.SasUrl should not be required in WebAppDiagnosticLogsConfiguration #608

Open WillooWisp opened 3 years ago

WillooWisp commented 3 years ago

The SasUrl should not be required below, but it fails if not specified. When importing resources with log level off it will fail, even if specifying empty string, since it will be marked as added.

AzureTableStorage = new AzureTableStorageApplicationLogsConfigArgs { Level = LogLevel.Off }

mikhailshilkov commented 3 years ago

but it fails if not specified

What exactly fails and with which error?

WillooWisp commented 3 years ago

I do not want to specify the SasUrl since I do not use table storage for logging, hence LogLevel.Off, but since you are requiring the SasUrl to be specified, not possible to leave unset, it will be marked as new and added and import fails. So you should just make sure that the SasUrl is not required.

mikhailshilkov commented 3 years ago

We take the metadata from the specs provided by Azure. SasUrl is marked as required in the Open API spec here. Do you want to open an issue there?

WillooWisp commented 3 years ago

But something must be wrong, since I am unable to import it as it is now, since I have no SasUrl in Azure, and specifying even an empty SasUrl in the args makes it think it differs from what's in Azure, even though it is not specified there.

mikhailshilkov commented 3 years ago

something must be wrong

The Open API spec could be wrong

WillooWisp commented 3 years ago

Okay, as it is now I am unable to import the state, and thus must fallback to the app service resource in the non-nextgen version of the azure plugin.

mikhailshilkov commented 3 years ago

Could you try specifying }, new CustomResourceOptions { IgnoreChanges = { "sasUrl" } }); for that resource and then trying to import?

WillooWisp commented 3 years ago

IgnoreChanges = { "applicationLogs.azureTableStorage.sasUrl" }

If I add it to ignore changes I get the error as well, same thing as not specifying it. error: azure-nextgen:web/v20200601:WebAppDiagnosticLogsConfiguration resource '' has a problem: missing required property 'applicationLogs.azureTableStorage.sasUrl'

mikhailshilkov commented 3 years ago

Yes, you still need to specify it, but at least you may be able to proceed with import

WillooWisp commented 3 years ago

I do that, still same error, ignoring "applicationLogs.azureTableStorage" or "applicationLogs.azureTableStorage.sasUrl" still gives the error above.

ApplicationLogs = new ApplicationLogsConfigArgs
  {
      AzureBlobStorage = azureBlobStorageApplicationLogsConfigArgs,
      AzureTableStorage = new AzureTableStorageApplicationLogsConfigArgs { Level = LogLevel.Off, SasUrl = "" },
      FileSystem = new FileSystemApplicationLogsConfigArgs { Level = LogLevel.Off }
  },
  HttpLogs = new HttpLogsConfigArgs
  {
      FileSystem = new FileSystemHttpLogsConfigArgs
      {
          RetentionInMb = 35
      }
  },
  Name = webAppName,
  ResourceGroupName = m_ResourceGroupName
}, importConfig != null ?
new CustomResourceOptions
{
  IgnoreChanges = { "applicationLogs.azureTableStorage" },
  ImportId = $"/subscriptions/{importConfig.SubscriptionId}/resourceGroups/{m_ResourceGroupName}/providers/Microsoft.Web/sites/{webAppName}/{resourceIdSuffix}"
} : null);
WillooWisp commented 3 years ago

And without ignore changes I get:

      ~ applicationLogs: {
          ~ azureTableStorage: {
              + sasUrl: ""
            }
        }
mikhailshilkov commented 3 years ago

Okay, I can reproduce this issue and have no ideas for good workarounds right now.

I was able to import the resource with the ancient version Pulumi.AzureNextGen.Web.V20150801.SiteLogsConfig which is the last one that doesn't mark SasUrl as required. It's not super useful as that resource is too old and not compatible with new shapes at all.

I raised an issue upstream https://github.com/Azure/azure-rest-api-specs/issues/12335

We'll see if we can improve the import process to work around such situations.

WillooWisp commented 3 years ago

Do they usually respond and fix any upstream issues that you report?

mikhailshilkov commented 3 years ago

Do they usually respond and fix any upstream issues that you report?

Sometimes... We try to get this improved, but so far the responsiveness wasn't great.

WillooWisp commented 3 years ago

Okay, so right now it does not seem possible to use this resource then?

mikhailshilkov commented 3 years ago

It's possible to use this resource. It doesn't seem to be possible to import this resource in your configuration. If you are open to temporarily change the settings in Azure, switch on the table storage logging and import it with SasUrl, then ignore changes and turn off table logging.

Otherwise, the only option would be to manually edit the state to import the resource.

WillooWisp commented 3 years ago

Table storage logging does not even seem possible to configure in Azure for a web app (app service), so I do not think that is even a supported option.

WillooWisp commented 3 years ago

What do you mean with "Otherwise, the only option would be to manually edit the state to import the resource." ?

WillooWisp commented 3 years ago

https://docs.microsoft.com/en-us/cli/azure/webapp/log?view=azure-cli-latest#az_webapp_log_show

Even here it is not possible to log to table storage, not an option. Can't you simply always ignore everything that has to do with table storage logging in your implementation of this resource?

mikhailshilkov commented 3 years ago

What do you mean with "Otherwise, the only option would be to manually edit the state to import the resource." ?

https://www.pulumi.com/docs/intro/concepts/state/#editing-state-manually

Basically, pulumi stack export >> state.json, edit state.json, cat state.json | pulumi stack import. Editing is tricky though, as you need to add a proper node of the proper shape to resources, likely by copy-pasting from another stack where a similar temporary resource is already created (without importing).

Can't you simply always ignore everything that has to do with table storage logging in your implementation of this resource?

The provider is ~fully driven by Open API specs with a minimal amount of manual code. So, an override is not simple, considering the maintenance costs that it brings.

If Azure publish this property, why would we remove it? How would we find other properties to remove? The Azure SDK includes it too, note how SasUrl is a required property there.

WillooWisp commented 3 years ago

Yes, and I think the SasUrl should be required if you configure and enable table storage for logging, which you cannot do. But the table storage itself then should not be required? So the AzureTableStorage property of ApplicationLogs should be possible to ignore, but that does not seem to be the case either.

If I ignore changes for applicationLogs.azureTableStorage it still complains about the sasUrl missing for azureTableStorage, but I want to ignore the azureTableStorage altogether.

WillooWisp commented 3 years ago

There seems to be different results using the import command through the pulumi cli and utilizing ImportId in code for each resource and doing a pulumi preview. The import cli command seem to handle the SasUrl correctly.

applicationLogs : { azureBlobStorage : { level : "Warning" retentionInDays: 30 sasUrl : "https://.blob.core.windows.net/logs?" } azureTableStorage: { level: "Off" } fileSystem : { level: "Off" } }

WillooWisp commented 3 years ago

@mikhailshilkov is the import functions totally different when using the import cli command and previewing when using ImportId in resources?

Is it true that the import cli command does compared to previewing with ImportId, does not at all consider and validate anything against the resources configured in code, they do not even have to exist?

mikhailshilkov commented 3 years ago

import cli does not at all consider and validate anything against the resources configured in code, they do not even have to exist?

yes, the idea of the CLI command is that you start from nothing and it will import the state + print the code for you to copy-paste into your program

WillooWisp commented 3 years ago

Okay, so this seems to work better for my scenarios, adopting Pulumi from previous use of ARM templates.

mikhailshilkov commented 3 years ago

That's good to hear because that CLI functionality was created for scenarios like your describe

devnev commented 3 months ago

@mikhailshilkov I'm running into this same issue, and I can't figure out how to proceed.

With the resource defined as it was originally created, I get the import diff

          ~ applicationLogs: {
              - azureTableStorage: {
                  - level: "Off"
                }
            }

With this original definition, ignoring changes to either of applicationLogs, applicationLogs.azureTableStorage gives the error azure-native:web:WebAppDiagnosticLogsConfiguration resource 'configlogs' has a problem: missing required property 'applicationLogs.azureTableStorage.sasUrl. Ignoring changes to applicationLogs.azureTableStorage.level gives the error Preview failed: cannot ignore changes to the following properties because one or more elements of the path are missing: "applicationLogs.azureTableStorage.level"

Adding properties to match the diff then gives a new diff,

          ~ applicationLogs: {
              ~ azureTableStorage: {
                  + sasUrl: ""
                }
            }

With this definition, ignoring changes to any of applicationLogs, applicationLogs.azureTableStorage or applicationLogs.azureTableStorage.sasUrl gives the error azure-native:web:WebAppDiagnosticLogsConfiguration resource 'configlogs' has a problem: missing required property 'applicationLogs.azureTableStorage.sasUrl.

mikhailshilkov commented 2 months ago

@devnev Did you try to import with the pulumi import command instead of the importId property? That's the only workaround I know of.

devnev commented 2 months ago

@mikhailshilkov thanks for the suggestion. Because I was moving resources between stacks, I ended up doing a stack export, used jq to massage the ids and then a stack import.

mikhailshilkov commented 2 months ago

@devnev Great to hear you are unblocked! Note that we now have a dedicated command to move resources between projects/stacks: https://www.pulumi.com/blog/move-resources-between-stacks/