Skip to content

Commit 592fa56

Browse files
anndonoTaoMsft
andauthored
[release-1.34]feat: implement tagsList-based Interconnect Group ID retrieval from IMDS (#10007)
* Label node with Interconnect Group ID (placeholder implementation) What type of PR is this? /kind feature What this PR does / why we need it: This adds the infrastructure/plumbing code for Interconnect Group node labeling but with a placeholder implementation that returns empty string. This allows the infrastructure to be in place and tested without breaking anything, while the actual IMDS parsing implementation can be added later when the format is finalized. Which issue(s) this PR fixes: N/A (New feature - infrastructure preparation) Special notes for your reviewer: Changes: - pkg/consts: Add LabelInterconnectGroup constant - pkg/provider: Add GetInterconnectGroupId method (returns empty) - pkg/nodemanager: Add GetInterconnectGroupId to interface and integration code - pkg/node: Implement GetInterconnectGroupId for both IMDS and ARM providers (both return empty) - pkg/provider: Add minimal unit test Behavior: - GetInterconnectGroupId() always returns ("", nil) - No label is applied to nodes - No errors, nothing breaks - Infrastructure is ready for future implementation This approach allows: 1. Code review and testing of the infrastructure separately 2. Future IMDS parsing logic to be added without modifying the interface 3. Safe deployment - guaranteed not to break anything 4. No assumptions about IMDS format (no TagsList or other structures added) Does this PR introduce a user-facing change? NONE Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: NONE * Implement TagsList-based Interconnect Group ID retrieval from IMDS This commit implements the actual IMDS parsing logic to retrieve the Platform Interconnect Group ID from Azure instance metadata using the tagsList field. Implementation changes: - Add TagNameInterconnectGroup constant for the IMDS tag name - Add Tag struct to represent IMDS tag key-value pairs - Add TagsList field to ComputeMetadata struct for JSON unmarshaling - Implement GetInterconnectGroupID to parse tagsList and extract Platform_Interconnect_Group tag value - Add comprehensive tests with HTTP mock server: * Tag present in tagsList * Tag absent from tagsList * Empty tagsList * UseInstanceMetadata disabled Test updates: - Update TestNodeInitialized and TestUpdateCloudNode to verify Interconnect Group labeling alongside PlatformSubFaultDomain - Tests return actual group IDs and validate labels are set correctly This builds on the infrastructure from PR #9559 which added the label constant and placeholder function. * fix: address review feedback for Interconnect Group ID retrieval --------- Co-authored-by: wtao <wtao@microsoft.com>
1 parent 6596b68 commit 592fa56

File tree

8 files changed

+181
-1
lines changed

8 files changed

+181
-1
lines changed

pkg/consts/consts.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ const (
4343
LabelFailureDomainBetaRegion = "failure-domain.beta.kubernetes.io/region"
4444
// LabelPlatformSubFaultDomain is the label key of platformSubFaultDomain
4545
LabelPlatformSubFaultDomain = "topology.kubernetes.azure.com/sub-fault-domain"
46+
// LabelPlatformInterconnectGroup is the label key of Platform Interconnect Group
47+
LabelPlatformInterconnectGroup = "topology.kubernetes.azure.com/interconnect-group"
48+
49+
// TagNameInterconnectGroup is the tag name in IMDS tagsList for Platform Interconnect Group
50+
TagNameInterconnectGroup = "Platform_Interconnect_Group"
4651

4752
// ADFSIdentitySystem is the override value for tenantID on Azure Stack clouds.
4853
ADFSIdentitySystem = "adfs"

pkg/node/node.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,3 +86,8 @@ func (np *IMDSNodeProvider) GetZone(ctx context.Context, _ types.NodeName) (clou
8686
func (np *IMDSNodeProvider) GetPlatformSubFaultDomain(ctx context.Context) (string, error) {
8787
return np.azure.GetPlatformSubFaultDomain(ctx)
8888
}
89+
90+
// GetInterconnectGroupID returns the Interconnect Group ID from IMDS.
91+
func (np *IMDSNodeProvider) GetInterconnectGroupID(ctx context.Context) (string, error) {
92+
return np.azure.GetInterconnectGroupID(ctx)
93+
}

pkg/node/nodearm.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,8 @@ func (np *ARMNodeProvider) GetZone(ctx context.Context, name types.NodeName) (cl
9090
func (np *ARMNodeProvider) GetPlatformSubFaultDomain(_ context.Context) (string, error) {
9191
return "", nil
9292
}
93+
94+
// GetInterconnectGroupID returns empty string for ARM provider.
95+
func (np *ARMNodeProvider) GetInterconnectGroupID(_ context.Context) (string, error) {
96+
return "", nil
97+
}

pkg/nodemanager/mock/mock_node_provider.go

Lines changed: 16 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/nodemanager/nodemanager.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ type NodeProvider interface {
6262
GetZone(ctx context.Context, name types.NodeName) (cloudprovider.Zone, error)
6363
// GetPlatformSubFaultDomain returns the PlatformSubFaultDomain from IMDS if set.
6464
GetPlatformSubFaultDomain(ctx context.Context) (string, error)
65+
// GetInterconnectGroupID returns the Interconnect Group ID from IMDS if set.
66+
GetInterconnectGroupID(ctx context.Context) (string, error)
6567
}
6668

6769
// labelReconcile holds information about a label to reconcile and how to reconcile it.
@@ -508,6 +510,14 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co
508510
nodeModifiers = append(nodeModifiers, addCloudNodeLabel(consts.LabelPlatformSubFaultDomain, platformSubFaultDomain))
509511
}
510512

513+
interconnectGroupID, err := cnc.getInterconnectGroupID(ctx)
514+
if err != nil {
515+
return nil, fmt.Errorf("failed to get interconnectGroupID: %w", err)
516+
}
517+
if interconnectGroupID != "" {
518+
nodeModifiers = append(nodeModifiers, addCloudNodeLabel(consts.LabelPlatformInterconnectGroup, interconnectGroupID))
519+
}
520+
511521
return nodeModifiers, nil
512522
}
513523

@@ -656,6 +666,14 @@ func (cnc *CloudNodeController) getPlatformSubFaultDomain(ctx context.Context) (
656666
return subFD, nil
657667
}
658668

669+
func (cnc *CloudNodeController) getInterconnectGroupID(ctx context.Context) (string, error) {
670+
ig, err := cnc.nodeProvider.GetInterconnectGroupID(ctx)
671+
if err != nil {
672+
return "", fmt.Errorf("cnc.getInterconnectGroupID: %w", err)
673+
}
674+
return ig, nil
675+
}
676+
659677
func (cnc *CloudNodeController) updateNetworkingCondition(node *v1.Node, networkReady bool) error {
660678
logger := log.Background().WithName("updateNetworkingCondition")
661679
_, condition := nodeutil.GetNodeCondition(&(node.Status), v1.NodeNetworkUnavailable)

pkg/nodemanager/nodemanager_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ func TestNodeInitialized(t *testing.T) {
183183
},
184184
}, nil).AnyTimes()
185185
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("1", nil)
186+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("group-123", nil)
186187

187188
cloudNodeController := NewCloudNodeController(
188189
"node0",
@@ -199,6 +200,7 @@ func TestNodeInitialized(t *testing.T) {
199200
assert.Equal(t, "node0", fnh.UpdatedNodes[0].Name, "Node was not updated")
200201
assert.Equal(t, 0, len(fnh.UpdatedNodes[0].Spec.Taints), "Node Taint was not removed after cloud init")
201202
assert.Equal(t, "1", fnh.UpdatedNodes[0].Labels[consts.LabelPlatformSubFaultDomain])
203+
assert.Equal(t, "group-123", fnh.UpdatedNodes[0].Labels[consts.LabelPlatformInterconnectGroup])
202204
}
203205

204206
func TestUpdateCloudNode(t *testing.T) {
@@ -261,6 +263,7 @@ func TestUpdateCloudNode(t *testing.T) {
261263
},
262264
}, nil).AnyTimes()
263265
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("1", nil)
266+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("group-456", nil)
264267

265268
eventBroadcaster := record.NewBroadcaster()
266269
cloudNodeController := NewCloudNodeController(
@@ -281,6 +284,7 @@ func TestUpdateCloudNode(t *testing.T) {
281284
assert.Equal(t, 2, len(fnh.UpdatedNodes[0].Status.Conditions), "Node Contions was not updated")
282285
assert.Equal(t, "NetworkUnavailable", string(fnh.UpdatedNodes[0].Status.Conditions[0].Type), "Node Condition NetworkUnavailable was not updated")
283286
assert.Equal(t, "1", fnh.UpdatedNodes[0].Labels[consts.LabelPlatformSubFaultDomain])
287+
assert.Equal(t, "group-456", fnh.UpdatedNodes[0].Labels[consts.LabelPlatformInterconnectGroup])
284288
}
285289

286290
// This test checks that a node without the external cloud provider taint are NOT cloudprovider initialized
@@ -394,6 +398,7 @@ func TestZoneInitialized(t *testing.T) {
394398
},
395399
}, nil).AnyTimes()
396400
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("", nil)
401+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("", nil)
397402

398403
eventBroadcaster := record.NewBroadcaster()
399404
cloudNodeController := &CloudNodeController{
@@ -475,6 +480,7 @@ func TestZoneInitialized(t *testing.T) {
475480
},
476481
}, nil).AnyTimes()
477482
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("", nil)
483+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("", nil)
478484

479485
eventBroadcaster := record.NewBroadcaster()
480486
cloudNodeController := &CloudNodeController{
@@ -572,6 +578,7 @@ func TestAddCloudNode(t *testing.T) {
572578
},
573579
}, nil).AnyTimes()
574580
mockNP.EXPECT().GetPlatformSubFaultDomain(gomock.Any()).Return("", nil)
581+
mockNP.EXPECT().GetInterconnectGroupID(gomock.Any()).Return("", nil)
575582

576583
factory := informers.NewSharedInformerFactory(fnh, 0)
577584
nodeInformer := factory.Core().V1().Nodes()
@@ -736,6 +743,7 @@ func TestNodeProvidedIPAddresses(t *testing.T) {
736743
},
737744
}, nil).AnyTimes()
738745
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("", nil)
746+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("", nil)
739747

740748
eventBroadcaster := record.NewBroadcaster()
741749
cloudNodeController := NewCloudNodeController(
@@ -1158,6 +1166,7 @@ func TestNodeProviderID(t *testing.T) {
11581166
},
11591167
}, nil).AnyTimes()
11601168
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("", nil).AnyTimes()
1169+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("", nil).AnyTimes()
11611170

