Open polkx opened 3 months ago
This is an upstream problem (in azure-native) that bubbles up to us. Do you need the value of the ip address? If so, the only way to deal with this is to fix upstream. Our serde layer is lazy (on purpose) so if you don't use a property that fails on deserialization it won't crash your program.
Thank you @polkx for the report. Looks like the underlying cause is two-fold:
1) field fqdn
is required and type of string, we get the protobuf's NULL_VALUE
- this is provider breaking its contract
2) field autoGeneratedDomainNameLabelScope
is optional with a default and we mistakenly generate the provider code where the field is not optional - this is besom codegen bug.
The reason why ipAddress = containerGroup.ipAddress.ip
is the bug trigger is because besom is trying to be smart about deserialization, and it is done when needed - so until someone accesses the filed witl broken value from the provider, then the problem does not propagate. Since this problem is widely spread among pulumi providers (of unexpected null values) then this should be helpful.
A separate discussion is whether or not, adding an "escape hatch", like a `recover function proposed by @lbialy would be helpful to the user. As pointed out by Łukasz, due to the fact that te user has limited possibilities how to remediate such a problem, the usefulness of such escape hatch would be limited.
For the reference, the schema from upstream defines the ipAddress
as:
"azure-native:containerinstance:IpAddressResponse": {
"description": "IP address for the container group.",
"properties": {
"autoGeneratedDomainNameLabelScope": {
"type": "string",
"description": "The value representing the security enum. The 'Unsecure' value is the default value if not selected and means the object's domain name label is not secured against subdomain takeover. The 'TenantReuse' value is the default value if selected and means the object's domain name label can be reused within the same tenant. The 'SubscriptionReuse' value means the object's domain name label can be reused within the same subscription. The 'ResourceGroupReuse' value means the object's domain name label can be reused within the same resource group. The 'NoReuse' value means the object's domain name label cannot be reused within the same resource group, subscription, or tenant.",
"default": "Unsecure"
},
"dnsNameLabel": {
"type": "string",
"description": "The Dns name label for the IP."
},
"fqdn": {
"type": "string",
"description": "The FQDN for the IP."
},
"ip": {
"type": "string",
"description": "The IP exposed to the public internet."
},
"ports": {
"type": "array",
"items": {
"type": "object",
"$ref": "#/types/azure-native:containerinstance:PortResponse"
},
"description": "The list of ports exposed on the container group."
},
"type": {
"type": "string",
"description": "Specifies if the IP is exposed to the public internet or private VNET."
}
},
"type": "object",
"required": [
"fqdn",
"ports",
"type"
]
},
besom azure native code:
final case class ContainerGroup private(
...
ipAddress: besom.types.Output[scala.Option[besom.api.azurenative.containerinstance.outputs.IpAddressResponse]],
...
) extends besom.CustomResource
final case class IpAddressResponse private(
autoGeneratedDomainNameLabelScope: String, // should be optional
dnsNameLabel: scala.Option[String],
fqdn: String, // is correct
ip: scala.Option[String],
ports: scala.collection.immutable.List[besom.api.azurenative.containerinstance.outputs.PortResponse],
`type`: String
)
pulumi TS azure native:
export interface IpAddressResponse {
autoGeneratedDomainNameLabelScope?: string; // <<<
dnsNameLabel?: string;
fqdn: string; // <<<
ip?: string;
ports: outputs.containerinstance.PortResponse[];
type: string;
}
This is what we get from the engine:
Value(
kind = StructValue(
value = Struct(
fields = Map(
"ip" -> Value(kind = StringValue(value = "4.157.146.145")),
"ports" -> Value(
kind = ListValue(
value = ListValue(
values = Vector(
Value(
kind = StructValue(
value = Struct(
fields = Map(
"port" -> Value(kind = NumberValue(value = 8080.0)),
"protocol" -> Value(kind = StringValue(value = "TCP"))
)
)
)
)
)
)
)
),
"type" -> Value(kind = StringValue(value = "Public"))
)
)
)
As visible above, the ip
that was accessed in the beginning, is present, and the errors are regarding neighboring fields.
The behavior I would expect is:
1) when accessing only the ip
field - no error
2) when accessing the entire ipAddress
- all errors regarding any of its fields
The question is, how can we do it, and how feasible is this.
Wait, this is an output type of struct, right? The property of a resource is an output of a case class that belongs to outputs package in codegened code? I thought that you meant that it's a property on a resource! If it's a field on a case class that's an output struct we can't do anything about it without breaking the type system. Let me elaborate - let's say we have:
case class SomeProps(a: String, b: Int, c: Double)
case class SomeResource(name: Output[String], props: Output[SomeProps])
Now in serde layer we have a resource decoder that will look up decoder instances for all fields in SomeResource
and it will find List(Decoder[String], Decoder[SomeProps])
. What we get from gRPC for the props field, for brevity in JSON form, is:
{
"a": "some string",
"b": 23
}
Notice the missing value for c
field! Given that the definition of SomeProps
case class expects non-nullable String, Int and Double there's literally nothing we can do to satisfy this expectation if we have a String and an Int only short of pushing a null
into the c: Double
position. That is subversion of the type system and as such is strictly against our philosophy of not lying in types.
The only other thing we could do here is to replace all the case classes of the output-kind of structs with wrappers extending Selectable
over Maps with type refinements to allow safe, inline-based access to the properties held inside like @prolativ proposed to solve bincompat issues. This would break any support in Intellij because it's unable to comprehend type refinements at all and would force us to write an Intellij plugin for Besom. This solution would also throw on missing values but given that it always happens in Outputs and is literally how we currently work with such defects it would keep the status quo but improve the situation with deeply nested missing or broken props (because it would be possible to defer errors until access to a broken field happens).
Other than that (and that's not really feasible given the Intellij problem) there's not much we can do beside complaining to the upstream to get their stuff fixed.
We've had a discussion regarding hotfixes on our side that would patch schema during codegen before and you were against this. Such a hotfix to schema would allow us to mark this field as optional, codegenerate the case class with Option field and then our decoders would work without any issues.
You are right, since output classes does not have Output
properties, we cannot defer the error to the last minute.
The easiest solution in my opinion would be to have output classes with output fields, then we'd have the granularity we need in regard to decoder errors. Since we already return Resources as outputs and we have a lot of lifts and other helpers to deal with outputs, this should not be that big of an issues, or am I missing something obvious, WDYT @lbialy?
Regarding other possibilities:
... extending Selectable over Maps with type refinements ...
I'd have to dig deeper into this proposal to fully understand all the implications, but I trust your judgment and from what you're saying I'm having mixed feelings about this one, I'm not against thou, just worried about consequences to the DX.
... complaining to the upstream to get their stuff fixed
Yes, this is always an options, unfortunately, I'd be surprised if this type of issue would get high priority, since all other SDK choose to deal with this by hiding the problem.
... patch schema during codegen before and you were against this
Yes, my main concern with schema patching is the scalability of maintenance, this would be fine with big enough community thou, but that's a bit of a chicken and the egg situation
... mark this field as optional ...
This would effectively turn every field into optional field, because if we cannot rely on the provider to conform to its schema then we have to assume that everything is optional, and this does not sound user friendly at all to me
There's one more consideration, the secretness. @polkx has made me aware that, when one of the fields in output class is secret, then the Output
of the whole output class is secret, effectively making every field secret, regardless of the underlying schema. Making every field an output would solve this issue as well.
Until we re-architecture the output classes, I'd like to put under consideration the addition of Output.recover
to allow for workarounds.
When I try to get ipAddress (
ipAddress = containerGroup.ipAddress.ip
) from azurenative.containerinstance.ContainerGroup the Error shows up about deserializing an option. The error disappears when I comment on the lineipAddress = containerGroup.ipAddress.ip
.Dependencies:
Main class:
Stack trace: