feat(azure): add provider ID detection for cluster-autoscaler integration#3
feat(azure): add provider ID detection for cluster-autoscaler integration#3IvanHunters wants to merge 5 commits intomainfrom
Conversation
Implement DetectAzureProviderID function to query Azure Instance Metadata Service (IMDS) for the resource ID. This enables automatic providerID setup on Azure nodes, which is required for cluster-autoscaler to properly track scaled nodes. The detector gracefully handles non-Azure environments by returning empty string when IMDS is unavailable. Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
Add method to set spec.providerID on nodes via JSON patch. The method includes idempotency check to avoid unnecessary updates and warns when overwriting existing providerID values. This enables cloud controllers to set provider-specific node identifiers required by components like cluster-autoscaler. Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
Add --enable-azure-provider-id flag to enable automatic provider ID detection and setup on Azure nodes. When enabled, the controller will: - Query Azure IMDS for the resource ID - Set spec.providerID if detected - Gracefully skip if not running in Azure This resolves cluster-autoscaler issues where nodes lack providerID and become orphaned from their cloud VM instances. Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
Add azure.enableProviderID parameter to values.yaml to control Azure provider ID detection. When enabled, the DaemonSet will pass --enable-azure-provider-id flag to local-ccm controller. Update chart README with Azure configuration example and document the new parameter in the configuration table. Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
Update README with Azure provider ID feature in features list, configuration table, and example configurations. Add explanation of how it integrates with cluster-autoscaler on Azure. Signed-off-by: ohotnikov.ivan <ohotnikov.ivan@e-queo.net>
📝 WalkthroughWalkthroughThis PR adds Azure provider ID detection to the CCM. A new Changes
Sequence DiagramsequenceDiagram
participant Controller as CCM Controller
participant IMDS as Azure IMDS
participant K8sAPI as Kubernetes API
Controller->>Controller: Check --enable-azure-provider-id flag
alt Feature Enabled
Controller->>IMDS: HTTP GET /metadata/instance/compute/resourceId<br/>(with Metadata: true header, 2s timeout)
alt IMDS Request Succeeds
IMDS-->>Controller: 200 OK + Resource ID
Controller->>Controller: Format as azure://resourceId
Controller->>K8sAPI: PATCH /nodes/{node}/spec/providerID<br/>(JSONPatchType, replace operation)
K8sAPI-->>Controller: Success
Controller->>Controller: Log provider ID set
else IMDS Request Fails
IMDS-->>Controller: Error or non-200 status
Controller->>Controller: Log debug message (no error)
end
else Feature Disabled
Controller->>Controller: Skip provider ID detection
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for automatic Azure spec.providerID detection via the Azure Instance Metadata Service (IMDS), which is essential for Azure cluster-autoscaler integration. The changes include new configuration flags, documentation updates, and the implementation of the detection and update logic. Feedback focuses on optimizing the reconciliation loop by avoiding redundant API calls and IMDS queries once the provider ID is set, reusing the HTTP client for better connection management, and improving the robustness of the JSON patch operation by using "add" instead of "replace".
| if enableAzureProviderID { | ||
| providerID, err := detector.DetectAzureProviderID(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to detect Azure provider ID: %w", err) | ||
| } | ||
| if providerID != "" { | ||
| if err := nodeUpdater.UpdateProviderID(ctx, providerID); err != nil { | ||
| return fmt.Errorf("failed to update provider ID: %w", err) | ||
| } | ||
| } else { | ||
| klog.V(3).Info("Azure provider ID not detected (not running in Azure)") | ||
| } | ||
| } |
There was a problem hiding this comment.
The Azure provider ID is immutable for the lifetime of a node. Currently, the controller queries the Azure Instance Metadata Service (IMDS) and potentially the Kubernetes API server every reconciliation loop (default 10s), even after the provider ID has been successfully set.
Adding a check to see if currentNode.Spec.ProviderID is already populated will significantly reduce unnecessary network traffic and API pressure.
| if enableAzureProviderID { | |
| providerID, err := detector.DetectAzureProviderID(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to detect Azure provider ID: %w", err) | |
| } | |
| if providerID != "" { | |
| if err := nodeUpdater.UpdateProviderID(ctx, providerID); err != nil { | |
| return fmt.Errorf("failed to update provider ID: %w", err) | |
| } | |
| } else { | |
| klog.V(3).Info("Azure provider ID not detected (not running in Azure)") | |
| } | |
| } | |
| // Set Azure provider ID if enabled and not already set | |
| if enableAzureProviderID && currentNode.Spec.ProviderID == "" { | |
| providerID, err := detector.DetectAzureProviderID(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to detect Azure provider ID: %w", err) | |
| } | |
| if providerID != "" { | |
| if err := nodeUpdater.UpdateProviderID(ctx, providerID); err != nil { | |
| return fmt.Errorf("failed to update provider ID: %w", err) | |
| } | |
| } else { | |
| klog.V(3).Info("Azure provider ID not detected (not running in Azure)") | |
| } | |
| } |
| // DetectAzureProviderID detects Azure provider ID from Instance Metadata Service | ||
| // Returns empty string if not running in Azure environment | ||
| func DetectAzureProviderID(ctx context.Context) (string, error) { | ||
| klog.V(3).Info("Attempting to detect Azure provider ID from IMDS") | ||
|
|
||
| // Create HTTP client with timeout | ||
| client := &http.Client{ | ||
| Timeout: AzureIMDSTimeout, | ||
| } | ||
|
|
||
| // Create request with context | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, AzureIMDSURL, nil) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create IMDS request: %w", err) | ||
| } | ||
|
|
||
| // Azure IMDS requires Metadata header | ||
| req.Header.Set("Metadata", "true") | ||
|
|
||
| klog.V(4).Infof("Sending request to Azure IMDS: %s", AzureIMDSURL) | ||
|
|
||
| // Execute request | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| // This is expected when not running in Azure | ||
| klog.V(3).Infof("Azure IMDS not available (likely not running in Azure): %v", err) | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
Creating a new http.Client on every call to DetectAzureProviderID prevents connection reuse and increases overhead. It is better practice to reuse a single client instance across requests to benefit from connection pooling.
var azureIMDSClient = &http.Client{
Timeout: AzureIMDSTimeout,
}
// DetectAzureProviderID detects Azure provider ID from Instance Metadata Service
// Returns empty string if not running in Azure environment
func DetectAzureProviderID(ctx context.Context) (string, error) {
klog.V(3).Info("Attempting to detect Azure provider ID from IMDS")
// Create request with context
req, err := http.NewRequestWithContext(ctx, http.MethodGet, AzureIMDSURL, nil)
if err != nil {
return "", fmt.Errorf("failed to create IMDS request: %w", err)
}
// Azure IMDS requires Metadata header
req.Header.Set("Metadata", "true")
klog.V(4).Infof("Sending request to Azure IMDS: %s", AzureIMDSURL)
// Execute request
resp, err := azureIMDSClient.Do(req)
if err != nil {
// This is expected when not running in Azure
klog.V(3).Infof("Azure IMDS not available (likely not running in Azure): %v", err)
return "", nil
}| func (u *Updater) UpdateProviderID(ctx context.Context, providerID string) error { | ||
| klog.V(2).Infof("Updating providerID for node %s: %s", u.nodeName, providerID) | ||
|
|
||
| // Get current node to check if update is needed | ||
| node, err := u.GetNode(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get node: %w", err) | ||
| } | ||
|
|
||
| // Check if providerID is already set correctly | ||
| if node.Spec.ProviderID == providerID { | ||
| klog.V(3).Infof("ProviderID already set correctly on node %s, skipping update", u.nodeName) | ||
| return nil | ||
| } | ||
|
|
||
| // Log if we're overwriting an existing providerID | ||
| if node.Spec.ProviderID != "" { | ||
| klog.Warningf("Overwriting existing providerID on node %s: %s -> %s", | ||
| u.nodeName, node.Spec.ProviderID, providerID) | ||
| } | ||
|
|
||
| // Create JSON patch for providerID | ||
| patch := []map[string]interface{}{ | ||
| { | ||
| "op": "replace", | ||
| "path": "/spec/providerID", | ||
| "value": providerID, | ||
| }, | ||
| } | ||
|
|
||
| patchBytes, err := json.Marshal(patch) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal patch: %w", err) | ||
| } | ||
|
|
||
| klog.V(4).Infof("Applying providerID patch to node %s: %s", u.nodeName, string(patchBytes)) | ||
|
|
||
| // Apply patch | ||
| _, err = u.client.CoreV1().Nodes().Patch( | ||
| ctx, | ||
| u.nodeName, | ||
| types.JSONPatchType, | ||
| patchBytes, | ||
| metav1.PatchOptions{}, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to patch node providerID: %w", err) | ||
| } | ||
|
|
||
| klog.Infof("Successfully updated providerID for node %s to %s", u.nodeName, providerID) | ||
| return nil |
There was a problem hiding this comment.
The UpdateProviderID method currently performs a redundant GetNode call. Since the caller in main.go already retrieves the node object, this extra API request is unnecessary.
Additionally, consider using the add operation instead of replace in the JSON patch. If spec.providerID is currently empty and omitted from the node's JSON representation, the replace operation will fail because the path does not exist. The add operation is safer as it works whether the field exists or not.
// UpdateProviderID updates the node's spec.providerID
func (u *Updater) UpdateProviderID(ctx context.Context, providerID string) error {
klog.V(2).Infof("Updating providerID for node %s: %s", u.nodeName, providerID)
// Create JSON patch for providerID
// Using "add" is safer than "replace" as the field might not exist in the JSON yet
patch := []map[string]interface{}{
{
"op": "add",
"path": "/spec/providerID",
"value": providerID,
},
}
patchBytes, err := json.Marshal(patch)
if err != nil {
return fmt.Errorf("failed to marshal patch: %w", err)
}
klog.V(4).Infof("Applying providerID patch to node %s: %s", u.nodeName, string(patchBytes))
// Apply patch
_, err = u.client.CoreV1().Nodes().Patch(
ctx,
u.nodeName,
types.JSONPatchType,
patchBytes,
metav1.PatchOptions{},
)
if err != nil {
return fmt.Errorf("failed to patch node providerID: %w", err)
}
klog.Infof("Successfully updated providerID for node %s to %s", u.nodeName, providerID)
return nil
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/detector/azure_provider.go`:
- Around line 44-46: The http.Client currently created with Timeout:
AzureIMDSTimeout uses the default transport which respects
HTTP_PROXY/HTTPS_PROXY; update the client to use an http.Transport with Proxy
set to nil so IMDS calls bypass proxies. Replace the simple client creation
(client := &http.Client{Timeout: AzureIMDSTimeout}) by constructing a transport
(e.g., clone or new http.Transport) with Proxy: nil and any required defaults,
assign it to client.Transport and keep Timeout: AzureIMDSTimeout to ensure
direct link-local access to 169.254.169.254 for the Azure metadata detection.
In `@pkg/node/updater.go`:
- Around line 174-179: The JSON Patch uses the "replace" op on
"/spec/providerID" which fails when spec.providerID is absent; update the patch
construction in pkg/node/updater.go (the variable patch that includes the map
with "path": "/spec/providerID" and "value": providerID) to use the "add"
operation instead of "replace" so the field will be created on first-write and
updated on subsequent writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4536e2ab-957e-4376-bc86-5b91b6bddac6
📒 Files selected for processing (7)
README.mdcharts/local-ccm/README.mdcharts/local-ccm/templates/daemonset.yamlcharts/local-ccm/values.yamlcmd/local-ccm/main.gopkg/detector/azure_provider.gopkg/node/updater.go
| client := &http.Client{ | ||
| Timeout: AzureIMDSTimeout, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the azure_provider.go file
find . -name "azure_provider.go" -type fRepository: cozystack/local-ccm
Length of output: 95
🏁 Script executed:
# Read the relevant section of the file
cat -n pkg/detector/azure_provider.go | head -80Repository: cozystack/local-ccm
Length of output: 2891
🏁 Script executed:
# Check if there are other HTTP clients or similar patterns in the detector package
rg -n "http\.Client\{" pkg/detector/ -A 2Repository: cozystack/local-ccm
Length of output: 221
🏁 Script executed:
# Verify if there are other cloud provider detectors that might have similar patterns
ls -la pkg/detector/Repository: cozystack/local-ccm
Length of output: 304
IMDS calls must explicitly bypass HTTP proxies to ensure detection works in proxied environments
The default http.Client at lines 44-46 uses DefaultTransport, which respects HTTP_PROXY/HTTPS_PROXY environment variables. In proxied clusters or corporate networks, this routes the IMDS call (169.254.169.254) through the proxy, breaking Azure provider detection. IMDS is a link-local metadata service that must communicate directly, never through proxies.
Proposed fix
- client := &http.Client{
- Timeout: AzureIMDSTimeout,
- }
+ transport := http.DefaultTransport.(*http.Transport).Clone()
+ transport.Proxy = nil
+ client := &http.Client{
+ Timeout: AzureIMDSTimeout,
+ Transport: transport,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client := &http.Client{ | |
| Timeout: AzureIMDSTimeout, | |
| } | |
| transport := http.DefaultTransport.(*http.Transport).Clone() | |
| transport.Proxy = nil | |
| client := &http.Client{ | |
| Timeout: AzureIMDSTimeout, | |
| Transport: transport, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/detector/azure_provider.go` around lines 44 - 46, The http.Client
currently created with Timeout: AzureIMDSTimeout uses the default transport
which respects HTTP_PROXY/HTTPS_PROXY; update the client to use an
http.Transport with Proxy set to nil so IMDS calls bypass proxies. Replace the
simple client creation (client := &http.Client{Timeout: AzureIMDSTimeout}) by
constructing a transport (e.g., clone or new http.Transport) with Proxy: nil and
any required defaults, assign it to client.Transport and keep Timeout:
AzureIMDSTimeout to ensure direct link-local access to 169.254.169.254 for the
Azure metadata detection.
| patch := []map[string]interface{}{ | ||
| { | ||
| "op": "replace", | ||
| "path": "/spec/providerID", | ||
| "value": providerID, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "updater.go" -path "*/node/*" | head -5Repository: cozystack/local-ccm
Length of output: 84
🏁 Script executed:
# Read the relevant section of the file
cat -n pkg/node/updater.go | sed -n '150,210p'Repository: cozystack/local-ccm
Length of output: 1931
🏁 Script executed:
# Search for how this patch is applied
rg -n "JSONPatchType\|json\.Marshal\|patch\s*=" pkg/node/updater.go -A 3 -B 3Repository: cozystack/local-ccm
Length of output: 45
🏁 Script executed:
# Check how the patch is used and if there are tests
rg -n "UpdateProviderID\|providerID" pkg/node/ -A 2 -B 2 | head -80Repository: cozystack/local-ccm
Length of output: 45
🏁 Script executed:
# Check if there are tests for this method
find . -name "*test.go" -path "*/node/*" | xargs rg -l "UpdateProviderID" 2>/dev/nullRepository: cozystack/local-ccm
Length of output: 104
🏁 Script executed:
# Search for any related issue tracking or comments
rg -n "providerID.*patch\|patch.*providerID\|replace.*add" --type go -iRepository: cozystack/local-ccm
Length of output: 45
🏁 Script executed:
# Let's check the test file that was found
cat -n pkg/node/updater.goRepository: cozystack/local-ccm
Length of output: 6772
🏁 Script executed:
# Search for any Node object creation or initialization
rg -n "corev1.Node\|Node{" pkg/node/ -A 3Repository: cozystack/local-ccm
Length of output: 162
🏁 Script executed:
# Check if there's a go.mod or similar to understand Kubernetes version
cat go.mod | grep kubernetesRepository: cozystack/local-ccm
Length of output: 45
🏁 Script executed:
# Search for Kubernetes version in go.mod or go.sum
find . -name "go.mod" -o -name "go.sum" | xargs grep -h "k8s.io" | head -20Repository: cozystack/local-ccm
Length of output: 1363
🏁 Script executed:
# Now let me verify the actual RFC 6902 JSON Patch behavior with an example
# Let's check the Kubernetes documentation approach
web_search
Does Kubernetes JSON Patch "replace" operation fail when field is missing/unset?Repository: cozystack/local-ccm
Length of output: 156
🌐 Web query:
RFC 6902 JSON Patch "replace" operation behavior when target path does not exist
💡 Result:
According to RFC 6902, the "replace" operation in JSON Patch fails if the target path does not exist. The specification states: "The target location MUST exist for the operation to be successful." If the target location does not exist, the operation is not successful, and evaluation of the JSON Patch document SHOULD terminate, with the entire patch application deemed unsuccessful (Section 5: Error Handling).
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc6902.txt
- 2: http://www.ietf.org/rfc/rfc6902.txt
- 3: https://datatracker.ietf.org/doc/html/rfc6902
- 4: https://rfc-editor.org/rfc/rfc6902.html
🌐 Web query:
Kubernetes JSON Patch replace operation optional fields missing null unset
💡 Result:
In Kubernetes JSON Patch (RFC 6902, used with kubectl patch --type=json), the replace operation requires the target path to exist in the resource. If the path (or any prefix) is missing, the operation fails with an error like "jsonpatch replace operation does not apply: doc is missing path". Key distinctions for optional/missing/unset/null fields: - replace: Replaces existing value at path. Path MUST exist (RFC 6902: "The target location MUST exist for the operation to be successful"). Fails if missing. Error example: "doc is missing key: /spec/template/spec/containers/0/image". - add: Adds value if path missing, or replaces if exists. Use for optional fields that might be unset. - remove: Removes value at path. Path must exist. - null values: Setting to null via replace keeps null (not removed). Use remove to delete fields. Kubernetes omits omitempty nulls in JSON, but literal null differs from absence. For missing optional fields, use add first to create parents (no auto-creation of nested objects), then replace/add child. Example from kustomize/CRDs: add "/spec/template/metadata/annotations" as {}, then add/replace sub-keys. JSON Patch differs from JSON Merge Patch (--type=merge): merge auto-creates missing paths/objects, null removes fields, but replaces lists entirely. Use JSON Patch for precise ops on existing structures; fails on missing paths to prevent unintended adds.
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc6902.txt
- 2: https://tools.ietf.org/rfc/rfc6902.txt
- 3: http://www.ietf.org/rfc/rfc6902.txt
- 4: https://stackoverflow.com/questions/57480205/error-while-applying-json-patch-to-kubernetes-custom-resource
- 5: https://pauldally.medium.com/the-most-common-problem-i-see-when-using-json6902-patching-with-kustomize-1d19a0f4a038
- 6: Confusion around add operations on non-existent fields kubernetes-sigs/kustomize#5311
- 7: kubectl patch - bad error message when JSON Patch is invalid JSON kubernetes/kubernetes#63247
- 8: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/
- 9: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch
replace cannot succeed when spec.providerID is initially missing
At line 176, using JSON Patch op replace on /spec/providerID fails on nodes where the field is unset, which is the exact first-write scenario this feature targets. RFC 6902 requires the target path to exist for replace to succeed. Use add instead to both create and update the field.
Proposed fix
patch := []map[string]interface{}{
{
- "op": "replace",
+ "op": "add",
"path": "/spec/providerID",
"value": providerID,
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patch := []map[string]interface{}{ | |
| { | |
| "op": "replace", | |
| "path": "/spec/providerID", | |
| "value": providerID, | |
| }, | |
| patch := []map[string]interface{}{ | |
| { | |
| "op": "add", | |
| "path": "/spec/providerID", | |
| "value": providerID, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/node/updater.go` around lines 174 - 179, The JSON Patch uses the
"replace" op on "/spec/providerID" which fails when spec.providerID is absent;
update the patch construction in pkg/node/updater.go (the variable patch that
includes the map with "path": "/spec/providerID" and "value": providerID) to use
the "add" operation instead of "replace" so the field will be created on
first-write and updated on subsequent writes.
1 participant
Summary
Add automatic Azure provider ID detection and setup via Instance Metadata Service (IMDS). This resolves cluster-autoscaler issues where nodes lack
spec.providerIDand become orphaned from their Azure VM instances.Changes
pkg/detector/azure_provider.go): queries Azure IMDS for resource ID with timeout and graceful fallback for non-Azure environmentspkg/node/updater.go): setsspec.providerIDvia JSON patch with idempotency checkcmd/local-ccm/main.go):--enable-azure-provider-idto enable featureazure.enableProviderIDparameter in values.yamlUsage
Enable in Helm chart:
```yaml
azure:
enableProviderID: true
```
Or via CLI flag:
```bash
--enable-azure-provider-id=true
```
Testing
CGO_ENABLED=0 go buildRelated
Resolves the issue described in https://github.com/cozystack/cozystack/discussions where Azure autoscaler nodes were orphaned due to missing providerID.
Summary by CodeRabbit
New Features
Documentation