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.

Sign up for GitHub

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

Jump to bottom

Fix networking resources deletion of child resources during adoption #3136

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Fix networking resources deletion of child resources during adoption #3136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44b99db
First pass at fixing networking resource adoption
matthchr Jun 23, 2023
44fc044
Attempt 2: Use Azure as the source of truth
matthchr Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 122 additions & 23 deletions v2/api/network/customizations/load_balancer_extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ package customizations

import (
"context"
"net/http"
"reflect"

"github.com/Azure/azure-sdk-for-go/sdk/azcore"
"github.com/go-logr/logr"
"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/conversion"

network "github.com/Azure/azure-service-operator/v2/api/network/v1api20201101storage"
"github.com/Azure/azure-service-operator/v2/internal/genericarmclient"
. "github.com/Azure/azure-service-operator/v2/internal/logging"
"github.com/Azure/azure-service-operator/v2/internal/resolver"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
Expand All @@ -28,12 +29,14 @@ var _ extensions.ARMResourceModifier = &LoadBalancerExtension{}

func (extension *LoadBalancerExtension) ModifyARMResource(
ctx context.Context,
armClient *genericarmclient.GenericClient,
armObj genruntime.ARMResource,
obj genruntime.ARMMetaObject,
kubeClient kubeclient.Client,
resolver *resolver.Resolver,
log logr.Logger,
) (genruntime.ARMResource, error) {

typedObj, ok := obj.(*network.LoadBalancer)
if !ok {
return nil, errors.Errorf("cannot run on unknown resource type %T, expected *network.LoadBalancer", obj)
Expand All @@ -43,40 +46,136 @@ func (extension *LoadBalancerExtension) ModifyARMResource(
// the hub type has been changed but this extension has not been updated to match
var _ conversion.Hub = typedObj

inboundNatRuleGVK := getInboundNatRuleGVK(obj)
resourceID, hasResourceID := genruntime.GetResourceID(obj)
if !hasResourceID {
// If we don't have an ARM ID yet, we've not been claimed so just return armObj as is
return armObj, nil
}

inboundNatRules := &network.LoadBalancersInboundNatRuleList{}
matchingFields := client.MatchingFields{".metadata.ownerReferences[0]": string(obj.GetUID())}
err := kubeClient.List(ctx, inboundNatRules, matchingFields)
apiVersion, err := genruntime.GetAPIVersion(typedObj, kubeClient.Scheme())
if err != nil {
return nil, errors.Wrapf(err, "failed listing InboundNatRules owned by LoadBalancer %s/%s", obj.GetNamespace(), obj.GetName())
return nil, errors.Wrapf(err, "error getting api version for resource %s while getting status", obj.GetName())
}

armInboundNatRules := make([]genruntime.ARMResourceSpec, 0, len(inboundNatRules.Items))
for _, inboundNatRule := range inboundNatRules.Items {
inboundNatRule := inboundNatRule

var transformed genruntime.ARMResourceSpec
transformed, err = transformToARM(ctx, &inboundNatRule, inboundNatRuleGVK, kubeClient, resolver)
if err != nil {
return nil, errors.Wrapf(err, "failed to transform InboundNatRules %s/%s", inboundNatRule.GetNamespace(), inboundNatRule.GetName())
// Get the raw resource
raw := make(map[string]any)
matthchr marked this conversation as resolved.
Show resolved Hide resolved
_, err = armClient.GetByID(ctx, resourceID, apiVersion, &raw)
if err != nil {
// If the error is NotFound, the resource we're trying to Create doesn't exist and so no modification is needed
var responseError *azcore.ResponseError
if errors.As(err, &responseError) && responseError.StatusCode == http.StatusNotFound {
return armObj, nil
}
armInboundNatRules = append(armInboundNatRules, transformed)
return nil, errors.Wrapf(err, "getting resource with ID: %q", resourceID)
}

log.V(Info).Info("Found InboundNatRules to include on LoadBalancer", "count", len(armInboundNatRules), "names", genruntime.ARMSpecNames(armInboundNatRules))
azureInboundNatRules, err := getRawChildCollection(raw, "inboundNatRules")
if err != nil {
return nil, errors.Wrapf(err, "failed to get inboundNatRules")
}

log.V(Info).Info("Found InboundNatRules to include on LoadBalancer", "count", len(azureInboundNatRules), "names", genruntime.RawNames(azureInboundNatRules))

err = fuzzySetResources(armObj.Spec(), armInboundNatRules, "InboundNatRules")
err = setInboundNatRules(armObj.Spec(), azureInboundNatRules)
if err != nil {
return nil, errors.Wrapf(err, "failed to set InboundNatRules")
return nil, errors.Wrapf(err, "failed to set inboundNatRules")
}

return armObj, nil
}

func getInboundNatRuleGVK(lb genruntime.ARMMetaObject) schema.GroupVersionKind {
gvk := genruntime.GetOriginalGVK(lb)
gvk.Kind = reflect.TypeOf(network.LoadBalancersInboundNatRule{}).Name() // "LoadBalancersInboundNatRule"
func getNameField(natValue reflect.Value) (ret reflect.Value, err error) {
defer func() {
if x := recover(); x != nil {
err = errors.Errorf("caught panic: %s", x)
}
}()

nameField := natValue.FieldByName("Name")
if !nameField.IsValid() {
return nameField, errors.Errorf("couldn't find name field")
}

nameField = reflect.Indirect(nameField)

return nameField, nil
}

func setInboundNatRules(lb genruntime.ARMResourceSpec, azureInboundNatRules []any) (err error) {
defer func() {
if x := recover(); x != nil {
err = errors.Errorf("caught panic: %s", x)
}
}()

inboundNatRulesField, err := getChildCollectionField(lb, "InboundNatRules")
if err != nil {
return err
}

elemType := inboundNatRulesField.Type().Elem()
inboundNatRulesSlice := reflect.MakeSlice(inboundNatRulesField.Type(), 0, 0)

for _, inboundNatRule := range azureInboundNatRules {
embeddedInboundNatRules := reflect.New(elemType)
err = fuzzySetResource(inboundNatRule, embeddedInboundNatRules)
if err != nil {
return err
}

inboundNatRulesSlice = reflect.Append(inboundNatRulesSlice, reflect.Indirect(embeddedInboundNatRules))
}

inboundNatRulesSlice, err = mergeNatRules(inboundNatRulesField, inboundNatRulesSlice)
if err != nil {
return errors.Wrapf(err, "failed to merge NAT rules")
}

inboundNatRulesField.Set(inboundNatRulesSlice)

return nil
}

func mergeNatRules(inboundNatRulesField reflect.Value, azureInboundNatRules reflect.Value) (reflect.Value, error) {
if inboundNatRulesField.Len() == 0 {
return azureInboundNatRules, nil
}

resultSlice := reflect.MakeSlice(inboundNatRulesField.Type(), 0, 0)

// The result slice should take every item from inboundNatRulesField, and merge in anything from azureInboundNatRules
// with a different name
for i := 0; i < inboundNatRulesField.Len(); i++ {
inboundNatRule := inboundNatRulesField.Index(i)
resultSlice = reflect.Append(resultSlice, inboundNatRule)
}

for i := 0; i < azureInboundNatRules.Len(); i++ {
inboundNatRule := azureInboundNatRules.Index(i)
newRuleName, err := getNameField(inboundNatRule)
if err != nil {
return reflect.Value{}, errors.Wrapf(err, "failed to get name for new rule")
}
foundExistingRule := false

for j := 0; j < inboundNatRulesField.Len(); j++ {
existingInboundNatRule := inboundNatRulesField.Index(j)
var existingName reflect.Value
existingName, err = getNameField(existingInboundNatRule)
if err != nil {
return reflect.Value{}, errors.Wrapf(err, "failed to get name for existing rule")
}

if existingName.String() == newRuleName.String() {
foundExistingRule = true
break
}
}

if !foundExistingRule {
resultSlice = reflect.Append(resultSlice, inboundNatRule)
}
Comment on lines +175 to +177
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is order significant? This puts all the extras at the end of the slice, so any that are known by ASO will be promoted in front of the others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good question. Let me check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order doesn't seem to matter. The networking RP seems to return the collection in order of creation time, so the oldest NatRules are first and the newer ones are last. Whatever order you supply rules to it in, that doesn't change.

For our purposes because we're doing a merge here based on name, it doesn't make a different to us I don't think, so the order we end up with here should be OK.

}

return gvk
return resultSlice, nil
}
127 changes: 112 additions & 15 deletions v2/api/network/customizations/load_balancer_extension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (

network "github.com/Azure/azure-service-operator/v2/api/network/v1api20201101"
"github.com/Azure/azure-service-operator/v2/internal/util/to"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
)

func Test_FuzzySetLoadBalancers(t *testing.T) {
Expand All @@ -35,25 +34,123 @@ func Test_FuzzySetLoadBalancers(t *testing.T) {
},
}

rule := &network.LoadBalancers_InboundNatRule_Spec_ARM{
Name: "myrule1",
Properties: &network.InboundNatRulePropertiesFormat_ARM{
BackendPort: to.Ptr(22),
FrontendPort: to.Ptr(23),
// Note that many of these fields are readonly and will not be present on the PUT
rawLBWithNatRules := map[string]any{
"name": "mylb",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg/providers/Microsoft.Network/loadBalancers/mylb",
"etag": "W/\"e764086f-6986-403d-a7e5-7e8d99301921\"",
"type": "Microsoft.Network/loadBalancers",
"location": "westus",
"properties": map[string]any{
"provisioningState": "Succeeded",
"resourceGuid": "a54785bf-72a4-4c89-ae93-fef560df2b53",
"inboundNatRules": []any{
map[string]any{
"name": "myrule1",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg/providers/Microsoft.Network/loadBalancers/mylb/inboundNatRules/myrule1",
"etag": "W/\"e764086f-6986-403d-a7e5-7e8d99301921\"",
"type": "Microsoft.Network/loadBalancers/inboundNatRules",
"properties": map[string]any{
"backendPort": 22,
"frontendPort": 23,
},
},
},
},
}

azureInboundNatRules, err := getRawChildCollection(rawLBWithNatRules, "inboundNatRules")
g.Expect(err).ToNot(HaveOccurred())

err = setInboundNatRules(lb, azureInboundNatRules)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(lb.Location).To(Equal(to.Ptr("westus")))
g.Expect(lb.Properties).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules).To(HaveLen(2))

rule0 := lb.Properties.InboundNatRules[0]
rule1 := lb.Properties.InboundNatRules[1]

g.Expect(rule0.Properties).ToNot(BeNil())
g.Expect(rule0.Name).To(Equal(to.Ptr("myrule")))
g.Expect(rule0.Properties.BackendPort).ToNot(BeNil())
g.Expect(rule0.Properties.FrontendPort).ToNot(BeNil())
g.Expect(rule1.Properties).ToNot(BeNil())
g.Expect(rule1.Name).To(Equal(to.Ptr("myrule1")))
g.Expect(rule1.Properties.BackendPort).ToNot(BeNil())
g.Expect(rule1.Properties.FrontendPort).ToNot(BeNil())
}

func Test_FuzzySetLoadBalancers_NatRulesMerged(t *testing.T) {
t.Parallel()
g := NewGomegaWithT(t)

lb := &network.LoadBalancer_Spec_ARM{
Location: to.Ptr("westus"),
Name: "mylb",
Properties: &network.LoadBalancerPropertiesFormat_ARM{
InboundNatRules: []network.InboundNatRule_LoadBalancer_SubResourceEmbedded_ARM{
{
Name: to.Ptr("myrule"),
Properties: &network.InboundNatRulePropertiesFormat_ARM{
BackendPort: to.Ptr(80),
FrontendPort: to.Ptr(90),
},
},
{
Name: to.Ptr("myrule1"),
Properties: &network.InboundNatRulePropertiesFormat_ARM{
BackendPort: to.Ptr(22),
FrontendPort: to.Ptr(23),
},
},
},
},
}

err := fuzzySetResources(lb, []genruntime.ARMResourceSpec{rule}, "InboundNatRules")
// Note that many of these fields are readonly and will not be present on the PUT
rawLBWithNatRules := map[string]any{
"name": "mylb",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg/providers/Microsoft.Network/loadBalancers/mylb",
"etag": "W/\"e764086f-6986-403d-a7e5-7e8d99301921\"",
"type": "Microsoft.Network/loadBalancers",
"location": "westus",
"properties": map[string]any{
"provisioningState": "Succeeded",
"resourceGuid": "a54785bf-72a4-4c89-ae93-fef560df2b53",
"inboundNatRules": []any{
map[string]any{
"name": "myrule1",
"id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg/providers/Microsoft.Network/loadBalancers/mylb/inboundNatRules/myrule1",
"etag": "W/\"e764086f-6986-403d-a7e5-7e8d99301921\"",
"type": "Microsoft.Network/loadBalancers/inboundNatRules",
"properties": map[string]any{
"backendPort": 22,
"frontendPort": 23,
},
},
},
},
}

azureInboundNatRules, err := getRawChildCollection(rawLBWithNatRules, "inboundNatRules")
g.Expect(err).ToNot(HaveOccurred())

err = setInboundNatRules(lb, azureInboundNatRules)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(lb.Location).To(Equal(to.Ptr("westus")))
g.Expect(lb.Properties).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules).To(HaveLen(2))
g.Expect(lb.Properties.InboundNatRules[0].Properties).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules[0].Name).To(Equal(to.Ptr("myrule")))
g.Expect(lb.Properties.InboundNatRules[0].Properties.BackendPort).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules[0].Properties.FrontendPort).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules[1].Properties).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules[1].Name).To(Equal(to.Ptr("myrule1")))
g.Expect(lb.Properties.InboundNatRules[1].Properties.BackendPort).ToNot(BeNil())
g.Expect(lb.Properties.InboundNatRules[1].Properties.FrontendPort).ToNot(BeNil())

rule0 := lb.Properties.InboundNatRules[0]
rule1 := lb.Properties.InboundNatRules[1]

g.Expect(rule0.Properties).ToNot(BeNil())
g.Expect(rule0.Name).To(Equal(to.Ptr("myrule")))
g.Expect(rule0.Properties.BackendPort).ToNot(BeNil())
g.Expect(rule0.Properties.FrontendPort).ToNot(BeNil())
g.Expect(rule1.Properties).ToNot(BeNil())
g.Expect(rule1.Name).To(Equal(to.Ptr("myrule1")))
g.Expect(rule1.Properties.BackendPort).ToNot(BeNil())
g.Expect(rule1.Properties.FrontendPort).ToNot(BeNil())
}
Loading