Better user experience with Kubernetes namespaces for UCP #1034

mat007 · 2018-05-02T13:54:39Z

- What I did

Made docker stack ls with the Kubernetes orchestrator list the namespaces a non-administrator user
has access to.

- How I did it

As this use case cannot be handled using the native Kubernetes API, I had to use an ad hoc workaround in the form of a call to the UCP API as a backup in the event of the call to the Kubernetes API failing.

Also as there is no simple means to retrieve the http client from the docker client, a new http client is being created…

- How to verify it

Create a Kubernetes user 'my-user' and grant it only access to the namespace of some installed services.

Without this PR docker stack ls fails when

> docker stack ls  --orchestrator=kubernetes --all-namespaces
stacks.compose.docker.com is forbidden: User "my-user" cannot list stacks.compose.docker.com at the cluster scope: access denied

whereas with the PR

> docker stack ls  --orchestrator=kubernetes --all-namespaces
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tutu

Also without ```--all-namespaces``` it falls back to the default ```--namespace=default``` and correctly fails
> docker stack ls  --orchestrator=kubernetes --all-namespaces
stacks.compose.docker.com is forbidden: User "my-user" cannot list stacks.compose.docker.com in the namespace "default": access denied

- Description for the changelog

None as this is merely a tweak for what was added in #1031

- A picture of a cute animal (not mandatory but encouraged)

vdemeester

The magic call to /kubernetesNamespaces is bothering me a bit, the fact that we hack the client out instead of extending/making the docker/docker/client composable, is bothering me way more..

cc @dnephin

vdemeester · 2018-05-03T09:33:37Z

cli/command/stack/list.go

+	"sort"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+


Fixed, thanks.

vdemeester · 2018-05-03T09:52:31Z

cli/command/stack/list.go

+	}
+	opts.AllNamespaces = false
+	for _, nm := range nms.Items {
+		stacks, err = getStacks(stacks, dockerCli, opts, kubernetes.NewOptions(flags, nm.Name))


Why not reusing getStacksWithNamespaces here ?(instead of the loop)

getStacksWithNamespaces iterates on the namespaces given on the command line with --namespace, here it iterates on the namespaces retrieved by getUserVisibleNamespaces, so a different set of namespaces.
We could factorize the loops but we would need to add code to convert one of the data structures to the other one, and in the end we wouldn't save much…

vdemeester · 2018-05-03T09:54:26Z

cli/command/stack/list.go

+				TLSClientConfig: tlsConfig,
+			}
+	}
+	endpoint.Scheme = "https"


Given that we set https here, we should just error out if tlsOptions are nil above.

Ah yes, good point, thanks!

vdemeester · 2018-05-03T10:06:07Z

cli/command/stack/list.go

