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: resource_docker_service tmpfs mounts #563 #570

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

Conversation

JanKoppe
Copy link

@JanKoppe JanKoppe commented Jul 7, 2023

terraform schemas can only work with int type which is either int32 or int64 equivalent depending on architecture, but the Docker lib expects an int64 for the size_bytes. Previously, the code assumed that the terraform schema would return an int64, which lead to provider crashes.

values are now being converted between int64 and int to handle this. The downside is possible value truncation: resources managed by Terraform can't have tmpfs size_bytes greater than just under 2 GiB (2147483647 byes by golang spec), which can lead to false information if the physical service has been modified to a tmfs size_bytes greater than that outside of terraform, because the int64->int conversion will possibly truncate that on go implementations where int is equivalent to int32.

As long as the limitiation of only TypeInt being available in the schema (and not e.g. TypeInt64) there will be no clean solution to this. An improvement could be to switch to either TypeFloat (53 bits of accuracy, still with the issue, but much less likely to impact in real life, it's 8TiB) or TypeString and convert the numbers between string representations. Both options exceed my Go capabilities though, I think.

terraform schemas can only work with int type which is either int32 or
int64 equivalent depending on architecture, but the Docker lib expects
an int64 for the size_bytes. Previously, the code assumed that the
terraform schema would return an int64, which lead to provider crashes.

values are now being converted between int64 and int to handle this. The
downside is possible value truncation: resources managed by Terraform
can't have tmpfs size_bytes greater than just under 2 GiB (2147483647
byes by golang spec), which can lead to false information if the
physical service has been modified to a tmfs size_bytes greater than
that outside of terraform, because the int64->int conversion will
possibly truncate that on go implementations where int is equivalent to
int32.

As long as the limitiation of only TypeInt being available in the schema
(and not e.g. TypeInt64) there will be no clean solution to this. An
improvement could be to switch to either TypeFloat (53 bits of accuracy,
still with the issue, but much less likely to impact in real life, it's
8TiB) or TypeString and convert the numbers between string
representations. Both options exceed my Go capabilities though, I think.
@tinexw
Copy link

tinexw commented Sep 22, 2023

Hi @JanKoppe, thanks for the fix! Would you mind adding a unit test to

  • document the truncation issue you mentioned above
  • (if possible) one that demonstrates the issue and the fix

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