CAS: cloudprovider-specific nodegroupset

[jackfrancis]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR moves the cloudprovider-specific implementations of nodegroupset into cloudprovider-managed code areas so that they can be more easily maintained without involving core cluster-autoscaler maintainers.

This should not have any impact on compilation outcomes, the idea is to simply re-organize code.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[jackfrancis]

/assign @x13n

This follows along from #6634

Not sure why the verify test is failing, UT are passing when I run make test-unit.

[jackfrancis]

/retest

[jackfrancis]

/retest

[jackfrancis]

/retest

[nojnhuh]

/test pull-cluster-autoscaler-e2e-azure

[jackfrancis]

@x13n @gjtempleton @MaciekPytel this one is ready for a final review

[x13n]

Apologies for taking so long to review. Looks good, just one minor comment. Feel free to cancel the hold if you disagree.

/lgtm
/approve
/hold

[x13n]

nit: Since aws is already the package name, maybe rename to just CreateNodeInfoComparator to avoid redundancy? Same for azure & gce.

[jackfrancis]

Fair enough. A better way would be to define an interface once, but the value of this cleanup work (IMO) doesn't warrant that type of surgery.

Done!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, x13n

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• cluster-autoscaler/OWNERS [x13n]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jackfrancis]

/hold cancel

[x13n]

Thanks!

/lgtm

[MaciekPytel]

This PR seems to have broken the ability to build CA with just one provider. Specifically:

• Let's be honest - the bar for adding CA provider is not very high. Provider owners have full autonomy to develop their code.
• A malicious actor hiding something nefarious in an init module somewhere deep in an obscure cloudprovider is a risk.

For this reason we've introduced https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/builder as the only import path for cloudproviders and have each provider gated on build tags. This allows using build tags to build CA with just the provider you want (and presumably trust) to mitigate the risk above.

This PR I think breaks this mechanism by introducing a new import path for individual cloudprovider implementations. I think the right solution would be a refactor that allows cloudproviders to register their processors via cloudprovider/builder mechanism.
In the meantime I think we should rollback this PR. WDYT @jackfrancis @x13n

note: it is not lost on me that we take a different security stance regarding cloudprovider code and provider-specific processors, but practically speaking the possibility to hide something nasty is vastly different between the two - not least because of set of approvers needed to add a processor today.

[jackfrancis]

@MaciekPytel I agree 100%, TIL about provider-specific build flows

I'll revert this now.

Do we want to add additional test coverage that enumerates through the set of provider build tags so that we catch this kind of thing in CI next time?

[x13n]

Yeah, good point. Revert + follow up refactor of cloudprovider/builder to allow compilation for a specific cloud provider makes sense to me.

[x13n]

And re: CI - that is also reasonable. If we say we support building CA with specific build tags, we should test whether these builds actually work.

[MaciekPytel]

Big +1 to all the comments above.

[jackfrancis]

@MaciekPytel @x13n see #7391 and kubernetes/test-infra#33656