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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Frontdoor compatible with XSkipDetailedDiffForChanges #1421

Closed
Tracked by #1936
t0yv0 opened this issue Nov 8, 2023 · 2 comments 路 Fixed by #2014
Closed
Tracked by #1936

Make Frontdoor compatible with XSkipDetailedDiffForChanges #1421

t0yv0 opened this issue Nov 8, 2023 · 2 comments 路 Fixed by #2014
Assignees
Labels
bug/diff kind/bug related to Pulumi generating wrong diffs on preview or up. kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@t0yv0
Copy link
Member

t0yv0 commented Nov 8, 2023

Hello!

  • Vote on this issue by adding a 馃憤 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

The Frontdoor resource needs tweaking to operate correctly with XSkipDetailedDiffForChanges.

Context: bridge is introducing a change to the diffing algorithm that is currently flagged under
XSkipDetailedDiffForChanges but is on the fast track to become turned on unconditionally as it
solves important issues for GCP and AWS provider. This change was introduced in:
pulumi/pulumi-terraform-bridge#1502

When pre-testing this change through Azure tests, TestAccFrontoor started failing. It is expected
that after a successful pulumi up, the subsequent pulumi preview generates a no-changes plan.
With XSkipDetailedDiffForChanges it started to generate an Update plan instead, based on these
attributes:

     CHANGING FROM DIFF_NONE to DIFF_SOME
    ~ "backend_pool_settings.#" => {1 0 false false <nil> false false 0}
    ~ "backend_pool_settings.0.backend_pools_send_receive_timeout_seconds" => {0 60 false false <nil> false false 0}

Investigating this further, the root cause was discovered. The
backendPoolsSendReceiveTimeoutSeconds setting is advertised as defaulting to 60 in the docs, but
this default value is not exposed through normal metadata in the upstream provider but enforced at
runtime. The code references here are:

It appears that this default value is applied on the read path. Since Pulumi differs from TF in not
automatically applying a Read upon every update, the initial pulumi up deployment fails to apply
the default, while the subsequent deployment does apply the default.

A possible mitigation here would be to lift the 60 default to the regular defaulting mechanism,
possibly by populating SchemaInfo.Default override for this property, which should make Pulumi apply
this default consistently and avoid unexpected plans.

Affected area/feature

@t0yv0 t0yv0 added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Nov 8, 2023
@mikhailshilkov mikhailshilkov added bug/diff kind/bug related to Pulumi generating wrong diffs on preview or up. and removed needs-triage Needs attention from the triage team labels Nov 8, 2023
VenelinMartinov added a commit to pulumi/pulumi-terraform-bridge that referenced this issue May 6, 2024
This reverts commit c57799f, reversing
changes made to e71c1a6.

reverts #1893

We have some downstream failures:

https://github.com/pulumi/pulumi-azure/actions/runs/8945276805/job/24576271881?pr=2001

https://github.com/pulumi/pulumi-github/actions/runs/8945161556/job/24576285007?pr=652

Some opened issues which look like prerequisites:
pulumi/pulumi-azure#1421 (related to the
failure above)
pulumi/pulumi-confluentcloud#264 (we don't
have credentials here, so likely why it did not produce a downstream
failure)

Anton also noted [some
issues](#1927)
with SkipDetailedDiff in AWS, so this change seems much less risky than
initially though.
@VenelinMartinov VenelinMartinov self-assigned this May 8, 2024
@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented May 8, 2024

Looks like this also happens in terraform:

terraform {
  required_providers {
    azurerm = {
      source  = "hashicorp/azurerm"
      version = "3.102.0"
    }
  }
}

# Configure the Microsoft Azure Provider
provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "example" {
  name     = "resourceGroup"
  location = "West Europe"
}

resource "azurerm_frontdoor" "example" {
  name                = "example476"
  resource_group_name = azurerm_resource_group.example.name

  routing_rule {
    name               = "rule1"
    accepted_protocols = ["Http", "Https"]
    patterns_to_match  = ["/*"]
    frontend_endpoints = ["frontEndEndpoint1"]

    forwarding_configuration {
      forwarding_protocol = "MatchRequest"
      backend_pool_name   = "exampleBackendBing"
    }
  }

  backend_pool_load_balancing {
    name = "exampleLoadBalancingSettings1"
  }

  backend_pool_health_probe {
    name         = "exampleHealthProbeSetting1"
    protocol     = "Https"
    probe_method = "HEAD"
  }

  backend_pool {
    name = "exampleBackendBing"
    backend {
      host_header = "www.bing.com"
      address     = "www.bing.com"
      http_port   = 80
      https_port  = 443
    }

    load_balancing_name = "exampleLoadBalancingSettings1"
    health_probe_name   = "exampleHealthProbeSetting1"
  }

  frontend_endpoint {
    name      = "frontEndEndpoint1"
    host_name = "example476.azurefd.net"
  }
}

I wonder if setting the property as computed would fix this, as the provider is giving a value for a non-computed property here.

@VenelinMartinov
Copy link
Contributor

Opened hashicorp/terraform-provider-azurerm#25911 upstream, will try to patch it on our side and ignore in the test if not.

VenelinMartinov added a commit to pulumi/pulumi-terraform-bridge that referenced this issue May 8, 2024
Fixes #1934

This is already in GCP and reverting it causes a bunch of test failures.

We need to push it to other providers.

Will address:
- [x]
https://github.com/pulumi/pulumi-azure/actions/runs/8945276805/job/24576271881?pr=2001
- [x]
https://github.com/pulumi/pulumi-github/actions/runs/8945161556/job/24576285007?pr=652
- [x] pulumi/pulumi-azure#1421
- [X] pulumi/pulumi-confluentcloud#264 - We do
not have credentials on this one. We can't test here.
VenelinMartinov added a commit that referenced this issue May 9, 2024
fixes #1421 by patching the
upstream resource to be computed as the provider sets a value for it.
This is needed for
pulumi/pulumi-terraform-bridge#1936

works around
hashicorp/terraform-provider-azurerm#25911

issue for patch: #2015

upstream PR:
hashicorp/terraform-provider-azurerm#25912
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/diff kind/bug related to Pulumi generating wrong diffs on preview or up. kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants