Add support for LoadBalancersInboundNatRule #2984

super-harsh · 2023-05-16T02:44:58Z

What this PR does / why we need it:

This PR adds support for LoadBalancersInboundNatRule to be created and managed separately.

Special notes for your reviewer:

Added load_balancer_extension.go to handle the edge case where InboundNatRule is present on LoadBalancer and independently.

If applicable:

this PR contains tests

v2/api/network/v1api20201101/load_balancer_types_gen.go

theunrepentantgeek · 2023-05-16T19:52:45Z

v2/api/network/customizations/load_balancer_extension.go

+func getInboundNatRuleGVK(lb genruntime.ARMMetaObject) schema.GroupVersionKind {
+	gvk := genruntime.GetOriginalGVK(lb)
+	gvk.Kind = reflect.TypeOf(network.LoadBalancersInboundNatRule{}).Name() // "LoadBalancersInboundNatRule"
+
+	return gvk
+}


Implementations of GetOriginalGVK() return a fully populated GroupVersionKind, e.g.:

// OriginalGVK returns a GroupValueKind for the original API version used to create the resource func (rule *LoadBalancersInboundNatRule) OriginalGVK() *schema.GroupVersionKind { return &schema.GroupVersionKind{ Group: GroupVersion.Group, Version: rule.Spec.OriginalVersion(), Kind: "LoadBalancersInboundNatRule", } }

It's not valid to grab the GVK for one kind of object and construct a GVK by overwriting one of the fields - there's no guarantee that the resulting GVK identifies anything at all. This might happen to currently work by coincidence, but that's far from ideal.

theunrepentantgeek · 2023-05-16T19:57:13Z

v2/api/network/customizations/load_balancer_extension.go

+	// the hub type has been changed but this extension has not been updated to match
+	var _ conversion.Hub = typedObj
+
+	inboundNatRuleGVK := getInboundNatRuleGVK(obj)


The safe way to do this (see my other comment) is to construct an instance of the type you want and call OriginalGVK() on it directly.

Something like this:

blankRule := &network.LoadBalancersInboundNatRule{} inboundNatRuleGVK := blankRule.GetOriginalGVK()

You may be able to combine these into one line.

The problem is that we are injecting one type into another. Imagine for example:

LoadBalancer OriginalGVK 20230101
LoadBalancerNATRule OriginalGVK 20230601

The "contract" (if you can call it that) is that 20230101 LoadBalancer can embed 20230101 NAT rules, but it can't embed the 20230601 version because that's newer. We need to run conversion.

This has a requirement that is poorly documented here and should probably be called out (here and in the vmss and RouteTable extenions too): In each API version, there must be a version of the subresource and the parent resource (no skipping one or the other). We should probably put a comment in azure-arm.yaml to that effect as well next to these resources.

I believe if the GVK doesn't exist, the List call will fail saying no such GVK, so at runtime we're "protected" here somewhat.

The issue is you can't know statically what GVK you want, so you can't do what you suggest and get the GVK statically. We need to get the GVK dynamically based on GVK that we're going to use to generate the ARM payload.

Reiterating this bit:

This has a requirement that is poorly documented here and should probably be called out (here and in the vmss and RouteTable extenions too): In each API version, there must be a version of the subresource and the parent resource (no skipping one or the other). We should probably put a comment in azure-arm.yaml to that effect as well next to these resources.

My takeaway from @theunrepentantgeek's comment, which we could try to find a way to do, is to ensure that there really is such a version for the parent type and the child type. right now we just make the assumption. Obviously that happens to work in practice but we're not very hardened against issues with that assumption here. Some combination of:

Comment in azure-arm.yaml saying that there must always be a parent and child in the same API version (to call it out for others when adding new versions)

Dynamic checks via scheme lookup here to ensure that such a type actually exists.

Seems the best way out to me?

I at least would be happy to see this done in a separate PR where we tackled all 3-4 of the resources that have this requirement together, rather than trying to do them all here, but also curious what @theunrepentantgeek thinks.

Ergh. I get it now.

My key problem here is that any failure happens at runtime of the controller, while the fix requires changes to the configuration of the generator - these are very far apart in the code base, and the errors will happen at very different times. It's also more than likely the person experiencing the failure won't be the person who needs to change the configuration.

I'd suggest the following attack:

In this PR, add comments in azure-arm.yaml to all the afflicted resources (not just the new resources in this PR).

Log an issue to track the problem

Add config + check to the pipeline to enforce the relationship, somehow, so that we can't possibly leave things out

Revisit this code to see if we can find a less-brittle way to do the conversion that's required

