Skip to content

fix: update IP tags on managed PIPs instead of recreating them #9470

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+195 −13
Open

fix: update IP tags on managed PIPs instead of recreating them #9470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 35 additions & 9 deletions pkg/provider/azure_loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3260,7 +3260,7 @@
serviceReferences []string,
lbShouldExist, lbIsInternal, isUserAssignedPIP bool,
desiredPipName string,
ipTagRequest serviceIPTagRequest,

Check failure on line 3263 in pkg/provider/azure_loadbalancer.go

View workflow job for this annotation

GitHub Actions / Lint 

unused-parameter: parameter 'ipTagRequest' seems to be unused, consider removing or renaming it as _ (revive)
) bool {
// skip deleting user created pip
if isUserAssignedPIP {
Expand All @@ -3270,12 +3270,6 @@
// Latch some variables for readability purposes.
pipName := *existingPip.Name

// Assume the current IP Tags are empty by default unless properties specify otherwise.
currentIPTags := []*armnetwork.IPTag{}
if existingPip.Properties != nil {
currentIPTags = existingPip.Properties.IPTags
}

// Check whether the public IP is being referenced by other service.
// The owned public IP can be released only when there is not other service using it.
// case 1: there is at least one reference when deleting the PIP
Expand All @@ -3296,9 +3290,9 @@
// #3 - If the name of this public ip does not match the desired name,
// NOTICE: For IPv6 Service created with CCM v1.27.1, the created PIP has IPv6 suffix.
// We need to recreate such PIP and current logic to delete needs no change.
(pipName != desiredPipName) ||
// #4 If the service annotations have specified the ip tags that the public ip must have, but they do not match the ip tags of the existing instance
(ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags))
(pipName != desiredPipName)
// #4 IP tags mismatch should be handled by updating the PIP, not recreating it

Check failure on line 3294 in pkg/provider/azure_loadbalancer.go

View workflow job for this annotation

GitHub Actions / Lint 

File is not properly formatted (goimports)
// (ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags))
}

// ensurePIPTagged ensures the public IP of the service is tagged as configured
Expand Down Expand Up @@ -3345,6 +3339,35 @@
return changed
}

// ensurePIPIPTagged ensures the public IP of the service has correct IP tags as configured
func (az *Cloud) ensurePIPIPTagged(service *v1.Service, pip *armnetwork.PublicIPAddress) bool {
serviceIPTagRequest := getServiceIPTagRequestForPublicIP(service)

// If no IP tags are requested by annotations, no update needed
if !serviceIPTagRequest.IPTagsRequestedByAnnotation {
return false
}

// Get current IP tags from the PIP
currentIPTags := []*armnetwork.IPTag{}
if pip.Properties != nil {
currentIPTags = pip.Properties.IPTags
}

// Compare current and desired IP tags
if areIPTagsEquivalent(currentIPTags, serviceIPTagRequest.IPTags) {
return false // No changes needed
}

// Update the IP tags
if pip.Properties == nil {
pip.Properties = &armnetwork.PublicIPAddressPropertiesFormat{}
}
pip.Properties.IPTags = serviceIPTagRequest.IPTags

return true // IP tags were updated
}

