From 4ffb05ec2b4331ffe644842dc6dcfb737be4e0d3 Mon Sep 17 00:00:00 2001 From: Hunter Fernandes Date: Thu, 4 Sep 2025 20:28:29 -0700 Subject: [PATCH 1/2] Fix Set Encoding error --- CHANGELOG.md | 9 ++ .../mathprovider/test_provider.py | 18 +++ tf/provider.py | 57 +++++---- tf/tests/test_provider.py | 111 +++++++++++++++++- 4 files changed, 173 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 142a053..b37a737 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added comprehensive end-to-end tests against OpenTofu. This ensures that providers, resources, and data sources work correctly against the real OpenTofu CLI. - **Tutorial**: - Added a tutorial to the documentation to help new users get started with writing a provider using `tf`. +- **Field Names for Encoding Errors**: + - Field names are now printed in rare cases where field values first pass validation/decoding, + but later catastrophically fail to encode or semantically compare. + These cases are generally bugs in `tf` itself, but the field names help identify the problematic fields. + +### Fixed +- **Set Crashes**: + - Fixed crashes when using `Set` types without either provided or default values. + This fixes the general case for complex types with custom semantic equality functions (only `Set` currently). ### Fixed diff --git a/e2e/mathprovider/mathprovider/test_provider.py b/e2e/mathprovider/mathprovider/test_provider.py index 881cc61..25f77cb 100644 --- a/e2e/mathprovider/mathprovider/test_provider.py +++ b/e2e/mathprovider/mathprovider/test_provider.py @@ -89,3 +89,21 @@ def test_apply_error(self): "The 'divisor' attribute cannot be zero.", ], ) + + def test_apply_for_each(self): + result = self.tf_apply( + """\ + data "math_div" "test" { + for_each = toset(["2", "5", "10"]) + + dividend = 20 + divisor = tonumber(each.value) + } + + output "the_sum" { + value = sum([for d in data.math_div.test : d.quotient]) + } + """, + expect_error=False, + ) + self.assertIn("the_sum = 16", result.stdout) diff --git a/tf/provider.py b/tf/provider.py index ca8de02..f9776ad 100644 --- a/tf/provider.py +++ b/tf/provider.py @@ -76,31 +76,39 @@ def _encode_state_d( encoded = {} for k, v in state.items(): - if v is Unknown: - encoded[k] = Unknown - elif k in attrs: - # Check if we can reuse the old encoded value - if old and k in old: - # For simple types, compare encoded values directly - if attrs[k].type.__class__.__name__ in ("Number", "String", "Bool"): - new_encoded = attrs[k].type.encode(v) - if old[k] == new_encoded: + try: + if v is Unknown: + encoded[k] = Unknown + elif k in attrs: + # Check if we can reuse the old encoded value + if old and k in old: + # For simple types, compare encoded values directly + if attrs[k].type.__class__.__name__ in ("Number", "String", "Bool"): + new_encoded = attrs[k].type.encode(v) + if old[k] == new_encoded: + encoded[k] = old[k] + else: + encoded[k] = new_encoded + + # If the previous value was Unknown and the new one is not, we just accept the new one + elif old[k] is Unknown: + encoded[k] = attrs[k].type.encode(v) + + # For complex types, use semantic equality + elif attrs[k].type.semantically_equal(attrs[k].type.decode(old[k]), v): encoded[k] = old[k] else: - encoded[k] = new_encoded - # For complex types, use semantic equality - elif attrs[k].type.semantically_equal(attrs[k].type.decode(old[k]), v): - encoded[k] = old[k] + encoded[k] = attrs[k].type.encode(v) else: encoded[k] = attrs[k].type.encode(v) else: - encoded[k] = attrs[k].type.encode(v) - else: - # block - if old and k in old and blocks[k].semantically_equal(blocks[k].decode(old[k]), v): - encoded[k] = old[k] - else: - encoded[k] = blocks[k].encode(v) + # block + if old and k in old and blocks[k].semantically_equal(blocks[k].decode(old[k]), v): + encoded[k] = old[k] + else: + encoded[k] = blocks[k].encode(v) + except Exception as exc: + raise EncodeError(f"Failed to encode field '{k}': {type(exc).__name__}: {exc}") from exc return encoded @@ -473,7 +481,10 @@ def ApplyResourceChange(self, request: pb.ApplyResourceChange.Request, context: planned_state = cast(dict, planned_state) new_state = inst.update(UpdateContext(diags, type_name), prior_state, planned_state) - # We use the planned value if its not semantically different to the new state + # We use the planned field values if they are semantically equivalent to the new state. + # For most fields on update and create, the TF client will have already done the hard work + # of encoding the field values to provide the planned state. + # We can skip re-encoding them if they semantically match what we got back from the resource. encoded_state = _encode_state(attrs, blocks, new_state, old=planned_enc) return pb.ApplyResourceChange.Response( @@ -599,3 +610,7 @@ def StopProvider(self, request: pb.StopProvider.Request, context: grpc.ServicerC # Return empty response to acknowledge shutdown request # The actual shutdown is handled by the interceptor and server loop return pb.StopProvider.Response() + + +class EncodeError(Exception): + pass diff --git a/tf/tests/test_provider.py b/tf/tests/test_provider.py index e176bb9..25fcad8 100644 --- a/tf/tests/test_provider.py +++ b/tf/tests/test_provider.py @@ -58,7 +58,7 @@ def get_data_sources(self) -> list[Type[DataSource]]: return [FavoriteNumberDataSource, FavoriteNumberErrorsDataSource] def get_resources(self) -> list[Type[Resource]]: - return [ExampleMathResource, ErrorsAlotResource, JsonResource] + return [ExampleMathResource, ErrorsAlotResource, JsonResource, ComplexResource] class ExampleMathResource(p.Resource): @@ -177,6 +177,58 @@ def delete(self, ctx: DeleteContext, current: State): return +class CustomTypeStrictSet(types.Set): + """A type of set for testing that will fail if Unknown is passed to semantically_equal""" + + def semantically_equal(self, a_decoded, b_decoded) -> bool: + self.on_check_semantic_equality(a_decoded, b_decoded) + + if a_decoded is Unknown or b_decoded is Unknown: + raise ValueError("Wow this should never have been called!!") + + return super().semantically_equal(a_decoded, b_decoded) + + @classmethod + def on_check_semantic_equality(cls, a, b): + pass + + +class ComplexResource(p.Resource): + """A resource with a set attribute: a complex type that requires special handling""" + + @classmethod + def get_name(cls) -> str: + return "complex" + + @classmethod + def get_schema(cls) -> schema.Schema: + return schema.Schema( + version=2, + attributes=[ + schema.Attribute("favorite_numbers", CustomTypeStrictSet(types.Number()), computed=True), + ], + ) + + def __init__(self, *args): + pass + + def create(self, ctx: CreateContext, planned: State) -> Optional[State]: + return { + "favorite_numbers": [9, 8, 7], + } + + def read(self, ctx: ReadContext, current: State) -> Optional[State]: + return { + "favorite_numbers": [9, 8, 7], + } + + def update(self, ctx: UpdateContext, current: State, planned: State) -> Optional[State]: + return {"favorite_numbers": list(reversed(planned["favorite_numbers"]))} # sets can be any order :) + + def delete(self, ctx: DeleteContext, current: State): + return None + + class FavoriteNumberDataSource(p.DataSource): @classmethod def get_name(cls) -> str: @@ -494,6 +546,14 @@ def test_provider_happy(self): ] ), ), + "test_complex": pb.Schema( + version=2, + block=pb.Schema.Block( + attributes=[ + pb.Schema.Attribute(name="favorite_numbers", type=b'["set","number"]', computed=True, **md), + ] + ), + ), }, ) @@ -988,6 +1048,55 @@ def test_create_nested_block_set_filled(self): {"set_block": [{"name": "a", "value": 1}, {"name": "b", "value": 2}]}, ) + def test_create_complex_types_planned_unknown(self): + """Verify complex fields can be created when their planned value is Unknown (computed)""" + provider, service, ctx = self.provider_servicer_context() + request = pb.ApplyResourceChange.Request( + type_name="test_complex", + prior_state=to_dynamic_value(None), + planned_state=to_dynamic_value({"favorite_numbers": Unknown}), + config=to_dynamic_value({"favorite_numbers": None}), + planned_private=b"", + provider_meta={}, + ) + + with mock.patch.object(CustomTypeStrictSet, "on_check_semantic_equality") as check_sem_eq: + resp = service.ApplyResourceChange(request, ctx) + + self.assert_no_diagnostic_errors(resp) + new_state = read_dynamic_value(resp.new_state) + self.assertEqual({7, 8, 9}, set(new_state["favorite_numbers"])) + + # There was no reason to call compare semantic equality, as there was a Unknown in the plan. + # (And even if we do later, NONE of the calls should have Unknown in them) + check_sem_eq.assert_not_called() + + @patch("sys.stderr", new_callable=StringIO) + def test_wrap_encoding_errors(self, stderr_mock): + """Verify exceptions raised during encoding reveal which field caused it""" + # This is a catastrophic error that should never happen, but if it does we'd like some breadcrumbs + provider, servicer, ctx = self.provider_servicer_context() + + request = pb.ApplyResourceChange.Request( + type_name="test_complex", + prior_state=to_dynamic_value(None), + planned_state=to_dynamic_value({"favorite_numbers": Unknown}), + config=to_dynamic_value({"favorite_numbers": Unknown}), + planned_private=b"", + provider_meta={}, + ) + + with mock.patch.object(CustomTypeStrictSet, "encode") as mock_encode: + mock_encode.side_effect = ValueError("Something bad happened during encoding!") + + with self.assertRaises(p.EncodeError) as raised: + servicer.ApplyResourceChange(request, ctx) + + self.assertIn( + "Failed to encode field 'favorite_numbers': ValueError: Something bad happened during encoding!", + str(raised.exception), + ) + def test_update_nested_block_unchanged_set_comparison(self): """Verify set-comparison semantics for nested block sets""" provider, servicer, ctx = self.provider_servicer_context(ComplexBlocksProvider) From e33c0e50637e57526c5a3a61c486a397571058e5 Mon Sep 17 00:00:00 2001 From: Hunter Fernandes Date: Tue, 16 Sep 2025 17:40:37 -0700 Subject: [PATCH 2/2] Consolidate Unreleased>Fixed sections --- CHANGELOG.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b37a737..f4451f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Set Crashes**: - Fixed crashes when using `Set` types without either provided or default values. This fixes the general case for complex types with custom semantic equality functions (only `Set` currently). - -### Fixed - -- Fixed the gRPC server self-signed certificate being generated with `not_valid_before` in the future - - This was caused by generating a `datetime` in the local timezone, but `x509` treating it as UTC +- **Certificate Generation Failure**: + - Fixed certificate generation failure on systems with a positive UTC offset (e.g. UTC+2). ## 1.1.0