cmd: get ep with labels, pod / container name, ID #1139

ianvernon · 2017-07-18T22:24:29Z

cmd: get ep with labels, pod / container name, ID

daemon: add handlers for new /endpoints API, update endpoint structures with container ID / pod name
pkg/endpoint: support new API models
pkg/endpointmanager: add pod name, container name entries as keys mapping to endpoints
pkg/labels: change Orchestration labels -> OrchestrationIdentity labels (labels used in computing a security identity) and OrchrestationInfo labels (labels not used in computing a security identity)

cilium endpoint get now supports the following:

cilium endpoint get -l <set of labels>
cilium endpoint get <eID, pod name, container name, etc.>

api: add pod / container name, labels for GET ep

add new fields to Endpoint (pod-name, container-name)
GET /endpoint now takes an optional array of labels to return a list of endpoints whose labels match the provided array.

Signed-off by: Ian Vernon ian@covalent.io

houndci-bot · 2017-07-18T22:24:43Z

pkg/option/option.go

+	return mutable
+}
+
+func (bo *BoolOptions) GetImmutableModel() models.ConfigurationMap {


exported method BoolOptions.GetImmutableModel should have comment or be unexported

houndci-bot · 2017-07-18T22:24:43Z

pkg/option/option.go

@@ -121,6 +121,25 @@ type BoolOptions struct {
 	Library *OptionLibrary `json:"-"`
 }

+func (bo *BoolOptions) GetMutableModel() models.ConfigurationMap {


exported method BoolOptions.GetMutableModel should have comment or be unexported

houndci-bot · 2017-07-18T22:24:43Z

pkg/endpointmanager/manager.go

@@ -180,6 +218,16 @@ func LookupLocked(id string) (*endpoint.Endpoint, error) {
 	case endpoint.DockerEndpointPrefix:
 		return lookupDockerEndpointLocked(eid), nil

+	case endpoint.ContainerNamePrefix:
+		log.Warningf("looking up container name: %s", eid)
+		for k, _ := range endpointsAux {


should omit 2nd value from range; this loop is equivalent to for k := range ...

houndci-bot · 2017-07-18T22:24:43Z

pkg/endpointmanager/manager.go

 	if ep, ok := endpointsAux[endpoint.NewID(endpoint.ContainerIdPrefix, id)]; ok {
 		return ep
 	}
+
+
+	for k, _ := range endpointsAux {


should omit 2nd value from range; this loop is equivalent to for k := range ...

houndci-bot · 2017-07-18T22:24:43Z

daemon/endpoint.go

+	d *Daemon
+}
+
+func NewGetEndpointsHandler(d *Daemon) GetEndpointsHandler {


exported function NewGetEndpointsHandler should have comment or be unexported

houndci-bot · 2017-07-18T22:24:43Z

daemon/endpoint.go

 )

+//"github.com/cilium/cilium/pkg/nodeaddress"


comment on exported method Daemon.SetEndpointIdentity should be of the form "SetEndpointIdentity ..."

houndci-bot · 2017-07-18T22:38:51Z

pkg/endpointmanager/manager.go

 	if ep, ok := endpointsAux[endpoint.NewID(endpoint.ContainerIdPrefix, id)]; ok {
 		return ep
 	}
+
+	for k, _ := range endpointsAux {


should omit 2nd value from range; this loop is equivalent to for k := range ...

houndci-bot · 2017-07-18T22:55:11Z

daemon/endpoint.go

 )

+//"github.com/cilium/cilium/pkg/nodeaddress"


comment on exported method Daemon.SetEndpointIdentity should be of the form "SetEndpointIdentity ..."

houndci-bot · 2017-07-18T22:55:12Z

daemon/endpoint.go

 )

+//"github.com/cilium/cilium/pkg/nodeaddress"


comment on exported method Daemon.SetEndpointIdentity should be of the form "SetEndpointIdentity ..."

houndci-bot · 2017-07-18T22:55:12Z

daemon/endpoint.go

 )

+//"github.com/cilium/cilium/pkg/nodeaddress"


comment on exported method Daemon.SetEndpointIdentity should be of the form "SetEndpointIdentity ..."

houndci-bot · 2017-07-18T23:12:09Z

pkg/option/option.go

+	return mutable
+}
+
+// GetMutableModel returns immutable configuration options as a ConfigurationMap model.


comment on exported method BoolOptions.GetImmutableModel should be of the form "GetImmutableModel ..."

houndci-bot · 2017-07-18T23:12:09Z

daemon/endpoint.go

+	d *Daemon
+}
+
+// NewGetEndpoints handler returns a new getEndpoints populated with provided Daemon.


comment on exported function NewGetEndpointsHandler should be of the form "NewGetEndpointsHandler ..."

houndci-bot · 2017-07-18T23:12:09Z

pkg/option/option.go

+	return mutable
+}
+
+// GetMutableModel returns immutable configuration options as a ConfigurationMap model.


comment on exported method BoolOptions.GetImmutableModel should be of the form "GetImmutableModel ..."

houndci-bot · 2017-07-18T23:12:09Z

daemon/endpoint.go

+	d *Daemon
+}
+
+// NewGetEndpoints handler returns a new getEndpoints populated with provided Daemon.


comment on exported function NewGetEndpointsHandler should be of the form "NewGetEndpointsHandler ..."

houndci-bot · 2017-07-18T23:12:09Z

pkg/option/option.go

+	return mutable
+}
+
+// GetMutableModel returns immutable configuration options as a ConfigurationMap model.


comment on exported method BoolOptions.GetImmutableModel should be of the form "GetImmutableModel ..."

houndci-bot · 2017-07-18T23:12:09Z

daemon/endpoint.go

+	d *Daemon
+}
+
+// NewGetEndpoints handler returns a new getEndpoints populated with provided Daemon.


comment on exported function NewGetEndpointsHandler should be of the form "NewGetEndpointsHandler ..."

houndci-bot · 2017-07-18T23:12:09Z

pkg/option/option.go

+	return mutable
+}
+
+// GetMutableModel returns immutable configuration options as a ConfigurationMap model.


comment on exported method BoolOptions.GetImmutableModel should be of the form "GetImmutableModel ..."

houndci-bot · 2017-07-18T23:12:09Z

daemon/endpoint.go

+	d *Daemon
+}
+
+// NewGetEndpoints handler returns a new getEndpoints populated with provided Daemon.


comment on exported function NewGetEndpointsHandler should be of the form "NewGetEndpointsHandler ..."

houndci-bot · 2017-07-18T23:12:10Z

pkg/option/option.go

+	return mutable
+}
+
+// GetMutableModel returns immutable configuration options as a ConfigurationMap model.


comment on exported method BoolOptions.GetImmutableModel should be of the form "GetImmutableModel ..."

houndci-bot · 2017-07-18T23:12:10Z

daemon/endpoint.go

+	d *Daemon
+}
+
+// NewGetEndpoints handler returns a new getEndpoints populated with provided Daemon.


comment on exported function NewGetEndpointsHandler should be of the form "NewGetEndpointsHandler ..."

tgraf

This will be awesome!!! 👍

tgraf · 2017-07-19T02:44:44Z

daemon/docker_watcher.go

@@ -249,6 +255,21 @@ func (d *Daemon) handleCreateContainer(id string, retry bool) {
 			}
 		}

