Secrets manager and base provider proposal #2758

protatremy · 2017-02-08T15:52:40Z

Proposal to implement Buildbot secrets service:

secrets manager (get API)
secret provider base

Contributor Checklist:

I have updated the unit tests
I have created a file in the master/buildbot/newsfragment directory (and read the README.txt in that directory)
I have updated the appropriate documentation

tardyp

Looks pretty solid! congrats!
a few nitpicks to fix goal is to go 100% coverage

tardyp · 2017-02-08T16:14:40Z

master/buildbot/secrets/secret.py

+        """
+        source of the secret
+        """
+        return self._source


please try to get 100% coverage. install codecov.io chrome extension to see that this line is not covered

tardyp · 2017-02-08T16:17:04Z

master/buildbot/secrets/secret.py

+    - source: provider  to retrieve secret
+    - key: secret key identifier
+    - value: secret value
+    - props: all other needed properties


what kind of properties do we need for secrets?

tardyp · 2017-02-08T16:17:18Z

master/buildbot/secrets/secret.py

+class SecretDetails(object):
+    """
+    A SecretDetails object has secrets attributes:
+    - source: provider  to retrieve secret


provider where the secret was retrieved

codecov · 2017-02-08T18:10:16Z

Codecov Report

Merging #2758 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2758      +/-   ##
==========================================
+ Coverage   87.31%   87.33%   +0.02%     
==========================================
  Files         305      308       +3     
  Lines       32564    32622      +58     
==========================================
+ Hits        28432    28491      +59     
+ Misses       4132     4131       -1

Impacted Files	Coverage Δ
master/buildbot/secrets/secret.py	`100% <100%> (ø)`
master/buildbot/secrets/providers/base.py	`100% <100%> (ø)`
master/buildbot/secrets/manager.py	`100% <100%> (ø)`
worker/buildbot_worker/init.py	`87.09% <ø> (-0.41%)`	❌
master/buildbot/init.py	`89.65% <ø> (-0.35%)`	❌
master/buildbot/scripts/cleanupdb.py	`96.49% <ø> (-0.07%)`	❌
master/buildbot/db/pool.py	`86.92% <ø> (ø)`	✅
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16a3c75...abe4e0a. Read the comment docs.

tardyp · 2017-02-09T20:29:10Z

master/buildbot/secrets/manager.py

+        secret_detail = None
+        providers = self.master.config.secretsManagers
+        for provider in providers:
+            value = provider.get(secret)


I think we need this get to be yield. Some providers will need to make network access to do the get

tardyp · 2017-02-09T20:29:28Z

master/buildbot/secrets/manager.py

+    """
+    name = 'secrets'
+
+    def get(self, secret, *args, **kwargs):


this function shall return deferred (inlinecallbacks)

tardyp · 2017-02-09T20:31:41Z

master/buildbot/secrets/provider/base.py

+    def get(self, *args, **kwargs):
+        """
+        this should be an abstract method
+        """


the you can mark it abstract https://docs.python.org/2/library/abc.html

tardyp · 2017-02-09T20:33:40Z

master/docs/developer/secrets.rst

+      def __init__(self, provider, key, value, props=None):
+
+A ``secretDetails`` is a python object initialized with the following parameters:
+- provider name to retrieve secrets,


name of provider where secret has been retrieved

tardyp · 2017-02-09T20:34:30Z

master/docs/developer/secrets.rst

+- value returned by the provider API
+- properties if needed.
+
+Each parameter is an object property that should returned the value.


its not very clear to me what you mean

tardyp · 2017-02-09T20:35:10Z

master/docs/developer/secrets.rst

+
+  c['secretsProviders'] = [SecretsProviderOne(params), SecretsProviderTwo(params)]
+
+If more than one provider is defined in the configuration, the manager returns the first founded value.


the manager returns the first value found.

tardyp · 2017-02-09T20:36:23Z

master/docs/developer/secrets.rst

+    secretDetails = secretsService.get(secret)
+
+The get API take the secret key as parameters and read the configuration to obtains the list of configured providers.
+The manager get the selected provider and returns a ``SecretDetails``.


+The manager call the get method of the configured provider and returns a SecretDetails if the call succeed.

tardyp · 2017-02-09T20:38:42Z

master/buildbot/test/unit/test_fake_secrets_manager.py

+                                                                 "other2": "value"})
+                                              ]
+        SecretManager.master = self.master
+        expectedSecretDetail = SecretDetails(


If not found, I think it should return None, not secretDefails(fakestorage2, None)

I preferred to return the object because it contains the provider class name information, that will be used to prompt a log error indication from where the information has not been found.

we spoke about that together, I changed it. The get API returns the first found value else returns None

tardyp · 2017-02-09T20:39:06Z

master/buildbot/test/unit/test_fake_secrets_manager.py

+                                              ]
+        SecretManager.master = self.master
+        expectedSecretDetail = SecretDetails(
+            OtherFakeSecretStorage({"foo2": "bar",


why dont you just write OtherFakeSecretStorage.__name__ ?

get is an abstract method in the provider base fixes in the rst

tardyp · 2017-02-13T09:12:07Z

master/buildbot/secrets/manager.py

+        """
+        secret_detail = None
+        providers = self.master.config.secretsManagers
+        for provider in providers:


you should return the first that is not None.

for provider in providers: value = yield provider.get(secret) if value is not None: source_name = provider.__class__.__name__ defer.returnValue(SecretDetails(source_name, secret, value)) # return None is implicit

tardyp · 2017-02-13T09:13:05Z

master/buildbot/test/unit/test_fake_secrets_manager.py

+                                             "foo3",
+                                             None)
+        secret_result = yield secret_service_manager.get("foo3")
+        self.assertEqual(secret_result, expectedSecretDetail)


Need to add a test for 2 providers having the secret in their DB (but not the same value):
should return the value of the first provider

tardyp · 2017-02-13T09:14:26Z

master/buildbot/secrets/provider/__init__.py

+# this program; if not, write to the Free Software Foundation, Inc., 51
+# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Copyright Buildbot Team Members


We will have several providers, so we should call this module buildbot.secrets.providers (with an s)

tardyp · 2017-02-13T13:10:27Z

master/buildbot/secrets/manager.py

+        for provider in providers:
+            value = yield provider.get(secret)
+            source_name = provider.__class__.__name__
+            if value:


if value is not None (a secret might be empty string)

tardyp · 2017-02-13T13:12:42Z

master/buildbot/test/unit/test_fake_secrets_manager.py

+                                              ]
+        SecretManager.master = self.master
+        secret_result = yield secret_service_manager.get("foo3")
+        self.assertEqual(secret_result, None)


I don't see the test with two providers providing the same secret.

Need another test with a secret value being empty string ( empty string is valid secret)

tardyp requested changes Feb 8, 2017

View reviewed changes

protatremy and others added 3 commits February 9, 2017 10:38

Secrets manager and base provider

7f0b373

Remove props from secretDetail and add coverage

a7e728c

Code coverage extented to str secretDetail

8ee5636

protatremy force-pushed the secretmanager branch from cb5cf25 to 8ee5636 Compare February 9, 2017 10:51

Pep8 errors fixes

708b34c

protatremy force-pushed the secretmanager branch from 9e8d715 to 708b34c Compare February 9, 2017 13:39

isort+autopep8 run

4e384a1

tardyp requested changes Feb 9, 2017

View reviewed changes

get is now a deferred in the manager

cd569c1

get is an abstract method in the provider base fixes in the rst

tardyp requested changes Feb 13, 2017

View reviewed changes

protatremy added 2 commits February 13, 2017 11:40

The get API returns the value found value else returns None

e0ec3ef

remove secret_detail not used variable

7a60974

tardyp reviewed Feb 13, 2017

View reviewed changes

rename folder providers, get API should return an empty value

abe4e0a

tardyp merged commit f56dff6 into buildbot:master Feb 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secrets manager and base provider proposal #2758

Secrets manager and base provider proposal #2758

protatremy commented Feb 8, 2017 •

edited

Loading

tardyp left a comment

tardyp Feb 8, 2017

tardyp Feb 8, 2017

tardyp Feb 8, 2017

protatremy Feb 8, 2017

codecov bot commented Feb 8, 2017 •

edited

Loading

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

tardyp Feb 9, 2017

protatremy Feb 10, 2017 •

edited

Loading

tardyp Feb 9, 2017

tardyp Feb 13, 2017

tardyp Feb 13, 2017

tardyp Feb 13, 2017

tardyp Feb 13, 2017

tardyp Feb 13, 2017


		c['secretsProviders'] = [SecretsProviderOne(params), SecretsProviderTwo(params)]

		If more than one provider is defined in the configuration, the manager returns the first founded value.

Secrets manager and base provider proposal #2758

Secrets manager and base provider proposal #2758

Conversation

protatremy commented Feb 8, 2017 • edited Loading

Contributor Checklist:

tardyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 8, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protatremy Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protatremy commented Feb 8, 2017 •

edited

Loading

codecov bot commented Feb 8, 2017 •

edited

Loading

protatremy Feb 10, 2017 •

edited

Loading