Skip to content

Commit 0a0bfb2

Browse files
committed
address copilot review comments
- flexvm: reject allocate_public_ip=true in validateSpec (was silently ignored; previously fell through to a private-only NIC) - flexvm: reject nil kubeadm spec in validateSpec (would nil-panic in CreateOrUpdate when calling AddNodeLabels) - karpenter cloudprovider: stamp AzureFlexNodeClassHashAnnotation on NodeClaim in Create(); without this IsDrifted never triggered - karpenter nodeclass_status: validate subnetID with arm.ParseResourceID instead of a prefix check (was letting malformed IDs through to VM create)
1 parent 9a8fdf5 commit 0a0bfb2

3 files changed

Lines changed: 26 additions & 5 deletions

File tree

karpenter/pkg/cloudproviders/azure/cloudprovider.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,15 @@ func (c *CloudProvider) Create(ctx context.Context, nodeClaim *v1.NodeClaim) (*v
157157
return nil, fmt.Errorf("creating azure-flex agent pool: %w", err)
158158
}
159159

160-
return agentPoolToNodeClaim(created, it), nil
160+
// Stamp the NodeClass drift hash onto the returned NodeClaim so that
161+
// IsDrifted can detect spec changes later. Without this annotation the
162+
// drift check silently no-ops.
163+
out := agentPoolToNodeClaim(created, it)
164+
if out.Annotations == nil {
165+
out.Annotations = map[string]string{}
166+
}
167+
out.Annotations[v1alpha1.AzureFlexNodeClassHashAnnotation] = driftHash(nodeClass.Spec)
168+
return out, nil
161169
}
162170

163171
func (c *CloudProvider) Delete(ctx context.Context, nodeClaim *v1.NodeClaim) error {

karpenter/pkg/controllers/azure/nodeclass_status.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"strings"
77

8+
"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
89
opcontroller "github.com/awslabs/operatorpkg/controller"
910
"github.com/awslabs/operatorpkg/reasonable"
1011
"k8s.io/apimachinery/pkg/api/equality"
@@ -105,6 +106,9 @@ func validateSpec(spec v1alpha1.AzureFlexNodeClassSpec) error {
105106
if !strings.HasPrefix(spec.SubnetID, "/subscriptions/") {
106107
return fmt.Errorf("subnetID %q must be a full ARM resource ID", spec.SubnetID)
107108
}
109+
if _, err := arm.ParseResourceID(spec.SubnetID); err != nil {
110+
return fmt.Errorf("subnetID %q is not a valid ARM resource ID: %w", spec.SubnetID, err)
111+
}
108112
if spec.ImageReference != nil && spec.ImageID != nil && *spec.ImageID != "" {
109113
return fmt.Errorf("imageReference and imageID are mutually exclusive")
110114
}

plugin/pkg/services/agentpools/azure/flexvm/agentpools.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,10 +137,9 @@ func (srv *agentpoolsServer) CreateOrUpdate(ctx context.Context, req *api.Create
137137
},
138138
}
139139
if spec.GetAllocatePublicIp() {
140-
// Phase 1: skip explicit PIP creation — leave a TODO. Per-NodeClass
141-
// public IP is deferred; documented in CRD.
142-
// (Falls through to private-only NIC.)
143-
_ = nicParams // satisfy linter
140+
// validateSpec rejects this; the branch is kept as a compile-time
141+
// reminder for when Phase 2 adds PIP support.
142+
return nil, errors.New("allocate_public_ip=true is not supported in Phase 1")
144143
}
145144
nicPoller, err := nicsClient.BeginCreateOrUpdate(ctx, spec.GetResourceGroup(), nicName, nicParams, nil)
146145
if err != nil {
@@ -357,6 +356,16 @@ func validateSpec(spec *AgentPoolSpec) error {
357356
if st := spec.GetSecurityType(); st != "" && st != "Standard" {
358357
return fmt.Errorf("unsupported security_type %q (only Standard is supported in Phase 1)", st)
359358
}
359+
// Public IP per NIC is not implemented in Phase 1. Reject instead of
360+
// silently creating a private-only NIC when callers expect a public one.
361+
if spec.GetAllocatePublicIp() {
362+
return errors.New("allocate_public_ip=true is not supported in Phase 1")
363+
}
364+
// kubeadm config carries the AKS bootstrap token + CA and is used to
365+
// render userdata; a nil value here would panic in CreateOrUpdate.
366+
if spec.GetKubeadm() == nil {
367+
return errors.New("kubeadm is required")
368+
}
360369
return nil
361370
}
362371

0 commit comments

Comments
 (0)