+	return mnms, nil
+}
+
+func getUserVisibleNamespaces(dockerCli command.Cli, forbiddenErr error) (*core_v1.NamespaceList, error) {


This is really hacky… I feel that we should extract this somewhere in the kubernetes package and make and make the hack there (a func or list of func, …) … This would allow, in the future, to code different methods to get the list of namespace.

This also duplicates a bunch of what the cli/client does… We may want to Update https://github.com/moby/moby/blob/master/client/request.go to make it a bit more composable — like allowing to define new endpoint to query (with a function that would handle the http response).

vdemeester · 2018-05-03T10:07:09Z

cli/command/stack/list.go

+func listUserNamespaces(httpClient *http.Client, endpoint url.URL) (*core_v1.NamespaceList, error) {
+	resp, err := httpClient.Get(endpoint.String())
+	if err != nil {
+		return nil, fmt.Errorf("unable to list user namespaces: %s", err)


Here (and later on), prefer to use github.com/pkg/errors with Wrap or Wrapf :

return nil, errors.Wrap(err, "unable to list user namespaces")

Noted, thanks!

dnephin

generally LGTM, one comment

dnephin · 2018-05-04T01:52:29Z

cli/command/stack/list.go

 	flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
+	flags.StringSliceVar(&opts.Namespaces, "namespace", []string{"default"}, "Kubernetes namespaces to use")
+	flags.SetAnnotation("namespace", "kubernetes", nil)
+	flags.SetAnnotation("namespace", "experimentalCLI", nil)


Can't this use InstallFlags() (which I would suggest should be renamed to AddNamespaceFlag().

It can't because here it's

flags.StringSliceVar(&opts.Namespaces, "namespace", []string{"default"}, "Kubernetes namespaces to use")

whereas in InstallFlags soon to be AddNamespaceFlag it's

flags.String("namespace", "default", "Kubernetes namespace to use")

e.g. most docker stack sub-commands accept only one --namespace but docker stack ls can handle a list of them.

dnephin · 2018-05-04T01:53:18Z

cli/command/stack/kubernetes/cli.go

 }

+// InstallFlags adds common Kubernetes related flags
+func InstallFlags(flags *flag.FlagSet) {


Ok, 2 comments :)

s/InstallFlags/AddNamespaceFlag/

Done, thanks!

mat007 · 2018-05-04T13:30:30Z

@vdemeester I added a commit to move the code into the kubernetes package and started to refactor it for toward using moby client directly, for which I have a prototype in https://github.com/mat007/cli/commit/fee52e12c225a4c9a04d480ef75ec06675b169bf
Does this look like what you had in mind?

wsong · 2018-05-11T18:35:15Z

cli/command/stack/kubernetes/list.go

+		return nil, err
+	}
+	endpoint.Scheme = "https"
+	endpoint.Path = "/kubernetesNamespaces"


UCP does not have a versioned API at present. If we're going to rely on UCP endpoints, we need to be careful about making sure that this stays backwards-compatible.

Is this UX a requirement? Kubernetes does not have any other operations where you can view a list of only the resources that you have access to.

You're right, this is really fragile and ad hoc, however this is indeed a UX requirement and Kubernetes does not provide this feature…
If you have any suggestion on how to better manage this I'll be happy to update the code.

UCP does not have a versioned API at present. If we're going to rely on UCP endpoints, we need to be careful about making sure that this stays backwards-compatible.

@wsong yes, that's the job of UCP to be backwards-compatible on that endpoint (or providing a versionned one)…

All right, let's stick with this approach for now, but we'll have to be careful to test this with different combinations of CLI version + UCP version.

vdemeester · 2018-05-14T09:35:34Z

cli/command/container/create_test.go

 				return container.ContainerCreateCreatedBody{}, fmt.Errorf("shouldn't try to pull image")
 			},
-		}, test.EnableContentTrust)
+		}).EnableContentTrust()


Why this change ? I tend to prefer functionnal argument that "chains" of call… If it's just to not have test., we can dot import…

I believe @thaJeztah suggested it, I personally don't have any strong opinion on this.

By the way this commit actually belongs to the PR before this one e.g. #1031

thaJeztah · 2018-05-14T10:28:32Z

cli/command/stack/list.go

-	return ss, nil
-}
-
-func removeDuplicates(namespaces []string) []string {


I just noticed that this commit 691eeff is refactoring code that wasn't even merged yet: 1f9c976 (#1031). To reduce code-churn, could this refactor be done in the commit that adds it?

Yes, good point.

mat007 · 2018-05-15T13:30:16Z

@vdemeester I tweaked the custom request a bit and opened a PR upstream, can you please take another look?
I left out the functions as arguments as I don't see how to do that without adding a lot of code and/or types from the http package to interface.go…

wsong · 2018-05-15T18:42:30Z

cli/command/stack/kubernetes/list.go

+		return nil, err
+	}
+	endpoint.Scheme = "https"
+	endpoint.Path = "/kubernetesNamespaces"


All right, let's stick with this approach for now, but we'll have to be careful to test this with different combinations of CLI version + UCP version.

wsong

Whoops, didn't mean to approve.

wsong · 2018-05-15T18:44:54Z

