Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breakup all the Broker Clients into a clients pkg #222

Merged
merged 4 commits into from
Jun 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 10 additions & 50 deletions pkg/apb/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
restclient "k8s.io/client-go/rest"

"github.com/openshift/ansible-service-broker/pkg/clients"
"github.com/pborman/uuid"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/homedir"
"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
)
Expand All @@ -40,8 +39,6 @@ First cut might have to pass kubecfg from broker. FIRST SPRINT broker passes use
admin/admin
*/

var DockerSocket = "unix:///var/run/docker.sock"

type ClusterConfig struct {
InCluster bool
Target string
Expand All @@ -56,53 +53,16 @@ type Client struct {
log *logging.Logger
}

func createClientConfigFromFile(configPath string) (*restclient.Config, error) {
clientConfig, err := clientcmd.LoadFromFile(configPath)
if err != nil {
return nil, err
}

config, err := clientcmd.NewDefaultClientConfig(*clientConfig, &clientcmd.ConfigOverrides{}).ClientConfig()
if err != nil {
return nil, err
}
return config, nil
}

func NewClient(log *logging.Logger) (*Client, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we push this into the clients package? I think it makes more sense there. Related to my overall comment on apb/client.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eriknelson I agree, but this is a much larger change since Client is a class. So things like RunImage need to be distributed out of client.go. For now, I'm going to keep this in place, but adding a TODO in the final patch for this series and we can return to it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rthallisey since I'm requesting the change, mind if I take a crack at it and submit for your review? Think I could probably just explain myself better with code anyways. I think it's totally fine to accept this PR without it and add the TODO for a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eriknelson That would be fantastic 👍

dockerClient, err := docker.NewClient(DockerSocket)
if err != nil {
log.Error("Could not load docker client")
return nil, err
}

// NOTE: Both the external and internal client object are using the same
// clientset library. Internal clientset normally uses a different
// library
clientConfig, err := restclient.InClusterConfig()
if err != nil {
log.Warning("Failed to create a InternalClientSet: %v.", err)

log.Debug("Checking for a local Cluster Config")
clientConfig, err = createClientConfigFromFile(homedir.HomeDir() + "/.kube/config")
if err != nil {
log.Error("Failed to create LocalClientSet")
return nil, err
}
}

clientset, err := clientset.NewForConfig(clientConfig)
if err != nil {
log.Error("Failed to create LocalClientSet")
return nil, err
}

rest := clientset.CoreV1().RESTClient()
//TODO: This object gets created each provision, bind, deprovision,
// and unbind. Instead, those functions should be using the global
// clients were needed and this class needs to be reworked.
k8s := clients.Clients.KubernetesClient

client := &Client{
dockerClient: dockerClient,
ClusterClient: clientset,
RESTClient: rest,
dockerClient: clients.Clients.DockerClient,
ClusterClient: k8s,
RESTClient: k8s.CoreV1().RESTClient(),
log: log,
}

Expand Down Expand Up @@ -178,14 +138,14 @@ func (c *Client) RunImage(
}

c.log.Notice(fmt.Sprintf("Creating pod %q in the %s namespace", pod.Name, ns))
_, err = c.ClusterClient.CoreV1().Pods(ns).Create(pod)
_, err = clients.Clients.KubernetesClient.CoreV1().Pods(ns).Create(pod)

return apbId, err
}

func (c *Client) PullImage(imageName string) error {
// Under what circumstances does this error out?
c.dockerClient.PullImage(docker.PullImageOptions{
clients.Clients.DockerClient.PullImage(docker.PullImageOptions{
Repository: imageName,
OutputStream: os.Stdout,
}, docker.AuthConfiguration{})
Expand Down
29 changes: 21 additions & 8 deletions pkg/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/openshift/ansible-service-broker/pkg/apb"
"github.com/openshift/ansible-service-broker/pkg/broker"
"github.com/openshift/ansible-service-broker/pkg/clients"
"github.com/openshift/ansible-service-broker/pkg/dao"
"github.com/openshift/ansible-service-broker/pkg/handler"
)
Expand Down Expand Up @@ -48,6 +49,8 @@ func CreateApp() App {
fmt.Println("== Starting Ansible Service Broker... ==")
fmt.Println("============================================================")

//TODO: Let's take all these validations and delegate them to the client
// pkg.
if app.config, err = CreateConfig(app.args.ConfigFile); err != nil {
os.Stderr.WriteString("ERROR: Failed to read config file\n")
os.Stderr.WriteString(err.Error())
Expand All @@ -60,12 +63,14 @@ func CreateApp() App {
os.Exit(1)
}

app.log.Debug("Connecting Dao")
if app.dao, err = dao.NewDao(app.config.Dao, app.log.Logger); err != nil {
app.log.Error("Failed to initialize Dao\n")
app.log.Debug("Creating Etcd Client")
if err = clients.NewEtcd(app.config.Dao, app.log.Logger); err != nil {
app.log.Error("Failed to initialize Etcd Client\n")
app.log.Error(err.Error())
os.Exit(1)
}

app.log.Debug("Connecting to Etcd")
serv, clust, err := app.dao.GetEtcdVersion(app.config.Dao)
if err != nil {
app.log.Error("Failed to connect to Etcd\n")
Expand All @@ -74,16 +79,18 @@ func CreateApp() App {
}
app.log.Info("Etcd Version [Server: %s, Cluster: %s]", serv, clust)

app.log.Debug("Creating Cluster Client")
client, err := apb.NewClient(app.log.Logger)
if err != nil {
app.log.Debug("Connecting Dao")
app.dao = dao.NewDao(app.config.Dao, app.log.Logger)

app.log.Debug("Creating Kubernetes Client")
if err = clients.NewKubernetes(app.log.Logger); err != nil {
app.log.Error(err.Error())
os.Exit(1)
}
app.log.Info("Cluster Client Created")

app.log.Debug("Connecting to Cluster")
body, err := client.RESTClient.Get().AbsPath("/version").Do().Raw()

body, err := clients.Clients.RESTClient.Get().AbsPath("/version").Do().Raw()
if err != nil {
app.log.Error(err.Error())
os.Exit(1)
Expand All @@ -103,6 +110,12 @@ func CreateApp() App {
os.Exit(1)
}

app.log.Debug("Creating Docker Client")
if err = clients.NewDocker(app.log.Logger); err != nil {
app.log.Error(err.Error())
os.Exit(1)
}

app.log.Debug("Connecting Registry")
if app.registry, err = apb.NewRegistry(
app.config.Registry, app.log.Logger,
Expand Down
4 changes: 2 additions & 2 deletions pkg/app/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import (

"github.com/openshift/ansible-service-broker/pkg/apb"
"github.com/openshift/ansible-service-broker/pkg/broker"
"github.com/openshift/ansible-service-broker/pkg/dao"
"github.com/openshift/ansible-service-broker/pkg/clients"
)

type Config struct {
Registry apb.RegistryConfig
Dao dao.Config
Dao clients.EtcdConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

This will mean that if we ever want to add some configuration to the dao that is not etcd related we'd have to redo this. dao.Config was easier so we could configure all of the dao. I see how you arrived here since the only configuration items on the dao were etcd related, but it paints in a corner of only being etcd.

Log LogConfig
Openshift apb.ClusterConfig
ConfigFile string
Expand Down
16 changes: 16 additions & 0 deletions pkg/clients/clients.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package clients

import (
d "github.com/fsouza/go-dockerclient"

"github.com/coreos/etcd/client"
"k8s.io/client-go/rest"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
)

var Clients struct {
EtcdClient client.Client
KubernetesClient *clientset.Clientset
DockerClient *d.Client
RESTClient rest.Interface
}
17 changes: 17 additions & 0 deletions pkg/clients/docker.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package clients

import (
docker "github.com/fsouza/go-dockerclient"
logging "github.com/op/go-logging"
)

func NewDocker(log *logging.Logger) error {
dockerClient, err := docker.NewClient(DockerSocket)
if err != nil {
log.Error("Could not load docker client")
return err
}

Clients.DockerClient = dockerClient
return nil
}
41 changes: 41 additions & 0 deletions pkg/clients/etcd.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package clients

import (
"fmt"
"time"

logging "github.com/op/go-logging"

"github.com/coreos/etcd/client"
)

type EtcdConfig struct {
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 this should remain dao.Config, which is composed of EtcdConfig. Although it's only etcd currently, leaves dao configuration flexible for change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of clients.() methods, do we want any of these to be singletons?

singleton object I think that we just create them during initialization of application with the configuration of the clients. Allows us to error early if something is going to go wrong imo.

I think I would have preferred for the PRs to have come in as one unit. Made it difficult to review the set since it looks like C builds on B builds on A, and they all introduce new features on top of the others.

wish that at least on comments that told us the order, which could be added.

We should get rid of this struct

👍

I like the clients pkg a lot, decouples things in a way that makes changes a lot less brittle (client init code isn't just thrown about where it's used; it's got a home). Also going to make it real easy and natural to add an Origin() client.

👍

Semantically, it's the part of the apb package that's responsible for actually executing apb methods. Could almost be apb/apb.go? Curious what you guys think.

To me, it looks like a lot of the ABP code is in their own files. and client.go should all be in the new package. I am wondering what methods you were thinking should be left in apb?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eriknelson +1 to EtcdConfig remaining Config. I just added that as a comment to my review.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, it looks like a lot of the ABP code is in their own files. and client.go should all be in the new package. I am wondering what methods you were thinking should be left in apb?

I don't think code that's responsible for the execution of an apb belongs in the clients pkg, it's a natural fit for the apb pkg. In my mind, the clients package encapsulates the various gateways to other systems and APIs, along with their setup and validation.

RE: the logic that belongs in the apb pkg, it's effectively ExecuteApb(method, parameters), and we should hide the guts of how that actually gets done behind that function (in this case, it's running the pod). ExecuteApb IMO is a better name for RunImage, it's a higher level of abstraction. RunImage bleeds implementation details into the name. It suggests "run this apb method with these parameters, I don't care how you do that" vs "run this image because that's how we happen to run apbs".

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of clients.() methods, do we want any of these to be singletons?

This sounds ok, we need to make sure we test multiple users of the SAME client at the SAME time. Will that work ok? IS it really a 'single' connection? The old way we created a new connection so if there were multiple provisions going on each worker would have their own connection.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should get rid of this struct

I also concur with this 👍

EtcdHost string `yaml:"etcd_host"`
EtcdPort string `yaml:"etcd_port"`
}

func NewEtcd(config EtcdConfig, log *logging.Logger) error {
// TODO: Config validation
endpoints := []string{etcdEndpoint(config.EtcdHost, config.EtcdPort)}

log.Info("== ETCD CX ==")
log.Infof("EtcdHost: %s", config.EtcdHost)
log.Infof("EtcdPort: %s", config.EtcdPort)
log.Infof("Endpoints: %v", endpoints)

etcdClient, err := client.New(client.Config{
Endpoints: endpoints,
Transport: client.DefaultTransport,
HeaderTimeoutPerRequest: time.Second,
})
if err != nil {
return err
}

Clients.EtcdClient = etcdClient
return nil
}

func etcdEndpoint(host string, port string) string {
return fmt.Sprintf("http://%s:%s", host, port)
}
52 changes: 52 additions & 0 deletions pkg/clients/kubernetes.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package clients

import (
logging "github.com/op/go-logging"
restclient "k8s.io/client-go/rest"

"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/homedir"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
)

func createClientConfigFromFile(configPath string) (*restclient.Config, error) {
clientConfig, err := clientcmd.LoadFromFile(configPath)
if err != nil {
return nil, err
}

config, err := clientcmd.NewDefaultClientConfig(*clientConfig, &clientcmd.ConfigOverrides{}).ClientConfig()
if err != nil {
return nil, err
}
return config, nil
}

func NewKubernetes(log *logging.Logger) error {
// NOTE: Both the external and internal client object are using the same
// clientset library. Internal clientset normally uses a different
// library
clientConfig, err := restclient.InClusterConfig()
if err != nil {
log.Warning("Failed to create a InternalClientSet: %v.", err)

log.Debug("Checking for a local Cluster Config")
clientConfig, err = createClientConfigFromFile(homedir.HomeDir() + "/.kube/config")
if err != nil {
log.Error("Failed to create LocalClientSet")
return err
}
}

clientset, err := clientset.NewForConfig(clientConfig)
if err != nil {
log.Error("Failed to create LocalClientSet")
return err
}

rest := clientset.CoreV1().RESTClient()

Clients.RESTClient = rest
Clients.KubernetesClient = clientset
return nil
}
5 changes: 5 additions & 0 deletions pkg/clients/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package clients

const (
DockerSocket = "unix:///var/run/docker.sock"
)