Acn Go Design Boundaries

// ❌ BAD — scenario name leaks deep into the stack
func createNCRequestFromStaticNCHelper(config *CNSConfig, nc NetworkContainer) Request {
    if config.ChannelMode == "SwiftV2" {
        // process primary prefix IP
    }
}
// Problems:
// 1. Not reusable if a new scenario wants the same behavior
// 2. Requires threading a whole scenario name through the stack
// 3. Low-level code depends on high-level config types

// ✅ GOOD — behavioral parameter
func createNCRequestFromStaticNCHelper(processPrimaryIP bool, nc NetworkContainer) Request {
    if processPrimaryIP {
        // process primary prefix IP
    }
}
// The caller already knows the scenario and passes the bool
// Any future scenario that needs this behavior works without code changes here

// ❌ BAD — function needs the whole CNS config to check one thing
func setupHealthCheck(config *configuration.CNSConfig) error {
    if config.ChannelMode == "CRD" {
        // enable NNC health check
    }
}

// ✅ GOOD — pass the behavioral decision
func setupHealthCheck(enableNNCCheck bool) error {
    if enableNNCCheck {
        // enable NNC health check
    }
}
// The caller (main/init) makes the scenario→behavior mapping

// ❌ BAD — mutates input, side effects, nil-pointer risk
func addACLPolicies(podIPInfo *cns.PodIpInfo, policies []policy.Policy) {
    // impossible to tell from the call site that podIPInfo is modified
    // also: nil pointer exception if podIPInfo is nil
    podIPInfo.EndpointPolicies = append(podIPInfo.EndpointPolicies, policies...)
}

// ✅ GOOD — pure function, returns result, no side effects
func buildACLPolicies(existingPolicies []policy.Policy, newPolicies []policy.Policy) []policy.Policy {
    return append(existingPolicies, newPolicies...)
}
// Caller explicitly assigns: info.EndpointPolicies = buildACLPolicies(info.EndpointPolicies, new)

// ❌ BAD — "validator" mutates the request while checking it
func validateReleaseRequest(req *IPConfigsRequest) error {
    if req == nil {
        return fmt.Errorf("request must not be nil")
    }
    if req.NetworkContainerID == "" {
        req.NetworkContainerID = deriveNCID(req.PodInfo) // hidden mutation
    }
    if len(req.IPsToRelease) == 0 {
        req.IPsToRelease = append(req.IPsToRelease, req.PodInfo.PodIP) // more hidden mutation
    }
    return nil
}

// ✅ GOOD — validator only reports validity
func validateReleaseRequest(req IPConfigsRequest) error {
    if req.NetworkContainerID == "" {
        return fmt.Errorf("networkContainerID is required")
    }
    if len(req.IPsToRelease) == 0 {
        return fmt.Errorf("at least one IP must be released")
    }
    return nil
}

// ❌ BAD — validates and mutates one IP at a time; partial release is possible
func (m *IPAMManager) releaseSecondaryIPs(ctx context.Context, req IPConfigsRequest) error {
    for _, ip := range req.IPsToRelease {
        if err := validateReleaseIP(req.NetworkContainerID, ip, m.assignedIPsByNC); err != nil {
            return err
        }
        if err := m.client.ReleaseIPs(ctx, IPConfigsRequest{
            NetworkContainerID: req.NetworkContainerID,
            IPsToRelease:       []string{ip},
        }); err != nil {
            return err
        }
    }
    return nil
}
// If the third IP is invalid, the first two may already be gone.

// ✅ GOOD — preflight the whole batch, then perform one mutation step
type releasePlan struct {
    ncID string
    ips  []string
}

func buildReleasePlan(ncID string, requested []string, assigned map[string]struct{}) (releasePlan, error) {
    seen := map[string]struct{}{}
    for _, ip := range requested {
        if _, ok := assigned[ip]; !ok {
            return releasePlan{}, fmt.Errorf("ip %s is not assigned to %s", ip, ncID)
        }
        if _, dup := seen[ip]; dup {
            return releasePlan{}, fmt.Errorf("duplicate IP %s in release request", ip)
        }
        seen[ip] = struct{}{}
    }
    return releasePlan{ncID: ncID, ips: requested}, nil
}

func (m *IPAMManager) releaseSecondaryIPs(ctx context.Context, req IPConfigsRequest) error {
    plan, err := buildReleasePlan(req.NetworkContainerID, req.IPsToRelease, m.assignedIPsByNC[req.NetworkContainerID])
    if err != nil {
        return err
    }
    return m.client.ReleaseIPs(ctx, IPConfigsRequest{
        NetworkContainerID: plan.ncID,
        IPsToRelease:       plan.ips,
    })
}

// ❌ BAD — constructor for struct with no initialization logic
func NewIPTablesClient() *Client {
    return &Client{}
}
// Adds a function that does nothing. Just use &iptables.Client{}

// ✅ GOOD — constructor enforces required dependencies
func NewService(logger *zap.Logger, store UserStore) *Service {
    if logger == nil {
        panic("logger must not be nil") // fail fast, not at first log call
    }
    return &Service{logger: logger, store: store}
}
// If I let you construct directly and you forget the logger, everything NPEs

// ❌ BAD — helper takes more space than inline and obscures what happens
func formatError(operation string, err error) error {
    return fmt.Errorf("failed to %s: %w", operation, err)
}
// ... 
return formatError("create endpoint", err)

// ✅ GOOD — inline is shorter and self-documenting
return fmt.Errorf("creating endpoint: %w", err)

// ❌ BAD — passing nil to configure behavior
func ReconcileIPAMState(nnc *v1alpha.NodeNetworkConfig) {
    if nnc == nil {
        // different behavior when nil
        return
    }
    // normal behavior
}
// Problems:
// 1. Hard to discover — how do you know nil means "skip"?
// 2. In normal scenarios, nil NNC is a fatal error we should catch
// 3. Accidentally passing nil gives wrong behavior instead of an error

// ✅ GOOD — validate inputs immediately, use explicit flags for behavior
func ReconcileIPAMState(nnc *v1alpha.NodeNetworkConfig, skipReconcile bool) error {
    if skipReconcile {
        return nil
    }
    if nnc == nil {
        // this is now clearly an error, not a feature
        return fmt.Errorf("nnc must not be nil")
    }
    // ...
    return nil
}

// ❌ BAD — scenario branching deep in the stack
func (w *Watcher) ReleaseIPs(ctx context.Context, req IPConfigsRequest) error {
    if w.isStatelessCNIWindows {  // scenario knowledge leaked!
        w.deleteHNSEndpoint(req)
    }
    return w.client.ReleaseIPs(ctx, req)
}

// ✅ GOOD — compose behavior at initialization
// In main:
var releaseIPs ReleaseIPsFunc = cnsClient.ReleaseIPs
if isStatelessCNIWindows(config) {
    releaseIPs = withHNSCleanup(cnsClient) // decorator wraps the base
}
watcher := fsnotify.NewWatcher(releaseIPs)
// Watcher has no knowledge of scenarios — zero changes to the fsnotify package

// ❌ BAD — single threshold, churns external state on every loop
func (m *Manager) reconcilePool(ctx context.Context, freeIPs int) error {
    if freeIPs < m.targetFree {
        return m.client.RequestIPs(ctx, m.batchSize)
    }
    if freeIPs > m.targetFree {
        return m.client.ReleaseIPs(ctx, freeIPs-m.targetFree)
    }
    return nil
}
// One pod create/delete around targetFree can alternate RequestIPs/ReleaseIPs forever.

// ✅ GOOD — hysteresis + sticky/in-flight state
type poolControl struct {
    requestInFlight bool
    releaseInFlight bool
}

func (m *Manager) reconcilePool(ctx context.Context, freeIPs int, ctl *poolControl) error {
    switch {
    case ctl.requestInFlight || ctl.releaseInFlight:
        return nil // wait for the previous external change to settle
    case freeIPs <= m.lowWatermark:
        ctl.requestInFlight = true
        return m.client.RequestIPs(ctx, m.batchSize)
    case freeIPs >= m.highWatermark:
        ctl.releaseInFlight = true
        return m.client.ReleaseIPs(ctx, freeIPs-m.targetFree)
    default:
        return nil // stay sticky inside the band
    }
}

// ❌ BAD — manual channel-based error collection
errs := make(chan error)
go func() { errs <- watchPendingDelete(ctx) }()
go func() { errs <- watchFS(ctx) }()
err := <-errs // only gets first error, leaks the other goroutine

// ✅ GOOD — errgroup.WithContext
g, groupCtx := errgroup.WithContext(ctx)
g.Go(func() error { return w.watchPendingDelete(groupCtx) })
g.Go(func() error { return w.watchFS(groupCtx) })
return g.Wait() // cancels groupCtx on first error, waits for all

// ❌ BAD — silently swallows error, returns broken watcher
func New(path string, logger *zap.Logger) *watcher {
    if err := os.Mkdir(path, 0o755); err != nil {
        logger.Error("error making directory", zap.Error(err))
        // continues with broken state!
    }
    return &watcher{path: path}
}

// ✅ GOOD — caller can handle the failure
func New(path string, logger *zap.Logger) (*watcher, error) {
    if err := os.Mkdir(path, 0o755); err != nil && !errors.Is(err, fs.ErrExist) {
        return nil, errors.Wrapf(err, "failed to create dir %s", path)
    }
    return &watcher{path: path}, nil
}

// ❌ BAD — synchronous metrics in request handler
func (s *Service) RequestIPConfig(ctx context.Context, req Request) Response {
    resp := s.allocateIP(req)
    s.publishMetrics(ctx)  // blocks on metrics publish!
    return resp
}

// ✅ GOOD — defer async recording
func (s *Service) RequestIPConfig(ctx context.Context, req Request) Response {
    defer s.recorder.Record()  // triggers async channel-based worker
    return s.allocateIP(req)
}

// Recorder uses sync.Once + channel worker pattern
type Recorder struct {
    once sync.Once
    ch   chan struct{}
}

func (r *Recorder) Record() {
    r.once.Do(func() { go r.worker() })
    select {
    case r.ch <- struct{}{}:
    default: // drop if worker is busy — metrics are best-effort
    }
}

// ❌ BAD — silently overwrites duplicate, hides corruption
for _, ip := range ips {
    state[ip.Address] = ip // duplicate? too bad, last write wins
}

// ✅ GOOD — detect and fail immediately
var ErrDuplicateIP = errors.New("duplicate IP in state")

for _, ip := range ips {
    if _, exists := state[ip.Address]; exists {
        return fmt.Errorf("%w: %s", ErrDuplicateIP, ip.Address)
    }
    state[ip.Address] = ip
}