netskopeoss / terraform-provider-netskope

Apache License 2.0
6 stars 5 forks source link

netskope_privateapps does not respect api rate limits #2

Closed Taoquitok closed 1 year ago

Taoquitok commented 1 year ago

When attempting to create / update / delete more than a couple of private apps at any one time, the netskope_privateapps resource throws the below API rate limit exceeded error:

│ Error: API rate limit exceeded │ │ with module.netskope_fqdn_privateapps["PLACEHOLDERDOMAIN"].netskope_privateapps.privateapp, │ on privateapp-module\main.tf line 24, in resource "netskope_privateapps" "privateapp": │ 24: resource "netskope_privateapps" "privateapp" { │

For the creation/deletion of private apps you can re-run the terraform apply until all apps are created, so though annoying, in this case the issue fails safe.

This is not true for private app updates.
If you're updating an existing app (e.g. updating port list), when the rate limit is hit the provider will update the state as if the change was successful, even when not.
This isn't appropriate terraform behaviour as the mismatch negates the whole purpose of using terraform.

Relevant resource reference from module example:

resource "netskope_privateapps" "privateapp" {
  app_name                = local.validated_app_name
  host                    = join(", ", var.hosts)
  use_publisher_dns       = var.publisher_dns
  clientless_access       = var.clientless_access
  trust_self_signed_certs = var.trust_self_signed_certs

  dynamic "protocols" {
    for_each = var.protocol_map
    content {
      type = protocols.key
      port = join(", ", protocols.value)
    }
  }

  dynamic "publisher" {
    for_each = var.publisher_list
    content {
      publisher_id   = publisher.value.id
      publisher_name = publisher.value.name
    }
  }
}

Environment information:

PS C:\git\netskope_config> terraform -v
Terraform v1.3.2
on windows_amd64
+ provider registry.terraform.io/netskopeoss/netskope v0.2.1
ns-sbrown commented 1 year ago

@Taoquitok Thanks for reporting this. I should be able to take a closer look in the next few days and I will let you know what to expect for a timeline on fixing once I dig into the issue.

Taoquitok commented 1 year ago

@ns-sbrown Thanks for checking. Good to know the issues are monitored too.

Before I commit some development time to it, are you open to pull requests too? Though I may not have time to investigate this specific issue, there's other minor bugs/inconsistent behaviours that I'm interested in improving, and at least one resource I'd like to add to manage the refreshing of publisher API tokens that would be beneficial to using this provider at scale. (All assuming I'm not stepping on the toes of existing in-house developement plans?)

ns-sbrown commented 1 year ago

@Taoquitok In general I am am open to pull requests. If it's a small fix or something quick and easy please go ahead and submit a pull request and I will happily review. If it is a larger effort on your part, I'd reach out and coordinate just to make sure you aren't duplicating effort.

On this specific issue, I'd hold off as I think I know how I want to handle this. In short I am likely going to fix this in the nsgo client by implementing retry logic. This way other project which use the client will also benefit. This will probably take me a few weeks between implementation and testing but I am close to having a plan ready to go. I do still need to investigate why you are seeing the issue are for what should be a failure in Apps so this may be a separate effort though may be masked when retry logic gets added.

ns-sbrown commented 1 year ago

@Taoquitok I released v0.2.2 this morning which uses new retry logic from the client. In my testing this has resolved issues with rate limiting. Would be good to get feedback before I close this issue.

Taoquitok commented 1 year ago

@ns-sbrown

Thanks for the quick change. I've run a few applies to add/remove ports to 20 apps a few times and it's consistently made the change. No noticeable slowdown either, so the behaviour is looking good.

I noticed there's an additional change to include tags? I assume this is a new feature that'll come to private apps? Is it something I could start including without risk, or best to wait?

ns-sbrown commented 1 year ago

Great to hear and thanks for testing. Tag functionality is partially implemented in the product right now so I slipped some support into the provider. That said this is not turned on in all tenants yet, so I'd probably hold off a bit.