// reconcilePublicIPs reconciles the PublicIP resources similar to how the LB is reconciled.
func (az *Cloud) reconcilePublicIPs(ctx context.Context, clusterName string, service *v1.Service, lbName string, wantLb bool) ([]*armnetwork.PublicIPAddress, error) {
logger := klog.FromContext(ctx).WithName("reconcilePublicIPs").
Expand Down Expand Up @@ -3541,6 +3564,9 @@
if az.ensurePIPTagged(service, pip) {
dirtyPIP = true
}
if az.ensurePIPIPTagged(service, pip) {
dirtyPIP = true
}
}
if shouldReleaseExistingOwnedPublicIP(pip, serviceReferences, wantLb, isInternal, isUserAssignedPIP, desiredPipName, serviceIPTagRequest) {
// Then, release the public ip
Expand Down
164 changes: 160 additions & 4 deletions pkg/provider/azure_loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,7 @@
expectedShouldRelease: false,
},
{
desc: "existing public ip with no format properties (unit test only?), tags required by annotation, expect release",
desc: "existing public ip with no format properties (unit test only?), tags required by annotation, should NOT release (update instead)",
existingPip: existingPipWithNoPublicIPAddressFormatProperties,
lbShouldExist: true,
lbIsInternal: false,
Expand All @@ -1314,7 +1314,7 @@
IPTagsRequestedByAnnotation: true,
IPTags: existingPipWithTag.Properties.IPTags,
},
expectedShouldRelease: true,
expectedShouldRelease: false,
},
{
desc: "LB no longer desired, expect release",
Expand Down Expand Up @@ -1353,7 +1353,7 @@
expectedShouldRelease: true,
},
{
desc: "mismatching, expect release",
desc: "mismatching IP tags, should NOT release (update instead)",
existingPip: existingPipWithTag,
lbShouldExist: true,
lbIsInternal: false,
Expand All @@ -1367,7 +1367,7 @@
},
},
},
expectedShouldRelease: true,
expectedShouldRelease: false,
},
{
// This test is for IPv6 PIP created with CCM v1.27.1 and CCM gets upgraded.
Expand Down Expand Up @@ -6391,6 +6391,162 @@
})
}

func TestEnsurePIPIPTagged(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

cloud := GetTestCloud(ctrl)

tests := []struct {
name string

Check failure on line 6401 in pkg/provider/azure_loadbalancer_test.go

View workflow job for this annotation

GitHub Actions / Lint 

File is not properly formatted (goimports)
service *v1.Service
pip *armnetwork.PublicIPAddress
expectedChanged bool
expectedIPTags []*armnetwork.IPTag
}{
{
name: "should update IP tags when they differ",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1,tag2=value2",
},
},
},
pip: &armnetwork.PublicIPAddress{
Properties: &armnetwork.PublicIPAddressPropertiesFormat{
IPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("oldvalue1"),
},
},
},
},
expectedChanged: true,
expectedIPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
{
IPTagType: ptr.To("tag2"),
Tag: ptr.To("value2"),
},
},
},
{
name: "should not change when IP tags match",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1",
},
},
},
pip: &armnetwork.PublicIPAddress{
Properties: &armnetwork.PublicIPAddressPropertiesFormat{
IPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
},
},
},
expectedChanged: false,
expectedIPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
},
},
{
name: "should not change when no IP tags requested",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{},
},
},
pip: &armnetwork.PublicIPAddress{
Properties: &armnetwork.PublicIPAddressPropertiesFormat{
IPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
},
},
},
expectedChanged: false,
expectedIPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
},
},
{
name: "should add IP tags to PIP with no properties",
service: &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
consts.ServiceAnnotationIPTagsForPublicIP: "tag1=value1",
},
},
},
pip: &armnetwork.PublicIPAddress{
Properties: nil,
},
expectedChanged: true,
expectedIPTags: []*armnetwork.IPTag{
{
IPTagType: ptr.To("tag1"),
Tag: ptr.To("value1"),
},
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
// Make a copy to avoid modifying the test data
pip := *test.pip
if test.pip.Properties != nil {
propertiesCopy := *test.pip.Properties
pip.Properties = &propertiesCopy
if test.pip.Properties.IPTags != nil {
pip.Properties.IPTags = make([]*armnetwork.IPTag, len(test.pip.Properties.IPTags))
for i, tag := range test.pip.Properties.IPTags {
tagCopy := *tag
pip.Properties.IPTags[i] = &tagCopy
}
}
}

changed := cloud.ensurePIPIPTagged(test.service, &pip)
assert.Equal(t, test.expectedChanged, changed, "unexpected changed result")

if pip.Properties != nil {
// Sort both actual and expected tags for comparison
if pip.Properties.IPTags != nil {
sortIPTags(&pip.Properties.IPTags)
}
expectedCopy := make([]*armnetwork.IPTag, len(test.expectedIPTags))
for i, tag := range test.expectedIPTags {
tagCopy := *tag
expectedCopy[i] = &tagCopy
}
sortIPTags(&expectedCopy)
assert.Equal(t, expectedCopy, pip.Properties.IPTags, "unexpected IP tags")
} else {
assert.Empty(t, test.expectedIPTags, "expected no IP tags but PIP has no properties")
}
})
}
}

func TestEnsureLoadBalancerTagged(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down
Loading