cli/command/stack/kubernetes/list.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode)
+	}
+	if resp.StatusCode != http.StatusOK {


If you're not talking to a UCP cluster, this is going to return a 404, and you'll get back a very ugly error message that just says "page not found" or something. You should probably have some sort of fallback behavior here if /kubernetesNamespaces returns a 404.

getUserVisibleNamespaces returns an error indeed, the fallback is implemented by the caller in https://github.com/docker/cli/pull/1034/files#diff-5199fcbf9bc932c71a9e1984b1952b7aR62

Yes, but you just log a debug message there and return err.

It logs err2, the error returned by getUserVisibleNamespaces, and then returns err from the previous call, which is different and in practice due to the apierrs.IsForbidden(err) test right before is effectively as if getUserVisibleNamespaces had not been called at all:

$ docker --orchestrator=all stack ls stacks.compose.docker.com is forbidden: User "user" cannot list stacks.compose.docker.com at the cluster scope: access denied

I see. I think a comment here would be helpful.

OK, I added

// ignore this last error and return the previous one instead

So, thinking:

A 404 error is not an error, just means: we don't have UCP

We're now hiding potential other errors (500 - UCP crashed, 403 - UCP is up, but credentials are incorrect (?), ??? - Other error)

Instead of returning expected errors, and ignoring them in getStacksWithAllNamespaces(), we should leave the logic/decision if it's an actual error or not to getUserVisibleNamespaces(). When running into a 404, just return an empty list.

func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) { stacks, err := getStacks(kubeCli, opts) if err == nil || !apierrs.IsForbidden(err) { return stacks, err } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { return nil, errors.Wrap(err2, "Failed to query user visible namespaces") } if err != nil { return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { kubeCli.kubeNamespace = namespace.Name ss, err := getStacks(kubeCli, opts) if err != nil { return nil, err } stacks = append(stacks, ss...) } return stacks, nil } func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, error) { host := dockerCli.Client().DaemonHost() endpoint, err := url.Parse(host) if err != nil { return nil, err } endpoint.Scheme = "https" endpoint.Path = "/kubernetesNamespaces" resp, err := dockerCli.Client().HTTPClient().Get(endpoint.String()) if err != nil { return nil, err } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } namespaces := &core_v1.NamespaceList{} switch resp.StatusCode { case http.StatusOK: if err := json.Unmarshal(body, namespaces); err != nil { return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) } case http.StatusNotFound: // UCP API not present default: return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } return namespaces, nil }

diff --git a/cli/command/stack/kubernetes/list.go b/cli/command/stack/kubernetes/list.go index 8a7b1406..cb467f9b 100644 --- a/cli/command/stack/kubernetes/list.go +++ b/cli/command/stack/kubernetes/list.go @@ -11,7 +11,6 @@ import ( "github.com/docker/cli/cli/command/formatter" "github.com/docker/cli/cli/command/stack/options" "github.com/pkg/errors" - "github.com/sirupsen/logrus" core_v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -61,9 +60,10 @@ func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts op } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { - logrus.Debugf("Failed to query user visible namespaces: %s", err2) - // ignore this last error and return the previous one instead - return nil, err + return nil, errors.Wrap(err2, "Failed to query user visible namespaces") + } + if err != nil { + return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { @@ -94,12 +94,16 @@ func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, er if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf(string(body)) - } namespaces := &core_v1.NamespaceList{} - if err := json.Unmarshal(body, namespaces); err != nil { - return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) + switch resp.StatusCode { + case http.StatusOK: + if err := json.Unmarshal(body, namespaces); err != nil { + return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) + } + case http.StatusNotFound: + // UCP API not present + default: + return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } return namespaces, nil }

vdemeester · 2018-05-17T08:56:51Z

cli/command/stack/kubernetes/list.go

+	if err != nil {
+		return nil, err
+	}
+	endpoint.Scheme = "https"


This part is a bit sad as this is something we already do in the moby/moby client package (setting the scheme to https when tls is configured)..

thaJeztah · 2018-05-17T12:00:26Z

cli/command/stack/kubernetes/list.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode)
+	}
+	if resp.StatusCode != http.StatusOK {


So, thinking:

A 404 error is not an error, just means: we don't have UCP

We're now hiding potential other errors (500 - UCP crashed, 403 - UCP is up, but credentials are incorrect (?), ??? - Other error)

Instead of returning expected errors, and ignoring them in getStacksWithAllNamespaces(), we should leave the logic/decision if it's an actual error or not to getUserVisibleNamespaces(). When running into a 404, just return an empty list.

func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) { stacks, err := getStacks(kubeCli, opts) if err == nil || !apierrs.IsForbidden(err) { return stacks, err } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { return nil, errors.Wrap(err2, "Failed to query user visible namespaces") } if err != nil { return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { kubeCli.kubeNamespace = namespace.Name ss, err := getStacks(kubeCli, opts) if err != nil { return nil, err } stacks = append(stacks, ss...) } return stacks, nil } func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, error) { host := dockerCli.Client().DaemonHost() endpoint, err := url.Parse(host) if err != nil { return nil, err } endpoint.Scheme = "https" endpoint.Path = "/kubernetesNamespaces" resp, err := dockerCli.Client().HTTPClient().Get(endpoint.String()) if err != nil { return nil, err } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } namespaces := &core_v1.NamespaceList{} switch resp.StatusCode { case http.StatusOK: if err := json.Unmarshal(body, namespaces); err != nil { return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) } case http.StatusNotFound: // UCP API not present default: return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } return namespaces, nil }

