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

CLI: Add support for using Configs as CredentialSpecs in services #1656

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 2, 2019

depends on moby/moby#38672 moby/moby#38632 (using a temporary vendoring), and built on top of #1655 (which isn't merged yet)

  • Improve validation of credential_spec in compose schema (I'll split that to a separate PR as well)
  • Add compose schema 3.8
  • Add config as option in credential_spec

@thaJeztah
Copy link
Member Author

ping @dperny @vdemeester @tiborvass PTAL

@thaJeztah
Copy link
Member Author

split the compose-file validation for older versions to #1657

@codecov-io
Copy link

codecov-io commented Feb 2, 2019

Codecov Report

Merging #1656 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
+ Coverage   56.26%   56.34%   +0.07%     
==========================================
  Files         308      308              
  Lines       21293    21306      +13     
==========================================
+ Hits        11980    12004      +24     
+ Misses       8439     8428      -11     
  Partials      874      874

@simonferquel
Copy link
Contributor

Should'nt credentials be stored as Secret objects more than Configs ?

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 4, 2019 via email

Copy link
Contributor

@simonferquel simonferquel left a comment

Choose a reason for hiding this comment

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

Aside from the temporary vendoring, seems good to me then (might require some squashing as well)

@thaJeztah
Copy link
Member Author

Thx! only the last two commits are new in this PR, other commits will disappear once the related PR's are merged

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, just requesting changes to prevent merging it until the moby PR is merged.

@vdemeester
Copy link
Collaborator

@thaJeztah needs a PR and some updates ?

@thaJeztah
Copy link
Member Author

Rebased on top of #1700, but there's still discussion about the current implementation in moby, so let's hold off merging this until that's in moby/moby#38777 (comment)

/cc @ddebroy @dperny @wk8

@thaJeztah

This comment has been minimized.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@dperny
Copy link
Contributor

dperny commented Mar 27, 2019

Final implementation for CredentialSpecs through swarm configs is done, so this can move forward.

The implementation of this feature requires two things: the Config needs to be in the ConfigReferences for the service, and the Config used for the CredentialSpec needs to be specified by ID, not by name.

Because ConfigReferences already must include the ConfigID, the logic we use to fill in that field should be used in turn to fill in the CredentialSpec field in the Service object appropriately. My vision of the CLI is to have this flag:

--credential-spec config://my-config-name

turn into these objects:

// assume the existence of some function `resolveConfigID(string) string`
// which takes a config name and returns its ID
credentialSpec := &swarm.CredentialSpec{
    Config: resolveConfigID(strings.TrimPrefix(value, "config://")),
}
func addCredentialSpecConfig(spec *swarm.ServiceSpec, c *swarm.Config) {
    spec.TaskTemplate.ContainerSpec.Configs = append(spec.TaskTemplate.ContainerSpec.Configs,
        &swarm.ConfigReference{
            Runtime: &swarm.ConfigReferenceRuntimeTarget{},
            ConfigID: c.ID,
            ConfigName: c.Name,
        },
    )
}

@dperny
Copy link
Contributor

dperny commented Mar 27, 2019

I'm going to pull down @thaJeztah's branch and make these changes myself.

@thaJeztah thaJeztah deleted the cli_credentialspec_config branch April 12, 2019 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants