Skip to content

Comments

use external config in examples#812

Merged
asfgit merged 3 commits intoapache:masterfrom
ahgittin:external-config-in-examples
Sep 13, 2017
Merged

use external config in examples#812
asfgit merged 3 commits intoapache:masterfrom
ahgittin:external-config-in-examples

Conversation

@ahgittin
Copy link
Contributor

@ahgittin ahgittin commented Sep 8, 2017

some of our tests and examples used plain-text passwords, and this has caused some alarm when people have seen them. we can say "you can use external but we haven't" though isn't it better if our message is "you should use external just like we've shown" ?

this sets up a pre-defined external provider called
brooklyn-demo-sample which defines hidden-brooklyn-password as br00k11n.
it can be overridden; PR to follow for docs to explain this.

i think examples elsewhere should be updated to use this wherever a plaintext password is encoded in a blueprint for anything other than the simplest teaching example (clearly caveated and linking to the docs). i'll do the rest of the stock brooklyn ones shortly.

…, "hidden-brooklyn-password")`

so we can use this in examples rather than a plain-text password
also tweak mysql config to set up users slightly better
@duncangrant
Copy link
Contributor

duncangrant commented Sep 8, 2017

Should we make it easier to disable the provider? That way a "live" deployment of brooklyn would prevent its use. So if a developer forgets to update their yaml to use a real provider it will fail to deploy.

@ahgittin
Copy link
Contributor Author

ahgittin commented Sep 8, 2017

OTOH someone using a live deployment might want to use one of the test blueprints. The docs describe how to change the password:

https://github.com/apache/brooklyn-docs/pull/209/files#diff-b6c73a383415d543418f8762ce2dbe9eR79

I think that's sufficient. If someone really wants to disable that key we can tell them to include just the first line there (the provider is present, but empty); if that becomes a popular feature request then we add the ability to completely disable.

@ahgittin
Copy link
Contributor Author

ahgittin commented Sep 8, 2017

note the two other PRs in downstream projects listed above

grant usage on *.* to 'brooklyn'@'localhost' identified by 'br00k11n';

# the below will create user (and note create user not supported in some dialects)
grant usage on *.* to 'brooklyn'@'%' identified by '${config["creation.script.password"]}';
Copy link
Member

Choose a reason for hiding this comment

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

Previously @aledsage deleted this version of the visitors creation script. We then found out a lot of downstream projects were using it. Do you think it would be worth adding a default here in case that is true for this file?

@m4rkmckenna
Copy link
Member

@ahgittin I think @neykov added a file based external config provider for this, That enable users to configure for development and then swap to a "proper" config provider. You could just configure it by default

@m4rkmckenna
Copy link
Member

Ignore me ... It was never added.

But i do think the flow i mentioned above would be incredibly useful

@aledsage
Copy link
Contributor

@m4rkmckenna there is a "file-based exteranl config supplier" you can use for dev (PropertiesFileExternalConfigSupplier), and then switch to Vault or whatever when you're ready.

But I think that @ahgittin's change here is a separate good addition as it means stock examples can work out-of-the-box without the user having to modify brooklyn.cfg to configure things like that, or to add the desired password to some file.


I agree with @drigodwin's suggestion of giving a default in visitors-creation-script.sql so that we don't break other people's examples that might be referencing that file. For Brooklyn 1.0.0 we can remove the default, perhaps first warning people on the mailing list.

@ahgittin
Copy link
Contributor Author

ahgittin commented Sep 13, 2017

Good point re examples pointing to master. Will add default password in SQL script with comment (in several places, here and in lib). But we should probably change links in example to point at specific branches too.

@asfgit asfgit merged commit 61c1988 into apache:master Sep 13, 2017
asfgit pushed a commit that referenced this pull request Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants