-
Notifications
You must be signed in to change notification settings - Fork 1
feat(azure): add provider ID detection for cluster-autoscaler integration #3
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
94072b7
f5e1563
b4c30ff
d5f072e
4d669d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,92 @@ | ||||||||||||||||||||
| /* | ||||||||||||||||||||
| Copyright 2025 The local-ccm Authors. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||
| you may not use this file except in compliance with the License. | ||||||||||||||||||||
| You may obtain a copy of the License at | ||||||||||||||||||||
|
|
||||||||||||||||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||
| See the License for the specific language governing permissions and | ||||||||||||||||||||
| limitations under the License. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| package detector | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import ( | ||||||||||||||||||||
| "context" | ||||||||||||||||||||
| "fmt" | ||||||||||||||||||||
| "io" | ||||||||||||||||||||
| "net/http" | ||||||||||||||||||||
| "strings" | ||||||||||||||||||||
| "time" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| "k8s.io/klog/v2" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const ( | ||||||||||||||||||||
| // AzureIMDSURL is the Azure Instance Metadata Service endpoint | ||||||||||||||||||||
| AzureIMDSURL = "http://169.254.169.254/metadata/instance/compute/resourceId?api-version=2021-02-01&format=text" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // AzureIMDSTimeout is the timeout for IMDS requests | ||||||||||||||||||||
| AzureIMDSTimeout = 2 * time.Second | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // 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, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+44
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| // 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 | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+38
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Creating a new 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
} |
||||||||||||||||||||
| defer resp.Body.Close() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Check response status | ||||||||||||||||||||
| if resp.StatusCode != http.StatusOK { | ||||||||||||||||||||
| klog.V(2).Infof("Azure IMDS returned non-OK status: %d", resp.StatusCode) | ||||||||||||||||||||
| return "", nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Read response body | ||||||||||||||||||||
| body, err := io.ReadAll(resp.Body) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| return "", fmt.Errorf("failed to read IMDS response: %w", err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| resourceID := strings.TrimSpace(string(body)) | ||||||||||||||||||||
| if resourceID == "" { | ||||||||||||||||||||
| klog.V(2).Info("Azure IMDS returned empty resource ID") | ||||||||||||||||||||
| return "", nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Format as Kubernetes provider ID | ||||||||||||||||||||
| providerID := fmt.Sprintf("azure://%s", resourceID) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| klog.V(2).Infof("Detected Azure provider ID: %s", providerID) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return providerID, nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -147,3 +147,57 @@ func (u *Updater) RemoveTaint(ctx context.Context) error { | |||||||||||||||||||||||||
| func (u *Updater) GetNode(ctx context.Context) (*v1.Node, error) { | ||||||||||||||||||||||||||
| return u.client.CoreV1().Nodes().Get(ctx, u.nodeName, metav1.GetOptions{}) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // 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) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // 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, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
Comment on lines
+174
to
+179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
💡 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:
🌐 Web query:
💡 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:
At line 176, using JSON Patch op Proposed fix patch := []map[string]interface{}{
{
- "op": "replace",
+ "op": "add",
"path": "/spec/providerID",
"value": providerID,
},
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||
|
Comment on lines
+152
to
+202
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Additionally, consider using the // 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
} |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ProviderIDis already populated will significantly reduce unnecessary network traffic and API pressure.