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] Terraform Destroy Does Not Delete AS3 Tenant #263

Open RavinderReddyF5 opened 4 years ago

RavinderReddyF5 commented 4 years ago

Issue by El-Coder Monday Jan 13, 2020 at 16:24 GMT Originally opened as https://github.com/terraform-providers/terraform-provider-bigip/issues/232


When I run Terraform Destroy it does not delete tenant from BIG-IP.


example.json

{
   "class": "AS3",
   "action": "deploy",
   "persist": true,
   "declaration": {
      "class": "ADC",
      "schemaVersion": "3.0.0",
      "id": "urn:uuid:33045210-3ab8-4636-9b2a-c98d22ab915d",
      "label": "Sample 1",
      "remark": "Simple HTTP application with RR pool",
      "Sample_01": {
         "class": "Tenant",
         "A1": {
            "class": "Application",
            "template": "http",
            "serviceMain": {
               "class": "Service_HTTP",
               "virtualAddresses": [
                  "10.0.1.10"
               ],
               "pool": "web_pool"
            },
            "web_pool": {
               "class": "Pool",
               "monitors": [
                  "http"
               ],
               "members": [{
                  "servicePort": 80,
                  "serverAddresses": [
                     "192.0.1.10",
                     "192.0.1.11"
                  ]
               }]
            }
         }
      }
   }
} 

Terraform Code

provider "bigip" {
  address = "$*.*.*.*"
  username = "admin"
  password = "*******"
}

resource "bigip_as3"  "as3-example" {
     as3_json = "${file("example.json")}"
     config_name = "sample_test"
 }
RavinderReddyF5 commented 4 years ago

Comment by papineni87 Monday Jan 13, 2020 at 16:55 GMT


@El-Coder can you clone the latest code which has the fix for above issue

RavinderReddyF5 commented 4 years ago

Comment by erjac77 Thursday Jan 16, 2020 at 21:10 GMT


@El-Coder Maybe I got it all wrong, but I think the config_name should reflect the Tenant name. As a matter of fact, the config_name variable was called tenant_name in previous versions. In your case, simply replace sample_test by Sample_01 in your resource and you should be fine.

@papineni87 In your PR #233, you removed the tenant name from the /declare endpoint. This means that all tenants will be deleted, instead of the ones specified in the declaration.

https://clouddocs.f5.com/products/extensions/f5-appsvcs-extension/latest/refguide/as3-api.html#delete-ref

DELETE examples:

DELETE https://192.0.2.10/mgmt/shared/appsvcs/declare

removes all tenants

DELETE https://192.0.2.10/mgmt/shared/appsvcs/declare/T1,T2,T5

removes Tenants T1, T2, and T5 leaving the rest of the most recent declared configuration for localhost in place (assuming there are other Tenants, such as T3 and T4).

I think you should consider reverting your changes, since they could have disastrous effects on others Tenants, and maybe rename back the config_name variable to tenant_name.

An other option would be to parse the JSON file, retrieve all the Tenants and append them (comma separated) to the /declare endpoint, as shown in the example above.

Like I said, I may be wrong, but that's what I understand when I look at the code.

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Friday Jan 17, 2020 at 16:53 GMT


@erjac77 config_name used in bigip_as3 resource doesn't reflect tenant name, it is arbitrary name used for uniquely identify bigip_as3 resource.

https://www.terraform.io/docs/providers/bigip/r/bigip_as3.html

as per PR #233 , we removed the tenant name which is actually just arbitrary name rather actual tenant name.

may be it is good approach to try to fetch available tenants and delete same.

RavinderReddyF5 commented 4 years ago

Comment by erjac77 Friday Jan 17, 2020 at 17:21 GMT


@RavinderReddyF5 Yes I know. But what I meant, is that it used to be the Tenant name (the variable name has recently changed). And removing the name from the /declare endpoint will delete all Tenants. Not sure this is a good idea and probably not what the user would want or expect.

Sorry if I was not clear, my english is not very good.

RavinderReddyF5 commented 4 years ago

Comment by strantalis Friday Jan 17, 2020 at 19:25 GMT


We submitted this issue #166 and fix #167 so it wouldn't delete all the tenants.

I agree with the two options @erjac77 proposed.

I think you should consider reverting your changes, since they could have disastrous effects on others Tenants, and maybe rename back the config_name variable to tenant_name.

An other option would be to parse the JSON file, retrieve all the Tenants and append them (comma separated) to the /declare endpoint, as shown in the example above.

RavinderReddyF5 commented 4 years ago

Comment by gageorsburn Saturday Jan 18, 2020 at 03:34 GMT


Yeah... I would revert PR #233 immediately. That is a very serious bug. E.g. we discovered the behavior accidentally by running a terraform destroy against a test environment which deleted production rules which caused a traffic outage... The problem is that variable got renamed, or which raises another big problem.. there is no unit test for delete in https://github.com/gageorsburn/terraform-provider-bigip/blob/master/bigip/resource_bigip_as3_test.go (which would have prevented the variable name PR from going through until the code in AS3 deletes was updated preventing the bug).

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Monday Jan 20, 2020 at 05:45 GMT


@erjac77 @strantalis @gageorsburn thanks for your Solution option proposed. i think it is better to proceed with below option.

"An other option would be to parse the JSON file, retrieve all the Tenants and append them (comma separated) to the /declare endpoint"

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Monday Jan 20, 2020 at 09:55 GMT


@gageorsburn @erjac77 @strantalis please pull the latest master branch code from terraform repo and check.

Updated with required changes as suggested in Option1, and updated document on how to use this resource. please refer below link for how to use resource.

https://github.com/terraform-providers/terraform-provider-bigip/blob/stable-website/website/docs/r/bigip_as3.html.markdown

RavinderReddyF5 commented 4 years ago

Comment by jakauppila Friday Feb 28, 2020 at 20:46 GMT


We were planning to start utilizing the provider to manage our F5 appliances with the bigip_as3 resource, but commits like https://github.com/terraform-providers/terraform-provider-bigip/pull/233/commits/43eec995cc1118398076f333f5481e391d9e6f4a raises cause for concern towards the level of extreme caution we would have to take both in its operation and upgrades.

What sort of additional oversight can be added to prevent such changes? Had a release been pushed, that could have been devastating to an organization.

RavinderReddyF5 commented 4 years ago

Comment by papineni87 Saturday Feb 29, 2020 at 12:44 GMT


@jakauppila we had reverted the above mentioned commit in master branch, and we are trying for new release with new design and also set of testcases for new features along with their results to avoid above kind of issues.

RavinderReddyF5 commented 4 years ago

Comment by papineni87 Thursday Mar 19, 2020 at 09:46 GMT


Fixed in latest release v1.1.2

RavinderReddyF5 commented 4 years ago

Comment by writemike Wednesday Apr 08, 2020 at 13:26 GMT


Just an FYI in case anyone else hits an issue similar to this one: As I was testing the example provided above, I noticed that if the tenant name contains a dash, you can apply the AS3 declaration, but not destroy it. So before you think of blaming terraform-providers/terraform-provider-bigip have a look at: https://github.com/F5Networks/f5-appsvcs-extension/issues/247

"AS3 3.17.0 and later allows dots and hyphens in Tenant and Application names." Source: https://clouddocs.f5.com/products/extensions/f5-appsvcs-extension/latest/refguide/declaration-purpose-function.html#naming-ref

RavinderReddyF5 commented 4 years ago

Comment by papineni87 Wednesday Jun 10, 2020 at 05:43 GMT


Please verify with latest relese, If anyone see the issue again, please feel free to open new issue