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

Get latest cloudflare-go and update paginations #443

Merged
merged 8 commits into from
Aug 18, 2022

Conversation

tamas-jozsa
Copy link
Contributor

@tamas-jozsa tamas-jozsa commented Aug 2, 2022

Updating cloudflare-go to v0.46.0 and use paginated APIs for filter, firewall-rule and rate-limit resources.

Furthermore, I had to fix the following test tf resources to make go test work:

  • cloudflare_access_application_simple_account/test.tf
  • cloudflare_access_application_with_cors_account/test.tf
  • cloudflare_access_rule_account/test.tf

Related issue cloudflare/terraform-provider-cloudflare#1819 about:

  • cloudflare_account_member/test.tf

I'm also a bit puzzled, why go vet is failing during test action.

@jacobbednarz I'd like to ask your guidance on the failing terraform validate step for certificate_authority resource. I see that the test resource doesn't reflect the tf resource: https://registry.terraform.io/providers/cloudflare/cloudflare/latest/docs/resources/certificate_pack

../cloudflare_certificate_pack/test.tf:

resource "cloudflare_certificate_pack" "terraform_managed_resource" {
  hosts   = ["example.com", "*.example.com", "www.example.com"]
  type    = "dedicated_custom"
  zone_id = "0da42c8d2132a9ddaf714f9e7c920711"
}

And I also can't align the new tf resource to the actual API response at: https://api.cloudflare.com/client/v4/zones/1fa53a4819fe128dd98be7df45ec7cc6/ssl/certificate_packs?status=all

Furthermore, it looks like the filter and firewall_rules APIs with the pagination updates are calling public API endpoints twice. This PR is aiming to fix that: cloudflare/cloudflare-go#1012

@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch 9 times, most recently from 52316ad to c22c787 Compare August 3, 2022 13:52
- Add account_id to cloudflare_access_rule test resource
- Add account_id to cloudflare_account_member test resource
@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch from c22c787 to 2e3800c Compare August 3, 2022 13:53
@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch from 0a2f38f to e073f42 Compare August 3, 2022 14:54
@tamas-jozsa tamas-jozsa changed the title Change how we call rate limit API Get latest cloudflare-go and update paginations Aug 3, 2022
@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch from f571b36 to f2a21cf Compare August 5, 2022 18:50
@jacobbednarz
Copy link
Member

for future reference, there is automation that handles bumping versions (we shouldn't do it manually). it helps with things like OS dependencies and any version mismatches, see #445 for an example.

as you've done the work here, we'll push forward with this one.

@jacobbednarz
Copy link
Member

the last failing validate step looks to be a conflict in the provider and output where not all fields are being exported. will need to dig in to the provider to see why that is.

@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch 2 times, most recently from 1a0fd9b to 1bd5ecb Compare August 9, 2022 11:52
@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch from 1bd5ecb to 1040714 Compare August 9, 2022 12:00
@tamas-jozsa
Copy link
Contributor Author

the last failing validate step looks to be a conflict in the provider and output where not all fields are being exported. will need to dig in to the provider to see why that is.

There are some inconsistencies here. The example response published by the doc seems to be outdated: https://api.cloudflare.com/#certificate-packs-list-certificate-packs

I run a query against https://api.cloudflare.com/client/v4/zones/{zone_id}/ssl/certificate_packs
And I see new fields like validity_days, validation_method, but I'm not seeing certificate_authority.

I wonder what should be my next action with this situation.

jacobbednarz added a commit to jacobbednarz/cloudflare-go that referenced this pull request Aug 10, 2022
Triggered by cloudflare/cf-terraforming#443 validation mismatches, I
went ahead and updated `CertificatePacks` to only reference ACM
configuration now that dedicated custom/custom certificates are no more.
@jacobbednarz
Copy link
Member

jacobbednarz commented Aug 10, 2022

there are a couple of things here to address this.

the first is that testdata/cloudflare/cloudflare_certificate_pack.yaml should be deleted as it is now deprecated and in it's place, testdata/cloudflare/cloudflare_certificate_pack_acm.yaml should be added. this can be done in https://github.com/cloudflare/cf-terraforming/blob/master/internal/app/cf-terraforming/cmd/generate_test.go#L93

And I see new fields like validity_days, validation_method, but I'm not seeing certificate_authority.

the issue looks to be that we are marshaling []CertificatePack from https://github.com/cloudflare/cf-terraforming/blob/master/internal/app/cf-terraforming/cmd/generate.go#L299, however, we're the new responses for ACM are wanting []AdvancedCertificatePack (hence the missing fields). to fix this, i've raised cloudflare/cloudflare-go#1032 to clean up the structs and return types which will address this failure.

alternatively, you could try manually mapping the values using something like https://github.com/tidwall/gjson however that seems really error prone instead of fixing this correctly in the underlying Go library.

@tamas-jozsa
Copy link
Contributor Author

tamas-jozsa commented Aug 10, 2022

I'm in no rush merging this, so I'll just wait for cloudflare/cloudflare-go#1032 :)
I'm also thinking waiting for the next cloudflare-go version bump here in cf-tf, so I can avoid the problems with double endpoint calls during paginations.

@tamas-jozsa
Copy link
Contributor Author

@jacobbednarz I updated cloudflare-go to 47, and it looks like one of the test cloudflare ruleset (no configuration) is killed by rate limit. Error is pointing here: https://github.com/cloudflare/cloudflare-go/blob/master/cloudflare.go#L255

Not sure though if this was the case in cloudflare-go 46 as well. I will look into this more later.

@jacobbednarz
Copy link
Member

if you track this down, let me know and we can cut a hotfix in cloudflare-go if needed. this was fine in the test suite but yet to run the full acceptance test suite in terraform.

@tamas-jozsa
Copy link
Contributor Author

@jacobbednarz so looking a bit deeper into I found that cf-tf test is failing because resp is nil here: cloudflare/cloudflare-go@8563c7a and that actually uncovered a missing mock:

https://github.com/cloudflare/cf-terraforming/blob/master/testdata/cloudflare/cloudflare_ruleset_zone_no_configuration.yaml

We have mocked the API call: https://api.cloudflare.com/client/v4/zones/0da42c8d2132a9ddaf714f9e7c920711/rulesets
But don't have a mock for https://api.cloudflare.com/client/v4/zones/0da42c8d2132a9ddaf714f9e7c920711/rulesets/0800fc577294c34e0b28ad2839435945

I'm curious how should that look like?

@tamas-jozsa tamas-jozsa force-pushed the tamas/use-paginated-ratelimits branch from 4579575 to 1e46fdc Compare August 17, 2022 16:49
@jacobbednarz
Copy link
Member

fwiw, both cloudflare-go and the entire terraform provider test suites aren't seeing issues so it looks like we're doing something in this library (possibly VCR related as you mentioned above).

will have a look into this one for you.

@jacobbednarz
Copy link
Member

@jacobbednarz jacobbednarz merged commit 5ee2f5a into master Aug 18, 2022
@jacobbednarz jacobbednarz deleted the tamas/use-paginated-ratelimits branch August 18, 2022 08:52
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.

2 participants