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

Fix array manipulation bug with the zone settings resource. #1925

Merged
merged 3 commits into from Sep 27, 2022

Conversation

turbomaze
Copy link
Contributor

This PR addresses a potential bug with the cloudflare_zone_settings_override resource. I was reviewing the implementation before provisioning it, and I encountered a bit of array manipulation code that might contain a bug. I haven't observed a problem in practice, but out of caution, I don't want to apply this resource until I'm sure it won't cause any problems for my zone.

The potential bug relates to how zone settings that must be fetched as single zone settings are removed from the zone settings list. I think there's an off-by-one type of error happening as the zone settings array is mutated.

NOTE: the code in this PR has never been tested or even ran

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2022

changelog detected ✅

@jacobbednarz
Copy link
Member

@turbomaze are you able to add a test case that demonstrates the issue and the fix here? i'm not quite sure if this is actually changing the behaviour.

@turbomaze
Copy link
Contributor Author

Unfortunately I won't be able to dig into the actual provider code to put together a proper test case. But here's some Go code demonstrating the issue and the resolution:

package main

import "fmt"

func main() {
	// current behavior, buggy
	arr := []int{0, 1, 2, 3, 4}
	indexesToCut := []int{0, 1}
	fmt.Println("expected", []int{2, 3, 4})

	for _, indexToCut := range indexesToCut {
		arr = append(arr[:indexToCut], arr[indexToCut+1:]...)
	}
	fmt.Println("current", arr)

	// proposed behavior, not buggy
	arr = []int{0, 1, 2, 3, 4}
	indexesToCut = []int{0, 1}

	offset := 0
	for _, indexToCut := range indexesToCut {
		adjustedIndexToCut := indexToCut - offset
		arr = append(arr[:adjustedIndexToCut], arr[adjustedIndexToCut+1:]...)
		offset += 1
	}

	fmt.Println("proposed", arr)
}

Output:

expected [2 3 4]
current [1 3 4]
proposed [2 3 4]

@jacobbednarz
Copy link
Member

@turbomaze after you add the CHANGELOG entry (#1925 (comment)) we can get this merged.

@turbomaze
Copy link
Contributor Author

@turbomaze after you add the CHANGELOG entry (#1925 (comment)) we can get this merged.

Should be good now, thanks

@jacobbednarz jacobbednarz merged commit 8f76096 into cloudflare:master Sep 27, 2022
@github-actions github-actions bot added this to the v3.25.0 milestone Sep 27, 2022
github-actions bot pushed a commit that referenced this pull request Sep 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

This functionality has been released in v3.25.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

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.

None yet

2 participants