RavinderReddyF5 / terraform-provider-bigip-version0.12

Terraform resources that can configure F5 BIGIP products
Mozilla Public License 2.0
0 stars 0 forks source link

[CLOSED] Not possible to remove persistence profile for a Virtual Server #200

Open RavinderReddyF5 opened 4 years ago

RavinderReddyF5 commented 4 years ago

Issue by dannyk81 Tuesday Oct 15, 2019 at 22:28 GMT Originally opened as https://github.com/terraform-providers/terraform-provider-bigip/issues/169


Provider version: 0.12.3 BigIP: 12.1.1

We have a set of Virtual Servers created with /Common/source_addr persistence profile, config:

resource "bigip_ltm_virtual_server" "fpx_vip" {
  name = format(
    "/%s/%s",
    var.bigip_dmz_partition,
    var.bigip_fpx_virtual_server_name,
  )
  destination   = var.fpx_vip
  ip_protocol   = "tcp"
  port          = 8080
  pool          = bigip_ltm_pool.fpx_pool[0].name
  vlans         = [bigip_net_vlan.bigip01_dmz_vlan[0].name]
  vlans_enabled = true

  profiles = ["/Common/tcp"]

  irules = [
    bigip_ltm_irule.enable_proxy_protocol[0].name,
  ]

  persistence_profiles = ["/Common/source_addr"]

  source_address_translation = "snat"
  snatpool = bigip_ltm_snatpool.dmz_snatpool[0].name
}

We decided we want to remove the persistence profile for the VS, so at first we removed persistence_profiles = ["/Common/source_addr"] from the config, that resulted in a "No Change" plan, so we set persistence_profiles = [] and got this plan:

  # bigip_ltm_virtual_server.fpx_vip will be updated in-place
  ~ resource "bigip_ltm_virtual_server" "fpx_vip" {
        client_profiles            = []
        destination                = "x.x.x.x"
        id                         = "/ABC/fpx_vip"
        ip_protocol                = "tcp"
        irules                     = [
            "/Common/enable_proxy_protocol",
        ]
        mask                       = "255.255.255.255"
        name                       = "/ABC/fpx_vip"
      ~ persistence_profiles       = [
          - "/Common/source_addr",
        ]
        policies                   = []
        pool                       = "/ABC/fpx_pool"
        port                       = 8080
        profiles                   = [
            "/Common/tcp",
        ]
        server_profiles            = []
        snatpool                   = "/ABC/dmz_snat_pool"
        source                     = "0.0.0.0/0"
        source_address_translation = "snat"
        translate_address          = "enabled"
        translate_port             = "enabled"
        vlans                      = [
            "/ABC/dmz_vlan",
        ]
        vlans_enabled              = true
    }

this looked good, so we applied only to find that nothing changed on the VS:

snippet from https://<bigip>/mgmt/tm/ltm/virtual/~ABC~fpx_vip after the apply:

"persist": [
    {
        "name":"source_addr",
        "partition":"Common",
        "tmDefault":"yes",
        "nameReference": {
            "link":"https://localhost/mgmt/tm/ltm/persistence/source-addr/~Common~source_addr?ver=12.1.1"
        }
    }
],

I didn't look at the code here yet, but looks like it's not handling things correctly when the list is empty.

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Wednesday Oct 16, 2019 at 01:29 GMT


Looking at the current implementation, I see a couple of issues:

1) The resource allows to define a set of persistence profiles, however no way to indicate which one is the default persistence profile, if one profile is defined - it becomes the default, however of multiple profiles were defined - none are set as default.

"persist": [
    {
        "name": "hash",
        "partition": "Common",
        "tmDefault": "no",
        "nameReference": {
            "link": "https://localhost/mgmt/tm/ltm/persistence/hash/~Common~hash?ver=12.1.1"
        }
    },
    {
        "name": "source_addr",
        "partition": "Common",
        "tmDefault": "no",
        "nameReference": {
            "link": "https://localhost/mgmt/tm/ltm/persistence/source-addr/~Common~source_addr?ver=12.1.1"
        }
    }
],

2) Profiles can be added and removed from the set as long as at least one profile is defined, however it is not possible to remove all the persistence profiles.

3) The code currently doesn't detect changes in the persistence profiles, I set the configuration to persistence_profiles = ["/Common/hash"] and modifed the virtual server in bigip to Common/source_addr, the plan returned No changes. Infrastructure is up-to-date.

I didn't dig deep enough yet, but one thing that strikes me as weird is that PersistenceProfile uses the Profile struct: https://github.com/f5devcentral/go-bigip/blob/87e86421b613acc20e776c6b5a3ff41de3dc631d/ltm.go#L540

https://github.com/terraform-providers/terraform-provider-bigip/blob/e2f2c75042e34c0b26e1426fd3c24f83d96bf281/bigip/resource_bigip_ltm_virtual_server.go#L370

which doesn't seem to match the persist sub-collection struct returned by the API.

/cc @scshitole @RavinderReddyF5 @papineni87

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Wednesday Oct 16, 2019 at 01:49 GMT


using the following curl command I was able to clear the persistence profile:

$ curl -sku admin:***** https://<bigip>/mgmt/tm/ltm/virtual/~ABC~fpx_vip -H "Content-Type: application/json" -X PATCH -d '{"persist": []}'
RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Wednesday Oct 16, 2019 at 13:21 GMT


@dannyk81 Looks like there are multiple issues in persistance_profile implementation, we are able to address issue(3) in above comments, we are working to fix all of them and update you.

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Wednesday Oct 16, 2019 at 13:34 GMT


Thanks @RavinderReddyF5

RavinderReddyF5 commented 4 years ago

Comment by papineni87 Wednesday Nov 06, 2019 at 09:37 GMT


@dannyk81 can you retry the above scenario with the latest code changes, if it works we can close this issue ?

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Wednesday Nov 06, 2019 at 16:24 GMT


sure @papineni87, was this fix included in v1.0.0?

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Wednesday Nov 06, 2019 at 16:27 GMT


@dannyk81 No,It will be Available for Next Release version, but changes are available in repo

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Wednesday Nov 06, 2019 at 16:39 GMT


ok, I'll build from master and test.

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Thursday Nov 07, 2019 at 15:47 GMT


@papineni87 I just did a few test:

1) setting persistence_profiles = [] indeed removes the profile from the VS.

2) the provider still doesn't detect changes done in the F5, for example if I set persistence_profiles = ["/Common/hash"] apply and then change in F5 (manually) to /Common/source_addr a plan/apply shows that everything is up to date which is incorrect of course.

with (2) I would consider this still as broken since the provider can't detect configuration drifts.

I think we should also add some tests around this.

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Thursday Nov 21, 2019 at 05:39 GMT


Issue1 : The resource allows to define a set of persistence profiles, however no way to indicate which one is the default persistence profile, if one profile is defined - it becomes the default, however of multiple profiles were defined - none are set as default.

Addressed in :https://github.com/terraform-providers/terraform-provider-bigip/pull/183

Issue2: Profiles can be added and removed from the set as long as at least one profile is defined, however it is not possible to remove all the persistence profiles.

Addressed in :https://github.com/terraform-providers/terraform-provider-bigip/pull/179

Issue3: The code currently doesn't detect changes in the persistence profiles, I set the configuration to persistence_profiles = ["/Common/hash"] and modifed the virtual server in bigip to Common/source_addr, the plan returned No changes. Infrastructure is up-to-date

Addressed in :https://github.com/terraform-providers/terraform-provider-bigip/pull/204

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Wednesday Nov 27, 2019 at 01:26 GMT


@dannyk81 we have addressed above outlined Issues, Closing this ticket as release is already out. please open new issues for any other issues if you see.