11621171
eventBroadcaster := record.NewBroadcaster()
11631172
cloudNodeController := &CloudNodeController{
@@ -1244,6 +1253,7 @@ func TestNodeProviderIDAlreadySet(t *testing.T) {
12441253
},
12451254
}, nil).AnyTimes()
12461255
mockNP.EXPECT().GetPlatformSubFaultDomain(ctx).Return("", nil).AnyTimes()
1256+
mockNP.EXPECT().GetInterconnectGroupID(ctx).Return("", nil).AnyTimes()
12471257

12481258
eventBroadcaster := record.NewBroadcaster()
12491259
cloudNodeController := &CloudNodeController{

pkg/provider/azure_instance_metadata.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ type Subnet struct {
6161
Prefix string `json:"prefix"`
6262
}
6363

64+
// Tag represents a key-value tag in IMDS.
65+
type Tag struct {
66+
Name string `json:"name"`
67+
Value string `json:"value"`
68+
}
69+
6470
// ComputeMetadata represents compute information
6571
type ComputeMetadata struct {
6672
Environment string `json:"azEnvironment,omitempty"`
@@ -77,6 +83,7 @@ type ComputeMetadata struct {
7783
VMScaleSetName string `json:"vmScaleSetName,omitempty"`
7884
SubscriptionID string `json:"subscriptionId,omitempty"`
7985
ResourceID string `json:"resourceId,omitempty"`
86+
TagsList []Tag `json:"tagsList,omitempty"`
8087
}
8188

8289
// InstanceMetadata represents instance information.
@@ -292,3 +299,35 @@ func (az *Cloud) GetPlatformSubFaultDomain(ctx context.Context) (string, error)
292299
}
293300
return "", nil
294301
}
302+
303+
// GetInterconnectGroupID returns the Platform Interconnect Group ID from IMDS if set.
304+
// It reads the value from the Platform_Interconnect_Group tag in tagsList.
305+
func (az *Cloud) GetInterconnectGroupID(ctx context.Context) (string, error) {
306+
logger := log.FromContextOrBackground(ctx).WithName("GetInterconnectGroupID")
307+
if az.UseInstanceMetadata {
308+
metadata, err := az.Metadata.GetMetadata(ctx, azcache.CacheReadTypeUnsafe)
309+
if err != nil {
310+
return "", err
311+
}
312+
if metadata.Compute == nil {
313+
_ = az.Metadata.imsCache.Delete(consts.MetadataCacheKey)
314+
return "", errors.New("failure of getting compute information from instance metadata")
315+
}
316+
317+
// Check tagsList for Platform_Interconnect_Group tag
318+
for _, tag := range metadata.Compute.TagsList {
319+
if tag.Name == consts.TagNameInterconnectGroup {
320+
if tag.Value == "" {
321+
logger.V(4).Info("Interconnect Group tag is present but value is empty")
322+
return "", nil
323+
}
324+
logger.V(2).Info("found Interconnect Group ID from tagsList", "InterconnectGroupID", tag.Value)
325+
return tag.Value, nil
326+
}
327+
}
328+
329+
// Tag not found - this is normal for VMs without Interconnect Groups
330+
logger.V(4).Info("Tag not found in IMDS", "tagName", consts.TagNameInterconnectGroup)
331+
}
332+
return "", nil
333+
}

