Skip to content
This repository has been archived by the owner on Mar 20, 2019. It is now read-only.

Support secrets #13

Merged
merged 1 commit into from Jan 26, 2016
Merged

Support secrets #13

merged 1 commit into from Jan 26, 2016

Conversation

felixb
Copy link
Contributor

@felixb felixb commented Jan 21, 2016

This allows to set up framework authentication.

@felixb
Copy link
Contributor Author

felixb commented Jan 22, 2016

I just rebased this PR on top of your develop branch. I guess it's easier to merge to review.
I worked in all your comments (w/o the require => File[..]. puppet is doing it's magic to auto-require)

$log_level = 'info',
$ulimit = undef,
$secret = undef,
$mesos_authentication_secret_file = '/etc/marathon/.secret',
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.. I've thought about this a bit and can we maybe do:

$mesos_auth_principal   = undef,
$mesos_auth_secret      = undef,
$mesos_auth_secret_file = '/etc/marathon/.secret',

And then:

if ($mesos_auth_principal != undef) and ($mesos_auth_secret != undef) {
  $secret_options = {
    'mesos_authentication_principal'   => $mesos_auth_principal,
    'mesos_authentication_secret_file' => $mesos_auth_secret_file,
  }
} else { ...

Sorry for chopping and changing... $mesos_authentication_secret_file seems a bit long and also the secret is not much good without a principal.

Does this API sound good to you?

@JayH5
Copy link
Contributor

JayH5 commented Jan 22, 2016

Can you explain to me what you mean by "puppet is doing it's magic to auto-require".. As far as I can see the require is required although things might work "by accident".

@felixb
Copy link
Contributor Author

felixb commented Jan 22, 2016

Setting all auth options as param is fine. It's a bit tricky to get the overload logic.

Autorequires: If Puppet is managing the user or group that owns a file, the file resource will autorequire them. If Puppet is managing any parent directories of a file, the file resource will autorequire them.

Source: https://docs.puppetlabs.com/references/latest/type.html#file

@JayH5
Copy link
Contributor

JayH5 commented Jan 22, 2016

Ah thanks. Learn something new every day about Puppet :/

$mesos_auth_secret = undef,
$mesos_auth_secret_file = '/etc/marathon/.secret',
$java_home = undef,
$java_opts = '-Xmx512m',
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation of the = a bit messed up here since the parameter names got shorter.

@felixb
Copy link
Contributor Author

felixb commented Jan 26, 2016

I just rebased on current develop and fixed the indenting.

@JayH5
Copy link
Contributor

JayH5 commented Jan 26, 2016

Thanks for your work.

I think we should simplify the Mesos authentication logic and just either completely manage the auth principal/secret or not at all. See this diff: https://gist.github.com/JayH5/57f236f74d917442db2b

@felixb
Copy link
Contributor Author

felixb commented Jan 26, 2016

Sounds good to me. I'll merge your git and update the tests.

@JayH5
Copy link
Contributor

JayH5 commented Jan 26, 2016

lgtm. Can you please add a test where one of the principal/secret is not set and the secrets file and properties are not set?

This allows to set up framework authentication
@felixb
Copy link
Contributor Author

felixb commented Jan 26, 2016

these?

@JayH5
Copy link
Contributor

JayH5 commented Jan 26, 2016

Perfect :) Thank you.

JayH5 added a commit that referenced this pull request Jan 26, 2016
@JayH5 JayH5 merged commit 81d619f into praekeltfoundation:develop Jan 26, 2016
@JayH5
Copy link
Contributor

JayH5 commented Jan 29, 2016

@felixb: Finally got around to releasing version 0.3.0 with your changes 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants