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

[WIP] Provide initial server-side stacks API rough-in #38896

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@dperny
Copy link
Contributor

commented Mar 18, 2019

Writes the rough interfaces for the server-side stacks API, as a preview of what final implementation will look like. This commit is for code review purposes, and does not implement any functionality.

Basically, there are a lot of open questions about the stacks API at this moment. Before we can make progress on those issues, however, we need to nail down the parts of the API bit by bit in slow steps. This PR demonstrates the types, the router, and the client interface.

Related to #38872.

/cc @cpuguy83 and @vdemeester, who are on the engine side of this conversation, @dhiltgen and @alexmavr, who are on the stacks side of this conversation, and @thaJeztah, without whom no issue of this caliber would be complete.

// ServiceUpdateOptions
//
// This field follows the format of the X-Registry-Auth header.
EncodedRegistryAuth string

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 18, 2019

Member

Don't think this will work; if a stack contains multiple services, it's not unlikely some of those services use images from different registries (e.g. one from docker.io/foo/bar and one from gcr.io/bar/baz).

So either this should sent all credentials from the CLI (not very desirable), or perhaps we should think of #38591

Would these be stored as part of the service? (i.e., in case a task is re-deployed)

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Mar 22, 2019

Contributor

I'm not sure if #38591 will help much here.
Probably the service spec will need something like an ImagePullSecretRef field and have that reference a secret... this of course will also be helpful for other scenarios (e.g. a login token times out and a task is rescheduled).

This comment has been minimized.

Copy link
@dperny

dperny Mar 22, 2019

Author Contributor

How does the CLI currently decide which credentials to send? This problem is, I think, bigger than this particular feature, and is only coming to a head now that we have a situation in which multiple different registries may be used.

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Mar 22, 2019

Contributor

The CLI creates each service and does --with-registry-auth to set the auth for each individual service when creating it. It just fetches the registered secret from the backing store.

Creating on the daemon side is a bit more tricky... we could use the proposal as above and have the engine-side stack API ask the client for different things...

or we could include all the auths that are needed for each service in the request... this could be the path of lease resistance, and on equally horrible footing as the existing solution.

This comment has been minimized.

Copy link
@dperny

dperny Mar 22, 2019

Author Contributor

Also, this type is partly defined like this because it doesn't come from the body of a ServiceCreate request, but instead comes from the headers of the request.

This comment has been minimized.

Copy link
@dperny

dperny Mar 22, 2019

Author Contributor

One option may be to have something like this in the StackSpec:

type StackSpec struct {
    // ... elide the existing fields

    // RegistryAuths holds the information for registry auth tokens for various services, if needed.
    RegistryAuths RegsitryAuthSet `json:",omitempty"`
}

// RegistryAuthSet is a set of Registry credentials.
type RegistryAuthSet struct {
    // Credentials is a map of Service Name to Registry Auth Token needed for that service.
    Credentials map[string]string `json:",omitempty"`
}

Many other ways to implement it may be available. The information would, unfortunately, have to be sent as part of the request body, instead of a header.

}

// StackSpec defines the spec for a server-side stack.
type StackSpec struct {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Mar 18, 2019

Member

Wondering where Volumes are kept that are defined as part of the stack; also if the structure should be more flexible somehow 🤔

This comment has been minimized.

Copy link
@dperny

dperny Mar 22, 2019

Author Contributor

Forgot to define Volumes. But also, how do you mean, "more flexible"?

@vdemeester
Copy link
Member

left a comment

This looks indeed like what we discussed during the call. Not sure on the API path / experimental status of the API but looks better than before already 👼

// are created asynchronously
ID string

// TODO(dperny): should we include a "Warnings" field?

This comment has been minimized.

Copy link
@vdemeester

vdemeester Mar 19, 2019

Member

We may want too indeed 👼

This comment has been minimized.

Copy link
@alexmavr

alexmavr Mar 19, 2019

What sort of state should result in a create success with warnings as opposed to an error?

This comment has been minimized.

Copy link
@dperny

dperny Mar 22, 2019

Author Contributor

I can't imagine any, which is why I've opted not to include it. Not including it doesn't close the door on at some point including warnings in the future. Currently, as far as I can tell, the warning is only used to warn about digest pinning.

// ServiceUpdateOptions
//
// This field follows the format of the X-Registry-Auth header.
EncodedRegistryAuth string

This comment has been minimized.

Copy link
@vdemeester

vdemeester Mar 19, 2019

Member

Most likely same as above (cf @thaJeztah comment)

"github.com/docker/docker/api/types/swarm"
)

// TODO(dperny): in the final implementation, these methods would all exist in

This comment has been minimized.

Copy link
@alexmavr

alexmavr Mar 19, 2019

nit: unfinished sentence

Provide initial stacks API rough-in
Writes the rough interfaces for the server-side stacks API, as a preview
of what final implementation will look like. This commit is for code
review purposes, and does not implement any functionality.

Signed-off-by: Drew Erny <drew.erny@docker.com>

@dperny dperny force-pushed the dperny:stacks-api branch from 160ce38 to 732d839 Mar 22, 2019

@dperny

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

  • Added Volumes to StackSpec.
  • Finished incomplete comment noted by @alexmavr.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.