+		if ep != nil {


Just move this to below the ep ==nil condition so you don't have to check for ep != nil

tgraf · 2017-07-19T02:47:42Z

daemon/docker_watcher.go

+			if d.conf.IsK8sEnabled() {
+				if dockerContainer.Config != nil {
+					podNamespace := k8sDockerLbls.GetPodNamespace(dockerContainer.Config.Labels)
+					podName := k8sDockerLbls.GetPodName(dockerContainer.Config.Labels)


Does this trigger another API call to the API server? If so, can we restructure the code to retrieve this when we already fetch them via getFilteredLabels()?

These calls to GetPodNamespace and GetPodName are lookups into the map dockerContainer.Config.Labels for io.kubernetes.pod.name and io.kubernetes.pod.namespace - so they are not calls to the API server.

tgraf · 2017-07-19T02:49:17Z

daemon/endpoint.go

+		args.Add("label", v)
+	}
+
+	containers, err := h.d.dockerClient.ContainerList(ctx.Background(), types.ContainerListOptions{


This is not a good idea. This will potentially block this API call for a long time. We should fetch and store the docker labels with the endpoint when we receive the create event and then just look for local endpoints which match the labels.

tgraf · 2017-07-19T02:50:16Z

pkg/endpoint/endpoint.go

-	State            string
+	ID                  uint16       // Endpoint ID.
+	Mutex               sync.RWMutex // Protects all variables from this structure below this line
+	DockerContainerName string       // Docker container name.


Can we call this ContainerName? We will rename the DockerID to ContainerID as well later.

tgraf · 2017-07-19T02:51:12Z

pkg/endpoint/id.go

@@ -47,6 +49,9 @@ func NewID(prefix PrefixType, id string) string {
 func SplitID(id string) (PrefixType, string) {
 	if s := strings.Split(id, ":"); len(s) == 2 {
 		return PrefixType(s[0]), s[1]
+	} else if len(s) == 3 {
+		// PodNamePrefix case.


Can you add an example here on how the id looks like to make it clear?

aanm · 2017-07-19T14:17:24Z

api/v1/openapi.yaml

@@ -224,6 +224,27 @@ paths:
          x-go-name: Failed
          schema:
            "$ref": "#/definitions/Error"
+  "/endpoints/":


Why not extending the /endpoint/ on line 170?

/endpoint/ returns one endpoint, while /endpoints/ returns an array of endpoints. I'm open to moving this under /endpoint/, though and changing /endpoint/ to return a list of endpoints as well.

"/endpoint": get: summary: Get list of all endpoints description: | Returns an array of all local endpoints.

You're right :) I've made this change as part of my most recent commit.

aanm · 2017-07-19T14:19:30Z

cilium/cmd/endpoint_get.go

+		if len(lbls) > 0 {
+			params := endpointApi.NewGetEndpointsParams().WithLabels(lbls)
+			if e, err := client.Endpoint.GetEndpoints(params); err != nil {
+				Fatalf("Cannot get endpoint for given list of labels %s: %s\n", lbls, err)


"endpoint", as a single endpoint? We can have multiple endpoints using the same list of labels.

aanm · 2017-07-19T14:23:53Z

daemon/endpoint.go

 	"github.com/go-openapi/runtime/middleware"
+	ctx "golang.org/x/net/context"


can't you replace with ctx "context"?

aanm · 2017-07-19T14:25:20Z

daemon/endpoint.go

+	}
+
+	containers, err := h.d.dockerClient.ContainerList(ctx.Background(), types.ContainerListOptions{
+		Size:   true,


Do you need the Size of the containers?

I have removed interacting with the Docker client in my most recent upload.

aanm · 2017-07-19T14:25:42Z

daemon/endpoint.go

+
+	containers, err := h.d.dockerClient.ContainerList(ctx.Background(), types.ContainerListOptions{
+		Size:   true,
+		All:    true,


All includes dead containers, not sure if we need it as well.

I have removed interacting with the Docker client in my most recent upload.

aanm · 2017-07-19T14:30:23Z

cilium/cmd/config.go

@@ -75,6 +75,8 @@ func configDaemon(cmd *cobra.Command, opts []string) {



Why part of the cilium config command? I think it should make more sense as part of the cilium status on the K8s api server

I had added this change initially as part of this commit, but ended up not needing it. Figured it might be useful if we added more visibility into what the configuration of Cilium is as part of the API, but I'm open to removing it as part of this patch. What do you think?

Removed this in most recent upload.

houndci-bot · 2017-07-19T22:13:15Z

pkg/endpoint/endpoint.go

+		if !found {
+			log.Warningf("HasLabels: label %v not found, returning false", v)
+			return false
+		} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2017-07-20T00:45:10Z

daemon/endpoint.go

+		endpointmanager.Mutex.RUnlock()
+		wg.Wait()
+		return NewGetEndpointOK().WithPayload(eps)
+	} else {


if block ends with a return statement, so drop this else and outdent its block

* add new fields to Endpoint (pod-name, container-name) * GET /endpoint now takes an optional array of labels to return a list of endpoints whose labels match the provided array. Signed-off by: Ian Vernon <ian@covalent.io>

* daemon: add handlers for new /endpoints API, update endpoint structures with container ID / pod name * pkg/endpoint: support new API models * pkg/endpointmanager: add pod name, container name entries as keys mapping to endpoints * pkg/labels: change Orchestration labels -> OrchestrationIdentity labels (labels used in computing a security identity) and OrchrestationInfo labels (labels not used in computing a security identity) `cilium endpoint get` now supports the following: * `cilium endpoint get -l <set of labels>` * `cilium endpoint get <eID, pod name, container name, etc.>` Signed-off by: Ian Vernon <ian@covalent.io>

houndci-bot reviewed Jul 18, 2017

View reviewed changes

ianvernon force-pushed the 675-endpoint-get-enhancements branch 2 times, most recently from f8293f8 to c54bfbf Compare July 18, 2017 23:38

ianvernon changed the title ~~WIP - 675 endpoint get enhancements~~ cmd: get ep with labels, pod / container name, ID Jul 18, 2017

ianvernon assigned tgraf and aanm Jul 18, 2017

ianvernon added the pending-review label Jul 18, 2017

tgraf requested changes Jul 19, 2017

View reviewed changes

aanm reviewed Jul 19, 2017

View reviewed changes

houndci-bot reviewed Jul 19, 2017

View reviewed changes

ianvernon requested review from tgraf and aanm July 19, 2017 23:52

houndci-bot reviewed Jul 20, 2017

View reviewed changes

ianvernon force-pushed the 675-endpoint-get-enhancements branch from bd5370f to fc41fa4 Compare July 20, 2017 00:59

api: add pod / container name, labels for GET ep

7f563f4

* add new fields to Endpoint (pod-name, container-name) * GET /endpoint now takes an optional array of labels to return a list of endpoints whose labels match the provided array. Signed-off by: Ian Vernon <ian@covalent.io>

ianvernon force-pushed the 675-endpoint-get-enhancements branch 3 times, most recently from 6dcea37 to 8b59d43 Compare July 20, 2017 02:50

ianvernon force-pushed the 675-endpoint-get-enhancements branch from 8b59d43 to df64441 Compare July 20, 2017 02:59

tgraf approved these changes Jul 20, 2017

View reviewed changes

tgraf merged commit d9a265e into master Jul 20, 2017

tgraf deleted the 675-endpoint-get-enhancements branch July 20, 2017 03:53

		"github.com/go-openapi/runtime/middleware"
		ctx "golang.org/x/net/context"

		@@ -75,6 +75,8 @@ func configDaemon(cmd *cobra.Command, opts []string) {

		)

		//"github.com/cilium/cilium/pkg/nodeaddress"

		)

		//"github.com/cilium/cilium/pkg/nodeaddress"

		)

		//"github.com/cilium/cilium/pkg/nodeaddress"

		)

		//"github.com/cilium/cilium/pkg/nodeaddress"

cmd: get ep with labels, pod / container name, ID #1139

cmd: get ep with labels, pod / container name, ID #1139

Conversation

ianvernon commented Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon commented Jul 18, 2017 •

edited

Loading