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

refactor minio.credentials #901

Merged
merged 4 commits into from
May 31, 2020

Conversation

balamurugana
Copy link
Member

@balamurugana balamurugana commented Apr 26, 2020

Depends on Minio.get_assume_role_creds() in #900

@balamurugana balamurugana force-pushed the refactor-minio-credentials branch 10 times, most recently from 8b14018 to 5bec5d4 Compare April 27, 2020 16:03
Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

Just few minor nits - Otherwise LGTM

minio/credentials/credentials.py Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
minio/credentials/__init__.py Outdated Show resolved Hide resolved
Praveenrajmani
Praveenrajmani previously approved these changes Apr 30, 2020
Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile Outdated Show resolved Hide resolved
minio/credentials/credentials.py Outdated Show resolved Hide resolved
Praveenrajmani
Praveenrajmani previously approved these changes Apr 30, 2020
@vadmeste
Copy link
Member

vadmeste commented May 7, 2020

@balamurugana could you explain what the API change done in this PR ?

Also could you make sure that examples are updated ?

@balamurugana
Copy link
Member Author

balamurugana commented May 7, 2020

@balamurugana could you explain what the API change done in this PR ?

Below is the design change I proposed earlier and this PR is a refactor with this design.

from abc import ABCMeta, abstractmethod
from datetime import datetime


class Value():
    """
    Denotes credential values such as access key, secret key and session token.
    """
    def init(self, access_key, secret_key, session_token):
        self.access_key = access_key
        self.secret_key = secret_key
        self.session_token = session_token


class Provider():
    """Credential retriever."""
    metaclass = ABCMeta

    @abstractmethod
    def retrieve(self) -> (Value, datetime):
        """Retrieve credential value and its expiry."""


class Credentials():
    def __init__(self, provider):
        self._provider = provider
        self._value = None
        self._validity = None

    def get(self, force=False):
        if (
                not self._value or
                (self._validity and self._validity < datetime.utcnow()) or
                force
        ):
            self._value, self._validity = self._provider.retrieva()
        return self._value

Also could you make sure that examples are updated ?

Let me check once again.

@balamurugana balamurugana force-pushed the refactor-minio-credentials branch 6 times, most recently from 0cb6611 to 9d01e93 Compare May 10, 2020 05:59
@balamurugana balamurugana force-pushed the refactor-minio-credentials branch 5 times, most recently from 582b8b5 to faf329b Compare May 15, 2020 18:37
Praveenrajmani
Praveenrajmani previously approved these changes May 18, 2020
Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Few comments, LGTM otherwise. Not tested yet

# ignore this error and try other providers.
pass

raise ValueError("no credentials retrieved")
Copy link
Member

@vadmeste vadmeste May 26, 2020

Choose a reason for hiding this comment

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

Maybe here we should return the last encountered ValueError exception.. so at least the user can see a more self explaining error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

minio/api.py Outdated
@@ -178,10 +177,9 @@ def __init__(self, endpoint, access_key=None,
self._credentials = credentials or Credentials(
provider=Chain(
providers=[
Static(access_key, secret_key, session_token),
Static(Value(access_key, secret_key, session_token)),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is enough if we can credentials in this form:

  Static(access_key, secret_key, session_token),

and create a Value instance inside Static init() function.

Let's review the API with @abperiasamy one last time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed to Static(access_key, secret_key, session_token=None, expiry=None)

@balamurugana balamurugana force-pushed the refactor-minio-credentials branch 2 times, most recently from a15a235 to 1579f2e Compare May 26, 2020 10:42
@@ -181,7 +180,6 @@ def __init__(self, endpoint, access_key=None,
Static(access_key, secret_key, session_token),
EnvAWS(),
EnvMinio(),
IamEc2MetaData(),
Copy link
Member

Choose a reason for hiding this comment

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

Well, this is a breaking change, we should not forget about mentioning it when we release minio-py

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think we are covering in #902

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM & tested, we probably need to write all possible breaking things in a document before we release

@kannappanr
Copy link
Collaborator

ping @Praveenrajmani

@balamurugana balamurugana merged commit 5e3a174 into minio:master May 31, 2020
@balamurugana balamurugana deleted the refactor-minio-credentials branch June 2, 2020 17:46
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.

None yet

4 participants