fivetran/terraform-provider-fivetran

An appeal for better adherence to semver (networking_method drifts from SshTunnel -> Directly for DB connectors)

jonathan-mothership opened this issue · 10 comments

jonathan-mothership commented

The recent release of 1.1.26 breaks existing connectors by adding new fields with default values that were not expected in the previous patch version. Patch updates should be fully compatible with previous configurations and in this case all 30 of our connections that rely on SSH tunnel config came up as needed updates when an unrelated change was planned.

  # fivetran_connector.postgres_cons["carrier-service"] will be updated in-place
~ resource "fivetran_connector" "postgres_cons" {
      ~ connected_by       = "stooping_germicide" -> (known after apply)
      ~ created_at         = "2024-03-27 01:14:17.0626 +0000 UTC" -> (known after apply)
      ~ id                 = "immigrant_reducing" -> (known after apply)
      ~ name               = "fivetran_carrier_service" -> (known after apply)
      ~ networking_method  = "SshTunnel" -> "Directly"
beevital commented

Patch updates should be fully compatible with previous configurations and in this case all 30 of our connections that rely on SSH tunnel config came up as needed updates when an unrelated change was planned.

It shouldn't make any changes in your configuration, seems like it's a bug on API side with this new field.

beevital commented

I've checked the upgrade from v1.1.25 to 1.1.26 and no issues with newly added field.
Looks like the problem is on API side, so API returned Directly instead of expected SshTunnel.
Could you please clarify what exact actions you were performing within version updarade. Did you change anything in config for your connetor?

pguinard-public-com commented

We've ran into the same problem when triggering an update to connectors when we are rotating passwords. This seems to be an issue on modifying connectors when terraform has not yet ran against 1.1.26. This seems to only be an issue on the initial apply and subsequent applies are fixed.

What we're seeing is this in the logs from the default being set to Directly rather than SshTunnel in https://github.com/fivetran/terraform-provider-fivetran/pull/320/files#diff-52e9d8a9d0f852892eab03543fae8578437e4a9e67450eacb320f7ca905503d7R108-R112

When applying changes to fivetran_connector.aurora["xxxxxx"], provider "provider[\"registry.terraform.io/fivetran/fivetran\"]" produced an unexpected new value: .networking_method: was cty.StringVal("Directly"), but now cty.StringVal("SshTunnel").

Verify that method is SSHTunnel:

curl -u ${FIVETRAN_APIKEY}:${FIVETRAN_APISECRET} -XGET https://api.fivetran.com/v1/connectors/YOUR_CONNECTOR | jq .

  1. Set connector to 1.1.25
  2. terraform init/apply
  3. Make changes to connectors / terraform apply to verify everything is working.
  4. Upgrade provider to 1.1.26
  5. Make changes to connectors
  6. Terraform plan (looks normal)
  7. Terraform apply (Run into the same issue)
pguinard-public-com commented

There are currently 2 ways to define this property, and that's not great from our side and should likely be looked as a replacement rather than having 2 at the same time. I'm not privy to the roadmap, is the plan to move away from tunnel_host in favor of Directly?

beevital commented

Looks like the problem is actually on API side. The problem is that when you applying changes inside connection credentials - it triggers a branch that updates connection method incorrectly, so you have a "drifting changes" with the new field introduced in 1.1.26
We will fix it on API side.

  • 👍2
itssimon commented

We're also hitting this problem. Any updates on a fix?

pguinard-public-com commented

For the short term you can switch away from pessimistic versioning, which is the root problem of this dicsussion. https://developer.hashicorp.com/terraform/language/expressions/version-constraints#version-constraint-syntax

  • 👍1
beevital commented

@itssimon

We're also hitting this problem. Any updates on a fix?

Not yet. As I said - the problem is on API side. We are working on it.

beevital commented

The issue with the drifting changes for networking_method was fixed on API side, default value for this field removed from provider schema as well.
@itssimon @pguinard-public-com please check if you're observing the issue still?
I'm going to close the issue.

pguinard-public-com commented

I would agree that the bug in the provider that caused this discussion has been fixed on version 1.2.3.

The process side may require an internal discussion on the release team should happen around release versioning per @jonathan-mothership's original request. https://semver.org/ does a great job of describing what is expected for versioning moving forward.

Given a version number MAJOR.MINOR.PATCH, increment the:

    MAJOR version when you make incompatible API changes
    MINOR version when you add functionality in a backward compatible manner
    PATCH version when you make backward compatible bug fixes

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

With the intent of this change (ignoring the fact that this was a breaking change, it happens) the version that I would expect would be the MINOR number instead of the PATCH number. The way that symantec versioning suggests is moving from 1.1.25 to 1.2.0 instead of from 1.1.25 to 1.1.16 as this is was intended to be a backwards compatible feature rather than a breaking change.

  • 🎉1