pkg/provider/azure_instance_metadata_test.go

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"fmt"
2323
"net"
2424
"net/http"
25+
"net/http/httptest"
2526
"testing"
2627

2728
"github.com/stretchr/testify/assert"
@@ -150,3 +151,85 @@ func TestGetPlatformSubFaultDomain(t *testing.T) {
150151
})
151152
}
152153
}
154+
155+
func TestGetInterconnectGroupID(t *testing.T) {
156+
testCases := []struct {
157+
name string
158+
useInstanceMetadata bool
159+
respString string
160+
expectedID string
161+
expectedErr bool
162+
}{
163+
{
164+
name: "InterconnectGroup tag present",
165+
useInstanceMetadata: true,
166+
respString: `{"compute":{"tagsList":[{"name":"Platform_Interconnect_Group","value":"group-123"},{"name":"Other_Tag","value":"other"}]}}`,
167+
expectedID: "group-123",
168+
expectedErr: false,
169+
},
170+
{
171+
name: "InterconnectGroup tag absent",
172+
useInstanceMetadata: true,
173+
respString: `{"compute":{"tagsList":[{"name":"Other_Tag","value":"other"}]}}`,
174+
expectedID: "",
175+
expectedErr: false,
176+
},
177+
{
178+
name: "InterconnectGroup tag present with empty value",
179+
useInstanceMetadata: true,
180+
respString: `{"compute":{"tagsList":[{"name":"Platform_Interconnect_Group","value":""}]}}`,
181+
expectedID: "",
182+
expectedErr: false,
183+
},
184+
{
185+
name: "Empty tagsList",
186+
useInstanceMetadata: true,
187+
respString: `{"compute":{"tagsList":[]}}`,
188+
expectedID: "",
189+
expectedErr: false,
190+
},
191+
{
192+
name: "Compute metadata is nil",
193+
useInstanceMetadata: true,
194+
respString: `{}`,
195+
expectedID: "",
196+
expectedErr: true,
197+
},
198+
{
199+
name: "UseInstanceMetadata false",
200+
useInstanceMetadata: false,
201+
respString: `{"compute":{"tagsList":[{"name":"Platform_Interconnect_Group","value":"group-123"}]}}`,
202+
expectedID: "",
203+
expectedErr: false,
204+
},
205+
}
206+
207+
for _, tc := range testCases {
208+
t.Run(tc.name, func(t *testing.T) {
209+
cloud := &Cloud{
210+
Config: config.Config{
211+
Location: "eastus",
212+
UseInstanceMetadata: tc.useInstanceMetadata,
213+
},
214+
}
215+
216+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
217+
fmt.Fprint(w, tc.respString)
218+
}))
219+
defer server.Close()
220+
221+
var err error
222+
cloud.Metadata, err = NewInstanceMetadataService(server.URL + "/")
223+
assert.NoError(t, err)
224+
225+
id, err := cloud.GetInterconnectGroupID(context.TODO())
226+
227+
if tc.expectedErr {
228+
assert.Error(t, err)
229+
} else {
230+
assert.NoError(t, err)
231+
assert.Equal(t, tc.expectedID, id)
232+
}
233+
})
234+
}
235+
}

0 commit comments

Comments
 (0)