theunrepentantgeek · 2023-05-16T19:58:29Z

v2/api/network/customizations/load_balancer_extension.go

+		inboundNatRule := inboundNatRule
+
+		var transformed genruntime.ARMResourceSpec
+		transformed, err = transformToARM(ctx, &inboundNatRule, inboundNatRuleGVK, kubeClient, resolver)


Could/Should we modify transformToARM into a generic method where you specify the type? We could avoid having to pass in the GVK by moving the logic internally.

theunrepentantgeek · 2023-05-16T20:35:18Z

v2/api/network/customizations/load_balancer_extension.go

+	if err != nil {
+		return errors.Wrap(err, "unable to check that embedded inboundNatRule is the same as inboundNatRule")
+	}
+	if string(embeddedInboundNatRuleJSON) != string(inboundNatRuleJSON) {


Does JSON serialization have a guarantee of ordering? I suspect this may be fragile.

This is the same logic we've been using for other resource extensions(vnet, routetable) as well, and noticed no serialization issues. Can you think of a better way to do this?

According to this stackoverflow post, which references the Go JSON serialization code, struct order will be consistent.

That's an implementation detail though and is technically subject to change.

Map ordering is not guaranteed, so if the structs had maps we could be at risk here. Since we can't know for sure that the structs don't have maps, I think in general I agree with Bevan that this is not ideal.

Possibly what we could do here is:

Serialize these types to JSON

Deserialize them into a generic map[string]interface{} (map of maps)

use reflect.DeepEqual on those.

While the actual JSON representations of the types might differ because of ordering, I believe that reflect.DeepEqual is not ordering sensitive for maps and so using that would be safe.

As above it might make sense to merge this as-is (given it's a bug we have across all the impls right now) and fix it across all of them together for consistency.

if we do decide to fix this but in a different PR, let's file a bug for it so we don't forget. Make sure to mention RouteTable and VNETs there too.

codecov-commenter · 2023-05-17T23:08:54Z

Codecov Report

❗ No coverage uploaded for pull request base (main@283df38). Click here to learn what that means.
The diff coverage is 57.62%.

@@           Coverage Diff           @@
##             main    #2984   +/-   ##
=======================================
  Coverage        ?   54.19%           
=======================================
  Files           ?     1368           
  Lines           ?   579780           
  Branches        ?        0           
=======================================
  Hits            ?   314200           
  Misses          ?   213892           
  Partials        ?    51688

Impacted Files	Coverage Δ
...i/network/customizations/route_table_extensions.go	`62.16% <ø> (ø)`
...twork/customizations/virtual_network_extensions.go	`53.84% <ø> (ø)`
.../v1api20201101/load_balancer_spec_arm_types_gen.go	`33.33% <ø> (ø)`
...i/network/v1api20201101/load_balancer_types_gen.go	`49.09% <ø> (ø)`
...twork/v1api20201101/network_interface_types_gen.go	`49.43% <ø> (ø)`
...pi20201101/public_ip_address_spec_arm_types_gen.go	`33.33% <ø> (ø)`
...twork/v1api20201101/public_ip_address_types_gen.go	`48.95% <ø> (ø)`
...1api20201101storage/network_interface_types_gen.go	`55.17% <ø> (ø)`
...1api20201101storage/public_ip_address_types_gen.go	`52.63% <ø> (ø)`
...d_balancers_inbound_nat_rule_spec_arm_types_gen.go	`33.33% <33.33%> (ø)`
... and 6 more

matthchr · 2023-05-19T21:34:55Z

v2/api/network/customizations/load_balancer_extension.go

+	// the hub type has been changed but this extension has not been updated to match
+	var _ conversion.Hub = typedObj
+
+	inboundNatRuleGVK := getInboundNatRuleGVK(obj)


Reiterating this bit:

This has a requirement that is poorly documented here and should probably be called out (here and in the vmss and RouteTable extenions too): In each API version, there must be a version of the subresource and the parent resource (no skipping one or the other). We should probably put a comment in azure-arm.yaml to that effect as well next to these resources.

My takeaway from @theunrepentantgeek's comment, which we could try to find a way to do, is to ensure that there really is such a version for the parent type and the child type. right now we just make the assumption. Obviously that happens to work in practice but we're not very hardened against issues with that assumption here. Some combination of:

Comment in azure-arm.yaml saying that there must always be a parent and child in the same API version (to call it out for others when adding new versions)

Dynamic checks via scheme lookup here to ensure that such a type actually exists.

Seems the best way out to me?

I at least would be happy to see this done in a separate PR where we tackled all 3-4 of the resources that have this requirement together, rather than trying to do them all here, but also curious what @theunrepentantgeek thinks.

matthchr · 2023-05-19T21:44:20Z

v2/api/network/customizations/load_balancer_extension.go

+	if err != nil {
+		return errors.Wrap(err, "unable to check that embedded inboundNatRule is the same as inboundNatRule")
+	}
+	if string(embeddedInboundNatRuleJSON) != string(inboundNatRuleJSON) {


According to this stackoverflow post, which references the Go JSON serialization code, struct order will be consistent.

That's an implementation detail though and is technically subject to change.

Map ordering is not guaranteed, so if the structs had maps we could be at risk here. Since we can't know for sure that the structs don't have maps, I think in general I agree with Bevan that this is not ideal.

Possibly what we could do here is:

Serialize these types to JSON

Deserialize them into a generic map[string]interface{} (map of maps)

use reflect.DeepEqual on those.

While the actual JSON representations of the types might differ because of ordering, I believe that reflect.DeepEqual is not ordering sensitive for maps and so using that would be safe.

As above it might make sense to merge this as-is (given it's a bug we have across all the impls right now) and fix it across all of them together for consistency.

matthchr · 2023-05-19T21:46:31Z

v2/api/network/customizations/load_balancer_extension.go

+	if err != nil {
+		return errors.Wrap(err, "unable to check that embedded inboundNatRule is the same as inboundNatRule")
+	}
+	if string(embeddedInboundNatRuleJSON) != string(inboundNatRuleJSON) {


if we do decide to fix this but in a different PR, let's file a bug for it so we don't forget. Make sure to mention RouteTable and VNETs there too.

matthchr · 2023-05-19T21:47:06Z

v2/internal/controllers/controller_resources.go

@@ -132,9 +132,17 @@ func getGeneratedStorageTypes(

 	// Verify we're using the hub version of VirtualNetworksSubnet in the loop below
 	var _ ctrlconversion.Hub = &networkstorage.VirtualNetworksSubnet{}
+	var _ ctrlconversion.Hub = &networkstorage.LoadBalancersInboundNatRule{}
+	var _ ctrlconversion.Hub = &networkstorage.RouteTablesRoute{}


minor: Why is this line (136) needed here?

Hub version verification was missing for RouteTablesRoute so just added that here

Harshdeep Singh added 3 commits May 15, 2023 16:01

Add LoadBalancersInboundNatRules resource

f6eee21

Add update load balancer extension; Register indexers for InboundNatRule

6220aea

Add tests; Add samples

e4c4ca6

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners May 16, 2023 02:44

super-harsh and others added 2 commits May 16, 2023 14:45

Merge branch 'main' into feature/inboundNATrules

757a856

Add sample recording

759102f

theunrepentantgeek approved these changes May 16, 2023

View reviewed changes

theunrepentantgeek assigned super-harsh May 17, 2023

Merge branch 'main' into feature/inboundNATrules

4c0caf1

matthchr reviewed May 19, 2023

View reviewed changes

Harshdeep Singh and others added 2 commits May 23, 2023 14:41

Add issue reference todo in azure-arm.yaml

d9ece1e

Merge branch 'main' into feature/inboundNATrules

1671a0b

super-harsh merged commit dd230cc into main May 23, 2023
9 checks passed

super-harsh deleted the feature/inboundNATrules branch May 23, 2023 04:27

theunrepentantgeek added a commit that referenced this pull request May 23, 2023

Update documentation after #2984

618adf2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LoadBalancersInboundNatRule #2984

Add support for LoadBalancersInboundNatRule #2984

super-harsh commented May 16, 2023

theunrepentantgeek May 16, 2023

theunrepentantgeek May 16, 2023

matthchr May 16, 2023 •

edited

matthchr May 19, 2023

theunrepentantgeek May 22, 2023

theunrepentantgeek May 16, 2023

theunrepentantgeek May 16, 2023

super-harsh May 17, 2023 •

edited

matthchr May 19, 2023

matthchr May 19, 2023

codecov-commenter commented May 17, 2023

matthchr May 19, 2023

matthchr May 19, 2023

matthchr May 19, 2023

matthchr May 19, 2023

super-harsh May 21, 2023

Add support for LoadBalancersInboundNatRule #2984

Add support for LoadBalancersInboundNatRule #2984

Conversation

super-harsh commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthchr May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-harsh May 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 17, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthchr May 16, 2023 •

edited

super-harsh May 17, 2023 •

edited