diff --git a/cli/command/stack/kubernetes/list.go b/cli/command/stack/kubernetes/list.go index 8a7b1406..cb467f9b 100644 --- a/cli/command/stack/kubernetes/list.go +++ b/cli/command/stack/kubernetes/list.go @@ -11,7 +11,6 @@ import ( "github.com/docker/cli/cli/command/formatter" "github.com/docker/cli/cli/command/stack/options" "github.com/pkg/errors" - "github.com/sirupsen/logrus" core_v1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -61,9 +60,10 @@ func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts op } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { - logrus.Debugf("Failed to query user visible namespaces: %s", err2) - // ignore this last error and return the previous one instead - return nil, err + return nil, errors.Wrap(err2, "Failed to query user visible namespaces") + } + if err != nil { + return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { @@ -94,12 +94,16 @@ func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, er if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf(string(body)) - } namespaces := &core_v1.NamespaceList{} - if err := json.Unmarshal(body, namespaces); err != nil { - return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) + switch resp.StatusCode { + case http.StatusOK: + if err := json.Unmarshal(body, namespaces); err != nil { + return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) + } + case http.StatusNotFound: + // UCP API not present + default: + return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } return namespaces, nil }

thaJeztah · 2018-05-17T12:36:45Z

cli/command/stack/kubernetes/list.go

+	}
+	endpoint.Scheme = "https"
+	endpoint.Path = "/kubernetesNamespaces"
+	resp, err := dockerCli.Client().HTTPClient().Get(endpoint.String())


Wondering if there are other things that should be set on the request (content-type JSON, custom headers that are configured in the client-configuration, etc);

cli/vendor/github.com/docker/docker/client/request.go

Lines 225 to 241 in 60930d3

func (cli *Client) addHeaders(req *http.Request, headers headers) *http.Request {

// Add CLI Config's HTTP Headers BEFORE we set the Docker headers

// then the user can't change OUR headers

for k, v := range cli.customHTTPHeaders {

if versions.LessThan(cli.version, "1.25") && k == "User-Agent" {

continue

}

req.Header.Set(k, v)

}

if headers != nil {

for k, v := range headers {

req.Header[k] = v

}

}

return req

}

I don't know, however this would not be an issue were the Client performing the request itself.
moby/moby#37071 has been merged in moby, but maybe it's not to late to bring that up?

Well, it's a bit of an odd situation; the /kubernetesNamespaces endpoint feels like an add-hoc/temporary solution. Some thoughts on that;

It's undocumented, and not sure how well-defined the endpoint is

the Engine remote API may perform validations, e.g.

correct request-type, respond different based on "accept" headers (plain-text/JSON)

version negotiation (not present in the UCP API) (this could be a problem if we used the docker/client/request functionality, as it may be done automatically - need to check)

So, should we export the docker/client/request client.get(), client.head() and so on functions? I don't know.

On the "plus" side; when using the docker/client/request functionality, we would get some things for free (improved/consistent errors if TLS fails, automatic handling of invalid JSON responses, etc.)

I think in an ideal situation, given that UCP defines its own API (which is the Engine remote-API + Additional endpoints/extensions) there would be an official UCP client that we could use.

I do agree with @thaJeztah that it is really an odd situation : we end up hacking the docker/docker client around for something that isn't even docker api related.

As discussed a bit offline earlier, a cleaner way (way cleaner) would have been to have a kubernetes api extension (same as the current "compose on kubernetes" one) :

we wouldn't need to hack docker/docker client around, we would just consume the go code from the extension (again, same way as "compose on kubernetes")

it would be "automatically" versionned (because of k8s way of doing things)

it wouldn't be a direct dependency on UCP as it could be easily deployed on other cluster in the future if we wanted

Being new to the org I cannot estimate whether this is a realistic solution given the time constraint, @chris-crone any opinion?

@thaJeztah there's an issue against the relevant repo to ensure that this endpoint is properly tested and that its behaviour isn't modified.

