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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use max limit to conserve api quota in all cases #1775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

exitcode0
Copy link
Contributor

on data_source_okta_groups.go we were previously only using the maximum limit value if the q query parram was used, now it should be used in all cases

we were previously only using the maximum limit value if the `q`
query param was used, now it should be used in all cases
@monde
Copy link
Collaborator

monde commented Oct 23, 2023

okta/data_source_okta_groups.go didn't do pagination in it's original implementation and was never updated to do so correctly. Okta users has a better technique @exitcode0

https://github.com/okta/terraform-provider-okta/blob/master/okta/data_source_okta_users.go#L100-L102

https://github.com/okta/terraform-provider-okta/blob/master/okta/data_source_okta_users.go#L138-L152

@exitcode0
Copy link
Contributor Author

My intent here is not so much to resolve a pagination bug, but more that we're using more API calls than needed to retrieve a given cohort of groups

I'll tweak my PR to add pagination
But i'd also like to keep the higher max limit parameter value if possible to save on API requests, thoughts?

@monde
Copy link
Collaborator

monde commented Oct 24, 2023

Being able to set the default limit parameter https://github.com/okta/terraform-provider-okta/blob/master/okta/utils.go#L28C1-L28C1 should probably a setting on the provider itself and then have it be readable by any resource/datasource via the global config. However, that might be too generic. Or maybe something generic to the otka provider itself where any resource can have a setting similar to timeouts TF exposes in its config runtime https://developer.hashicorp.com/terraform/plugin/framework/resources/timeouts

@exitcode0
Copy link
Contributor Author

I thought about adding the parameter at the provider level, I even attempted a PR last night but was a bit out of my depth.

Some thoughts/questions regarding `limit

  1. Is there a objectively correct value for limit?
    or is it subjective and based upon use-case?
  2. Does Okta continue to slow down responses if you set a limit far above the max value
    can we just set the default limit to a sufficiently large number e.g 100000?
  3. Should we allow people to set this per endpoint?
    i.e make the provider argument a map.
  4. Should We allow people to set this as an argument on each Data_Source?
    This seems like a bad idea to me as removing all abstraction leaves us with a http provider re-implementation
  5. Is this a problem the Terraform should be solving?
    Is this better solved by okta-sdk-golang or okta-management-openapi-spec?

@monde
Copy link
Collaborator

monde commented Oct 25, 2023

Right, I think limit should probably be a provider config value that makes it into the global config. The limit value is already set to 200, which is a mirror of the same value the Okta API has for itself internally. So, let the operator choose their default. There must be a max, I don't know what it is, perhaps @exitcode0 inferred using Postman and found it to be 10K?

The best value depends on your use case. To that end, the best way to do it would be to have api limit be a setting on every data source and resource (like timeouts) but that would be a lot of by hand code changes, perhaps using buildSchema https://github.com/okta/terraform-provider-okta/blob/master/okta/utils.go#L30

given new code

// in utils.go?
var apiLimitSchema = map[string]*schema.Schema{
	"limit": {
		Type:        schema.TypeInt,
		Default:    200,
		Description: "API request objects limit",
	},
}

for example, the brand data source schema https://github.com/okta/terraform-provider-okta/blob/master/okta/data_source_okta_brand.go#L14-L23 would be this:

		Schema: buildSchema(
			map[string]*schema.Schema{
				"brand_id": {
					Type:        schema.TypeString,
					Required:    true,
					Description: "Brand ID",
				},
			},
			brandDataSourceSchema,
			apiLimitSchema
		),

but then each resource and data source would have to pass that value into the SDK client to use.

Having it as a global configuration would be much easier implement and be closer to what is desired.

@monde
Copy link
Collaborator

monde commented Sep 11, 2024

@arvindkrishnakumar-okta is going to follow up on this. The API docs are inconsistent. On the one hand it states the max is 10000 but the docs also recommend not using a value greater than 200 because of eventual consistency concerns.

https://developer.okta.com/docs/api/openapi/okta-management/management/tag/Group/#tag/Group/operation/listGroups!in=query&path=limit&t=request

@exitcode0
Copy link
Contributor Author

@arvindkrishnakumar-okta is going to follow up on this. The API docs are inconsistent. On the one hand it states the max is 10000 but the docs also recommend not using a value greater than 200 because of eventual consistency concerns.

https://developer.okta.com/docs/api/openapi/okta-management/management/tag/Group/#tag/Group/operation/listGroups!in=query&path=limit&t=request

I'd imagine the limit for the /groups endpoint is 10k as trying to retrieve all groups using pages of 200 items could take quite some time in large Okta instances (e.g >100k groups)

@arvindkrishnakumar-okta
Copy link
Contributor

arvindkrishnakumar-okta commented Sep 12, 2024

@exitcode0 I'm reading this issue and have a basic question - are you running into API rate limit quota soon (or are the results taking too long to page through multiple pages?) because of a lower limit value? In other words, is the motivation to increase it to a large number 10000 because of the fact that you are running into API rate limit issue and are trying to avoid it by doing so? Kindly clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants