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

Azure Load Balancer reconciliation should consider all Kubernetes-controlled properties of a LB NSG #55752

Merged
merged 2 commits into from
Nov 22, 2017
Merged

Azure Load Balancer reconciliation should consider all Kubernetes-controlled properties of a LB NSG #55752

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
d39b517
findSecurityRule returns true when it matches most of characteristics.
kevinkim9264 Nov 21, 2017
8514537
comment on findSecurityRule and fmt
kevinkim9264 Nov 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 30 additions & 2 deletions pkg/cloudprovider/providers/azure/azure_loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1205,11 +1205,39 @@ func findRule(rules []network.LoadBalancingRule, rule network.LoadBalancingRule)
return false
}

// This compares rule's Name, Protocol, SourcePortRange, DestinationPortRange, SourceAddressPrefix, Access, and Direction.
// Note that it compares rule's DestinationAddressPrefix only when it's not consolidated rule as such rule does not have DestinationAddressPrefix defined.
// We intentionally do not compare DestinationAddressPrefixes in consolidated case because reconcileSecurityRule has to consider the two rules equal,
// despite different DestinationAddressPrefixes, in order to give it a chance to consolidate the two rules.
func findSecurityRule(rules []network.SecurityRule, rule network.SecurityRule) bool {
for _, existingRule := range rules {
if strings.EqualFold(*existingRule.Name, *rule.Name) {
return true
if !strings.EqualFold(*existingRule.Name, *rule.Name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would reflect.DeepEqual() help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think DeepEqual() is helpful here as we are comparing two strings?
Other values - numbers, bools, strings, and channels - are deeply equal if they are equal using Go's == operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so in fact probably EqualFold is better- EqualFold reports whether s and t, interpreted as UTF-8 strings, are equal under Unicode case-folding.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think DeepEqual is overkill. There are things that don't need to specified and really can't be reconciled, like the rule priority. I'll forgo comment on whether other fields should be included. Since we don't set them, I'm inclined to suggest that they can be added to the comparison in the future, just as is being done with this PR as a followup to the initial fix for sourceAddressPrefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This PR should focus on finding correct differences based on what we already set in security rule.

continue
}
if existingRule.Protocol != rule.Protocol {
continue
}
if !strings.EqualFold(*existingRule.SourcePortRange, *rule.SourcePortRange) {
continue
}
if !strings.EqualFold(*existingRule.DestinationPortRange, *rule.DestinationPortRange) {
continue
}
if !strings.EqualFold(*existingRule.SourceAddressPrefix, *rule.SourceAddressPrefix) {
continue
}
if !allowsConsolidation(existingRule) && !allowsConsolidation(rule) {
if !strings.EqualFold(*existingRule.DestinationAddressPrefix, *rule.DestinationAddressPrefix) {
continue
}
}
if existingRule.Access != rule.Access {
continue
}
if existingRule.Direction != rule.Direction {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Some Azure regions support "augmented security rules" which permit more than one destination address in a rule (https://azure.microsoft.com/en-au/updates/public-preview-features-for-nsgs/). This is part of the 11.x Azure SDK, which is in master as of 14e134c. It might be a good idea to check rule.DestinationAddressPrefixes as well as DestinationAddressPrefix - we won't hit this with unmodified rules in current master but it could happen if someone modifies a rule through the portal, and it will be used within Kubernetes if #55658 is accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I think that additional check can be done in follow-up PR? As for this PR, I think if makes sense to do the check on only controlled properties that are explicitly set while generating security rules. (at least for this branch).

}
return true
}
return false
}
Expand Down
30 changes: 30 additions & 0 deletions pkg/cloudprovider/providers/azure/azure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,36 @@ func TestReconcileLoadBalancerAddServiceOnInternalSubnet(t *testing.T) {
validateLoadBalancer(t, lb, svc)
}

func TestReconcileSecurityGroupFromAnyDestinationAddressPrefixToLoadBalancerIP(t *testing.T) {
az := getTestCloud()
svc1 := getTestService("serviceea", v1.ProtocolTCP, 80)
svc1.Spec.LoadBalancerIP = "192.168.0.0"
sg := getTestSecurityGroup(az)
// Simulate a pre-Kubernetes 1.8 NSG, where we do not specify the destination address prefix
sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(""), true)
if err != nil {
t.Errorf("Unexpected error: %q", err)
}
sg, err = az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(svc1.Spec.LoadBalancerIP), true)
if err != nil {
t.Errorf("Unexpected error: %q", err)
}
validateSecurityGroup(t, sg, svc1)
}

func TestReconcileSecurityGroupDynamicLoadBalancerIP(t *testing.T) {
az := getTestCloud()
svc1 := getTestService("servicea", v1.ProtocolTCP, 80)
svc1.Spec.LoadBalancerIP = ""
sg := getTestSecurityGroup(az)
dynamicallyAssignedIP := "192.168.0.0"
sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, to.StringPtr(dynamicallyAssignedIP), true)
if err != nil {
t.Errorf("unexpected error: %q", err)
}
validateSecurityGroup(t, sg, svc1)
}

// Test addition of services on an internal LB using both default and explicit subnets.
func TestReconcileLoadBalancerAddServicesOnMultipleSubnets(t *testing.T) {
az := getTestCloud()
Expand Down