@vdemeester I agree that those solutions would be cleaner but given the timeline for this; they're not feasible. Since this is a fallback we can replace it or demote it in the fallback list at a later date once we've had time to investigate the cleaner options.

mat007 · 2018-05-17T16:22:08Z

I updated moby/moby and the needed dependencies and code, and also integrated the last suggestion from @thaJeztah regarding how to handle a 404 when requesting the UCP endpoint (with a slight variation, feel free to question me on the code if it's not obvious why).

mat007 · 2018-05-17T16:34:24Z

Obviously ci/circleci: validate fails, is there something else I'm supposed to do when updating a dependency?

wsong · 2018-05-17T16:35:05Z

cli/command/stack/kubernetes/list.go

+	return stacks, nil
+}
+
+func getUserVisibleNamespaces(dockerCli command.Cli, previousErr error) (*core_v1.NamespaceList, error) {


@thaJeztah @mat007 I think it's a little weird to pass in previousErr to this function so that we can return it in certain cases.

@thaJeztah, what did you have in mind here?

I suppose I could return also a bool meaning «404, just ignore the call»? Or create a custom error to be returned just for the sake a filtering right after the call?
The previousErr remains the solution with the less extra code, which is why I went for it.

I think your previous solution (return an error and then have getStacksWithAllNamespaces check for that err and return the previous error) was actually better, which is why I'm wondering if @thaJeztah had something different in mind.

My suggestion (here #1034 (comment)) was because the previous solution was discarding all errors returned by UCP, which was definitely not the right thing to do.

I see I made one mistake in that example, which probably lead to @mat007 rewriting it (updated version below);

updated example:

func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) { stacks, err := getStacks(kubeCli, opts) if err == nil || !apierrs.IsForbidden(err) { return stacks, err } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { return nil, errors.Wrap(err2, "Failed to query user visible namespaces") } if nms == nil { return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { kubeCli.kubeNamespace = namespace.Name ss, err := getStacks(kubeCli, opts) if err != nil { return nil, err } stacks = append(stacks, ss...) } return stacks, nil } func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, error) { host := dockerCli.Client().DaemonHost() endpoint, err := url.Parse(host) if err != nil { return nil, err } endpoint.Scheme = "https" endpoint.Path = "/kubernetesNamespaces" resp, err := dockerCli.Client().HTTPClient().Get(endpoint.String()) if err != nil { return nil, err } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } switch resp.StatusCode { case http.StatusOK: namespaces := &core_v1.NamespaceList{} if err := json.Unmarshal(body, namespaces); err != nil { return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) } return namespaces, nil case http.StatusNotFound: // UCP API not present return nil, nil default: return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } }

An alternative could be to flip the order (dummy code);
if stacks, err := getUCPStacks(dockerCli, opts); err != nil || stacks != nil { return stacks, err } return getStacks(kubeCli, opts)

In the above;

In case of a 404, getUCPStacks() returns no error, and nil for stacks, in that case, fall back to plain kubernetes

Any other error is an issue with the UCP endpoint, so should be handled. In that case, don't fallback to plain kubernetes

Advantage: There's no back and forth between trying UCP and k8s; currently, we try k8s, get an error, try UCP, discover it's not there, and head back to the original k8s error

Disadvantage: It's odd for Docker CE clients to try an API that won't be there for 99% of the users

I believe we cannot swap the function calls as what we want is first Kubernetes, then UCP only if Kubernetes fails for lack of user rights.

@wsong we have 3 possible results when querying UCP : not there (404), there but error'ed and valid result, so we need to distinguish between the three
@thaJeztah ah yes, I suppose we could test for nil to distinguish a 404 from []string{} an empty result, OK I'll do that instead

Thank you both!

thaJeztah · 2018-05-17T21:27:10Z

cli/command/stack/kubernetes/list.go

 }
+func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) {
+	stacks, err := getStacks(kubeCli, opts)
+	if err == nil || !apierrs.IsForbidden(err) {


I think this could just be;

if !apierrs.IsForbidden(err) { return stacks, err }

(no need to change)

You're right, I'll change it while I'm at it.

thaJeztah · 2018-05-17T22:26:38Z

cli/command/stack/kubernetes/list.go

+		return nil, errors.Wrap(err, "failed to query user visible namespaces")
+	}
+	opts.AllNamespaces = false
+	for _, namespace := range nms.Items {


May not be worth changing in this PR, but just realised if we change getUserVisibleNamespaces() to return a []string, we could use getStacksWithNamespaces() here

thaJeztah · 2018-05-17T22:45:49Z

cli/command/stack/kubernetes/list.go

 	}
 	return formattedStacks, nil
 }
+func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) {


Heh, also just occurred to me: KubeCli is also a command.Cli (see WrapCLI()); so you'd only need to pass in kubeCli

thaJeztah · 2018-05-17T23:42:29Z

cli/command/stack/kubernetes/list.go

+	return stacks, nil
+}
+
+func getUserVisibleNamespaces(dockerCli command.Cli, previousErr error) (*core_v1.NamespaceList, error) {


My suggestion (here #1034 (comment)) was because the previous solution was discarding all errors returned by UCP, which was definitely not the right thing to do.

I see I made one mistake in that example, which probably lead to @mat007 rewriting it (updated version below);

updated example:

func getStacksWithAllNamespaces(dockerCli command.Cli, kubeCli *KubeCli, opts options.List) ([]*formatter.Stack, error) { stacks, err := getStacks(kubeCli, opts) if err == nil || !apierrs.IsForbidden(err) { return stacks, err } nms, err2 := getUserVisibleNamespaces(dockerCli) if err2 != nil { return nil, errors.Wrap(err2, "Failed to query user visible namespaces") } if nms == nil { return nil, errors.Wrap(err, "Failed to retrieve stacks") } opts.AllNamespaces = false for _, namespace := range nms.Items { kubeCli.kubeNamespace = namespace.Name ss, err := getStacks(kubeCli, opts) if err != nil { return nil, err } stacks = append(stacks, ss...) } return stacks, nil } func getUserVisibleNamespaces(dockerCli command.Cli) (*core_v1.NamespaceList, error) { host := dockerCli.Client().DaemonHost() endpoint, err := url.Parse(host) if err != nil { return nil, err } endpoint.Scheme = "https" endpoint.Path = "/kubernetesNamespaces" resp, err := dockerCli.Client().HTTPClient().Get(endpoint.String()) if err != nil { return nil, err } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, errors.Wrapf(err, "received %d status and unable to read response", resp.StatusCode) } switch resp.StatusCode { case http.StatusOK: namespaces := &core_v1.NamespaceList{} if err := json.Unmarshal(body, namespaces); err != nil { return nil, errors.Wrapf(err, "unmarshal failed: %s", string(body)) } return namespaces, nil case http.StatusNotFound: // UCP API not present return nil, nil default: return nil, fmt.Errorf("received %d status while retrieving namespaces: %s", resp.StatusCode, string(body)) } }

An alternative could be to flip the order (dummy code);
if stacks, err := getUCPStacks(dockerCli, opts); err != nil || stacks != nil { return stacks, err } return getStacks(kubeCli, opts)

In the above;

In case of a 404, getUCPStacks() returns no error, and nil for stacks, in that case, fall back to plain kubernetes

Any other error is an issue with the UCP endpoint, so should be handled. In that case, don't fallback to plain kubernetes

Advantage: There's no back and forth between trying UCP and k8s; currently, we try k8s, get an error, try UCP, discover it's not there, and head back to the original k8s error

Disadvantage: It's odd for Docker CE clients to try an API that won't be there for 99% of the users

mat007 · 2018-05-18T08:22:02Z

@thaJeztah PR updated!

thaJeztah

changes LGTM

(but we should definitely look into the UCP implementation for a future update)

I saw that the vendor-check failed, so may have to check that

Also bringing: . golang.org/x/net 5561cd9b4330353950f399814f427425c0a26fd2 . github.com/docker/distribution 83389a148052d74ac602f5f1d62f86ff2f3c4aa5 . github.com/docker/swarmkit bd69f6e8e301645afd344913fa1ede53a0a111fb . github.com/docker/go-metrics d466d4f6fd960e01820085bd7e1a24426ee7ef18 . github.com/prometheus/client_golang 52437c81da6b127a9925d17eb3a382a2e5fd395e . github.com/beorn7/perks 4c0e84591b9aa9e6dcfdf3e020114cd81f89d5f9 . github.com/prometheus/client_model fa8ad6fec33561be4280a8f0514318c79d7f6cb6 . github.com/prometheus/common ebdfc6da46522d58825777cf1f90490a5b1ef1d8 . github.com/prometheus/procfs abf152e5f3e97f2fafac028d2cc06c1feb87ffa5 . github.com/matttproud/golang_protobuf_extensions v1.0.0 Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

vdemeester

SGTM 🍂

thaJeztah · 2018-05-18T12:40:31Z

Merged!

@chris-crone can you handle opening tickets for the follow-up (better implementation of the all namespaces endpoint)?

mat007 requested a review from vdemeester as a code owner May 2, 2018 13:54

GordonTheTurtle added the status/0-triage label May 2, 2018

mat007 closed this May 2, 2018

mat007 reopened this May 2, 2018

GordonTheTurtle added dco/no and removed dco/no labels May 3, 2018

docker deleted a comment from GordonTheTurtle May 3, 2018

vdemeester added status/2-code-review and removed status/0-triage labels May 3, 2018

vdemeester requested changes May 3, 2018

View reviewed changes

dnephin reviewed May 4, 2018

View reviewed changes

mat007 changed the title ~~Better user experience with namespaces in docker stack ls with Kubernetes~~ Better user experience with Kubernetes namespaces in docker stack ls May 4, 2018

mat007 changed the title ~~Better user experience with Kubernetes namespaces in docker stack ls~~ [WIP] Better user experience with Kubernetes namespaces in docker stack ls May 9, 2018

mat007 requested a review from thaJeztah as a code owner May 9, 2018 12:10

thaJeztah added area/stack area/kubernetes labels May 9, 2018

wsong reviewed May 11, 2018

View reviewed changes

vdemeester requested changes May 14, 2018

View reviewed changes

thaJeztah reviewed May 14, 2018

View reviewed changes

mat007 changed the title ~~[WIP] Better user experience with Kubernetes namespaces in docker stack ls~~ [WIP] Better user experience with Kubernetes namespaces for UCP May 15, 2018

mat007 mentioned this pull request May 15, 2018

Extend client API with custom HTTP requests moby/moby#37071

Merged

thaJeztah added the status/needs-vendoring label May 15, 2018

thaJeztah changed the title ~~[WIP] Better user experience with Kubernetes namespaces for UCP~~ Better user experience with Kubernetes namespaces for UCP May 15, 2018

wsong approved these changes May 15, 2018

View reviewed changes

wsong suggested changes May 15, 2018

View reviewed changes

wsong approved these changes May 16, 2018

View reviewed changes

vdemeester reviewed May 17, 2018

View reviewed changes

thaJeztah requested changes May 17, 2018

View reviewed changes

thaJeztah reviewed May 17, 2018

View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels May 17, 2018

wsong reviewed May 17, 2018

View reviewed changes

thaJeztah requested changes May 17, 2018

View reviewed changes

thaJeztah approved these changes May 18, 2018

View reviewed changes

mat007 added 2 commits May 18, 2018 11:44

Query UCP Kubernetes user namespaces endpoint

bd20885

Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

docker deleted a comment from GordonTheTurtle May 18, 2018

vdemeester approved these changes May 18, 2018

View reviewed changes

thaJeztah removed the status/needs-vendoring label May 18, 2018

thaJeztah merged commit 7de6aae into docker:master May 18, 2018

GordonTheTurtle added this to the 18.06.0 milestone May 18, 2018

chris-crone mentioned this pull request May 18, 2018

Find cleaner solution for fetching all Kubernetes namespaces #1067

Closed

thaJeztah mentioned this pull request Sep 19, 2025

client: remove unused Client.HTTPClient() method moby/moby#51011

Merged

	func (cli Client) addHeaders(req http.Request, headers headers) *http.Request {
	// Add CLI Config's HTTP Headers BEFORE we set the Docker headers
	// then the user can't change OUR headers
	for k, v := range cli.customHTTPHeaders {
	if versions.LessThan(cli.version, "1.25") && k == "User-Agent" {
	continue
	}
	req.Header.Set(k, v)
	}

	if headers != nil {
	for k, v := range headers {
	req.Header[k] = v
	}
	}
	return req
	}

Better user experience with Kubernetes namespaces for UCP #1034

Better user experience with Kubernetes namespaces for UCP #1034

Uh oh!

Conversation

mat007 commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mat007 May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mat007 commented May 2, 2018 •

edited

Loading

vdemeester left a comment •

edited

Loading

mat007 May 16, 2018 •

edited

Loading

mat007 May 17, 2018 •

edited

Loading