Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Cloud provider interface #3

Closed
wants to merge 4 commits into from

Conversation

abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Feb 9, 2018

Add an pkg cloudprovider.

Also tests travisci.

/cc: @ericchiang

)

// Interface defines functions that need to be implemented by cloudproviders.
type Interface interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's too early to have a cloud provider interface. Best we could probably do is create boundaries between the Kubernetes client code and the cloud provider code.

Can we build something first, then try to figure out what interfaces we need? Interfaces usually make more sense when the fall out naturally from the use cases rather than trying to define them up front.

@ericchiang
Copy link
Contributor

I would have imagined something like:

cmd/kube-aws-approver/main.go
pkg/approver/aws/aws.go

@ericchiang
Copy link
Contributor

Then as we want to expand our use-cases and extend to different cloud, we can figure out a more detailed package structure and interfaces.

@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Feb 9, 2018

When was testing with approver in #1 , And I think an interface like

// InstanceID returns the cloud provider ID of the node with the specified NodeName.
InstanceID(ctx context.Context, nodeName string) (string, error)
// InstanceExistsByID returns true if the instance for the given provider id still is running.
InstanceExistsByID(ctx context.Context, id string) (bool, error)
// InstanceGroupID returns the cloud provider ID of the parent instance group of the specified NodeName.
InstanceGroupID(ctx context.Context, nodeName string) (string, error)

gives the required functionality for approver to match CN to instanceid in csr.spec.Username and a valid ASG.

With atleast the interface down for a minute I can test nodeapprover functions with unit tests...

@abhinavdahiya
Copy link
Contributor Author

If it makes more sense I can change the dir structure to look like

pkg/
    nodeapprover/
        cloudprovider/

@ericchiang
Copy link
Contributor

Package APIs are waaaaay harder to design than just writing code. My general recommendation is:

  • Don't export anything you don't have to
  • Avoid interfaces like the plague
  • Avoid mocks

I've never seen someone do a cloud provider package well.

I would imagine that the package API would look something like:

package aws

type Config struct {
    Kubernetes rest.Config
    
    AWS aws.Config

    // other options like what autoscaling groups you're allowing
}

type Approver struct {
    // all fields un-exported
}

func New(c *Config) (*Approver, error) {
}

func (a *Approver) Run(ctx context.Context) {
}

Then after we build that, we can figure out what parts are re-usable from a general sense.

@ericchiang
Copy link
Contributor

closing in favor of #5

@ericchiang ericchiang closed this Feb 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants