Skip to content

kube, forge: delete unused cluster and Azure helpers - #678

Open
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-kube-forge
Open

kube, forge: delete unused cluster and Azure helpers#678
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-kube-forge

Conversation

@plombardi89

Copy link
Copy Markdown
Collaborator

Part of the #670 dead-code sweep, split by component. The tooling that produced these findings is #673; this PR is deletions only and is independent of the other eight — the file sets are disjoint, so they can be reviewed and merged in any order and in parallel.

Why two analyzers were needed to find this

unused cannot see dead exported code under internal/. That is a documented design decision, not a gap: unused/unused.go:48-62 holds that a package uses its exported named types and a named type uses its exported methods, so every method on an exported type is transitively live because the type is exported.

x/tools/cmd/deadcode does not close that gap on its own either. x/tools/go/callgraph/rta/rta.go:281-287,433-437 — converting a value to an interface materializes its runtime type and marks every exported method of that type reachable, because reflection could call any of them. One var i any = x anywhere buys all of x's exported methods a clean bill of health.

So #673 adds both deadcode and hack/cmd/unreferenced, which resolves identifiers instead of tracing reachability. Neither subsumes the other, and findings below are attributed to whichever one saw them.


kube, forge: delete unused cluster and Azure helpers

internal/kube:

  • configmap.go goes entirely; ApplyConfigMap had no caller.
  • CheckKubectlAvailable, KubectlCmd and its private streamLogs are dead.
    Kubectl and KubectlFunc stay: cmd/kubectl-unbounded and two forge packages
    use them to build the command themselves.
  • ApplyManifestsInDirectory had no caller outside its own tests. The live
    entry point is ApplyManifests, which takes bytes and is called from
    site_init.go and machine_register.go. Its five tests
    (TestApplyManifestsV2_WalksDirectory, _NotADirectory, _DirectoryNotFound,
    _EmptyDirectory, _SkipPaths) go with it; they were the only thing keeping
    the directory walk alive.

BootstrapToken.String in internal/kube is reported dead and is kept, with a
comment saying so. It redacts the secret, so its absence is what would be the
bug: the hazard shows up the first time anyone formats the struct into a log
line. This is the one case in this series where "no caller" is the wrong reason
to delete.

The identically named method in hack/cmd/forge/forge/kube does the opposite -
it interpolates the raw secret - and that one is deleted rather than kept. The
two packages are grouped in one change because that contrast is the argument.

hack/cmd/forge:

  • subnet.go, virtual_machine.go and network_interface.go go entirely.
    SubnetManager, VirtualMachineManager and NetworkInterfaceManager have no
    constructor anywhere; forge provisions through the AKS and VMSS paths.

  • ManagedClusterBuilder.DNSServiceIP, ManagedClusterBuilder.WithGeneratedSSH,
    AzureKubernetesClusterManager.AddAgentPool and KeyVaultManager.Delete.

  • AzureKubernetesClusterManager.GetAgentPool is a second-order finding: it
    shared the AgentPoolsCli field with AddAgentPool, and with both gone the
    field has no reader, and neither does the
    ClientSet.ManagedClusterAgentPoolsClient that filled it. The whole
    agent-pool client plumbing goes.

    That chain is what unused structurally cannot reach: a struct field whose
    only remaining mention is the assignment that populates it counts as used
    under field-writes-are-uses, so a dead method leading to a dead client
    factory call stays invisible until someone follows it by hand.

  • ClientSet.Credential and ClientSet.CurrentIdentityType.
    ClientSet.NewBlobStorageClient was the package's only use of the azblob
    module.

  • kube/secret.go; ApplySecret had no caller.

  • defaultGatewayPoolAgentPoolName, an unexported constant in a const group
    whose other member is live, which is why unused never reported it.

Refs #670


Verification

go build ./..., go vet ./..., golangci-lint over the touched trees (0 issues), make fmt producing no diff, and go build -tags e2e,integrationtest,storageboundary ./.... Full suite runs in CI.

The eight deletion branches were reassembled onto the tooling commits and diffed against the original combined branch: byte-identical, so nothing was lost or duplicated in the split.

internal/kube:

  - configmap.go goes entirely; ApplyConfigMap had no caller.
  - CheckKubectlAvailable, KubectlCmd and its private streamLogs are dead.
    Kubectl and KubectlFunc stay: cmd/kubectl-unbounded and two forge packages
    use them to build the command themselves.
  - ApplyManifestsInDirectory had no caller outside its own tests. The live
    entry point is ApplyManifests, which takes bytes and is called from
    site_init.go and machine_register.go. Its five tests
    (TestApplyManifestsV2_WalksDirectory, _NotADirectory, _DirectoryNotFound,
    _EmptyDirectory, _SkipPaths) go with it; they were the only thing keeping
    the directory walk alive.

BootstrapToken.String in internal/kube is reported dead and is kept, with a
comment saying so. It redacts the secret, so its absence is what would be the
bug: the hazard shows up the first time anyone formats the struct into a log
line. This is the one case in this series where "no caller" is the wrong reason
to delete.

The identically named method in hack/cmd/forge/forge/kube does the opposite -
it interpolates the raw secret - and that one is deleted rather than kept. The
two packages are grouped in one change because that contrast is the argument.

hack/cmd/forge:

  - subnet.go, virtual_machine.go and network_interface.go go entirely.
    SubnetManager, VirtualMachineManager and NetworkInterfaceManager have no
    constructor anywhere; forge provisions through the AKS and VMSS paths.
  - ManagedClusterBuilder.DNSServiceIP, ManagedClusterBuilder.WithGeneratedSSH,
    AzureKubernetesClusterManager.AddAgentPool and KeyVaultManager.Delete.
  - AzureKubernetesClusterManager.GetAgentPool is a second-order finding: it
    shared the AgentPoolsCli field with AddAgentPool, and with both gone the
    field has no reader, and neither does the
    ClientSet.ManagedClusterAgentPoolsClient that filled it. The whole
    agent-pool client plumbing goes.

    That chain is what `unused` structurally cannot reach: a struct field whose
    only remaining mention is the assignment that populates it counts as used
    under field-writes-are-uses, so a dead method leading to a dead client
    factory call stays invisible until someone follows it by hand.

  - ClientSet.Credential and ClientSet.CurrentIdentityType.
    ClientSet.NewBlobStorageClient was the package's only use of the azblob
    module.
  - kube/secret.go; ApplySecret had no caller.
  - defaultGatewayPoolAgentPoolName, an unexported constant in a const group
    whose other member is live, which is why `unused` never reported it.

Refs #670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant