Make the Provider less powerful

[Callisto13]

Adding new things, specifically new things with tests is quite challenging now due to the all powerful Provider.

The Provider contains all AWS interfaces, and is passed around everywhere, meaning that you can pretty much do anything, anywhere in the codebase.

There are few boundaries or tightly scoped components with clear responsibilities delineated by interfaces: nearly everything gets a Provider and needs little else to get things done.

This makes unit tests particularly hard, and not very "unity", as you often end up inadvertently testing things several layers down the stack in neighbouring packages. Unit tests being what they are: you should not need to think about stuff happening elsewhere.

This ticket is about fixing some of this, and namely removing some of the monopoly from the Provider. How that is done is player's choice 😉 .

[aclevername]

What is the provider?

The ClusterProvider is an inteface

// ClusterProvider is the interface to AWS APIs
type ClusterProvider interface {
	CloudFormation() cloudformationiface.CloudFormationAPI
	CloudFormationRoleARN() string
	CloudFormationDisableRollback() bool
	EKS() eksiface.EKSAPI
	EC2() ec2iface.EC2API
	ELB() elbiface.ELBAPI
	ELBV2() elbv2iface.ELBV2API
	STS() stsiface.STSAPI
	SSM() ssmiface.SSMAPI
	IAM() iamiface.IAMAPI
	CloudTrail() cloudtrailiface.CloudTrailAPI
	Region() string
	Profile() string
	WaitTimeout() time.Duration
}

The interface prescribes a powerful struct that provides the ability to interact with various AWS apis, e.g. CloudFormation and EKS.

Whats wrong with it?

Used too widely

We pass a struct implementation of this interface around everywhere- doing a code lookup finds its referenced 36 times. We pass this all-powerful struct into method/functions that only require a very slim subsection of its capabilitys, e.g. in the Manager package we only ever make provider.CloudFormation and provider.EKS calls. Giving the package the provider can lead to us to continue to over-use it, e.g. making EC2 calls in the manager package when perhaps this logic should be abstracted out into a different location. Having this powerful provider everywhere tempts us into bad design choices (should the manager even be making EKS calls?)

Hard to test

The provider is the central point for doing almost all of the mocking in the code base. We don't have interfaces sitting at a higher abstraction. If for example I want to test that the eksctl drain nodegroup logic in https://github.com/weaveworks/eksctl/blob/master/pkg/actions/nodegroup/delete.go I have to pass in my mock provider, and then mockout calls that occur in the manger package because I can't mock the manager package itself. If then down the line someone refactored the logic inside manger to make a new provider call, I would have to also update the tests for the drain package. We end up testing sub-packages and this results in us having a high cost to making changes

How can we fix this?

There are main ways we can fix this:

• Stop passing around the whole provider, and instead starts to just pass in the subsections needed. E.g. pass in the provider.EKS and provider.CloudFormation into manager to reduce its scope
• Start creating interfaces for packages that interact with external APIs. For example the Addon package has its own interface for the manager package, making the tests easy to write and allowing changes to the manager package without breaking any tests https://github.com/weaveworks/eksctl/blob/master/pkg/actions/addon/addon.go#L30-L35

Once the above two are complete we can look into removing the interface and struct itself, if we only ever pass in the subsections of the provider then it doesn't need to exist.

[Callisto13]

Start creating interfaces for packages that interact with external APIs. For example the Addon package has its own interface for the manager package, making the tests easy to write and allowing changes to the manager package without breaking any tests https://github.com/weaveworks/eksctl/blob/master/pkg/actions/addon/addon.go#L30-L35

Would this be like a wrapper around each external API, the interface for which can then be passed to packages which need it? Or do you mean that each package would define its own version of the interface it needs? I'm guessing the first but would like to clarify.

[aclevername]

Start creating interfaces for packages that interact with external APIs. For example the Addon package has its own interface for the manager package, making the tests easy to write and allowing changes to the manager package without breaking any tests https://github.com/weaveworks/eksctl/blob/master/pkg/actions/addon/addon.go#L30-L35

Would this be like a wrapper around each external API, the interface for which can then be passed to packages which need it? Or do you mean that each package would define its own version of the interface it needs? I'm guessing the first but would like to clarify.

I think the first is the easiest, but the second is the nicest in that its lets you right even smaller interfaces, but its probably not worth the cost of constantly rewriting.

[Callisto13]

Yeh I think the second may also end up being a bit confusing: lots of partially duplicated interfaces all over the shop.

[aclevername]

@cPu1 @michaelbeaumont thoughts?

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[nikimanoledaki]

Very happy to see that there is a long-term goal about ClusterProvider! :) Here are some thoughts after doing some work around this in #3728.

What is the provider? (Part 2)

As pointed out by @aclevername, the ClusterProvider is an interface for making calls to various AWS APIs, but it is also a struct that contains many methods that are not in any interface. These are often helper functions or logic used across other packages.

The ClusterProvider has methods in the following files in pkg/eks:

├── api.go
├── cache.go
├── client.go
├── compatibility.go
├── eks.go
├── nodegroup.go
├── tasks.go
└── update.go

What's the problem?

Interface-less methods are hard to mock

Since these methods are not included in any interface, fakes cannot be generated for them, which makes them hard to mock. This contributes to having untested areas in other packages, as was the case for pkg/actions/nodegroup/create.go.

Methods are tightly coupled with the receiver's fields and other methods

Another issue is that these methods are often making calls to the ClusterProvider receiver: they are tightly coupled to the other fields of the struct, Provider and the Status, or to other methods of the Provider. This dependency makes extracting methods out of ClusterProvider more difficult.

How can we fix this?

• Refactor and increase test coverage for creating nodegroups #3728 refactored the ClusterProvider by going through pkg/actions/nodegroup/create.go and refactoring one call to the Provider (ctl) at a time.
• As indicated by the file names in the tree above^, methods are arranged according to their functionality, which is a good thing and can help with refactoring them into other structs/interfaces. In Refactor and increase test coverage for creating nodegroups #3728, it was fairly straightforward to move methods from pkg/eks/nodegroup.go to the NodeGroupInitialiser interface and NewNodeGroupService struct.
• Given the dependency of some methods with the ClusterProvider, it was difficult to extract them from the Provider straight away. A good starting point is to create new interfaces for them. In Refactor and increase test coverage for creating nodegroups #3728, a few of these methods were bundled into a new KubeProvider interface. Fakes were then generated for it and used to mock calls to these functions in the unit tests.

[Skarlso]

The next step was to separate some of the functions out from provider.
#5259
This PR took kubeprovider and made it a bit more kube specific and refactored a tiny bit about the Provider.

[Himangini]

Closing this off since we have split a lot of this work into smaller tickets which we will be working on.