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

Adding functionality to set same token value across cluster #10801

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shivamdurgbuns
Copy link
Member

@shivamdurgbuns shivamdurgbuns commented Nov 5, 2024

This is for Vault Configuration.
fixes: https://issues.redhat.com/browse/OCSQE-1636

@shivamdurgbuns shivamdurgbuns requested a review from a team as a code owner November 5, 2024 07:42
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines label Nov 5, 2024
dahorak
dahorak previously approved these changes Nov 5, 2024
petr-balogh
petr-balogh previously approved these changes Nov 5, 2024
vavuthu
vavuthu previously approved these changes Nov 5, 2024
shylesh
shylesh previously approved these changes Nov 5, 2024
Copy link
Contributor

@shylesh shylesh left a comment

Choose a reason for hiding this comment

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

lgtm

ocs_ci/utility/kms.py Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Nov 6, 2024
petr-balogh
petr-balogh previously approved these changes Nov 6, 2024
clacroix12
clacroix12 previously approved these changes Nov 7, 2024
@shivamdurgbuns
Copy link
Member Author

@shivamdurgbuns
Copy link
Member Author

@shivamdurgbuns
Copy link
Member Author

ocs_ci/utility/kms.py Outdated Show resolved Hide resolved
ocs_ci/utility/kms.py Outdated Show resolved Hide resolved
Copy link
Contributor

@clacroix12 clacroix12 left a comment

Choose a reason for hiding this comment

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

Couple things here:

  • changed set_vault_token from a staticmethod to a standard method since we need an instance reference.
  • changed the name to get_vault_token and updated the docstring since we are returning the value

ocs_ci/utility/kms.py Outdated Show resolved Hide resolved
@@ -578,7 +593,7 @@ def vault_create_policy(self, policy_name=None):
raise VaultOperationError(
f"Failed to create policy f{self.vault_policy_name}"
)
self.vault_path_token = self.generate_vault_token()
self.vault_path_token = self.set_vault_token(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.vault_path_token = self.set_vault_token(self)
self.vault_path_token = self.get_vault_token()

ocs_ci/utility/kms.py Outdated Show resolved Hide resolved
petr-balogh
petr-balogh previously approved these changes Jan 7, 2025
clacroix12
clacroix12 previously approved these changes Jan 7, 2025
@openshift-ci openshift-ci bot added the lgtm label Jan 7, 2025
@petr-balogh petr-balogh dismissed their stale review January 7, 2025 16:56

This approach will not work

str: Vault token
"""
if self.vault_path_token is None:
self.vault_path_token = self.generate_vault_token()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.vault_path_token = self.generate_vault_token()
Vault.vault_path_token = self.generate_vault_token()

You need to change class variable, not instance variable here otherwise it will not work.

I checked example you shared with me and little update to prove the uuid will be same in both calls and it's not:

import uuid

class Vault():
    vault_path_token = None

    def __init__(self):
        self.vault_path_token = Vault.vault_path_token

    def get_vault_token(self):
        """
        Get the Vault token value. Generate it first if it does not already exist.
        Returns:
            str: Vault token
        """
        if self.vault_path_token is None:
            self.vault_path_token = self.generate_vault_token()
        return self.vault_path_token

    def deploy(self):
        self.vault_path_token = self.get_vault_token()

    def generate_vault_token(self):
        print("inside generate_vault_token")
        # Generate a unique random value
        return str(uuid.uuid4())


v1 = Vault()
print(v1.vault_path_token)
v1.deploy()
print(v1.vault_path_token)

v2 = Vault()
v2.deploy()
print(v2.vault_path_token)

Output:

None
inside generate_vault_token
368f386a-dce3-4370-b60a-4309acf26fed
inside generate_vault_token
393d3edc-46a7-493c-b45c-8037e731924c

You clearly see it's different number - but we need to achieve to have same for both instances

Updated working code

import uuid

class Vault():
    vault_path_token = None

    def __init__(self):
        self.vault_path_token = Vault.vault_path_token

    def get_vault_token(self):
        """
        Get the Vault token value. Generate it first if it does not already exist.
        Returns:
            str: Vault token
        """
        if self.vault_path_token is None:
            Vault.vault_path_token = self.generate_vault_token()
        return Vault.vault_path_token

    def deploy(self):
        self.vault_path_token = self.get_vault_token()

    def generate_vault_token(self):
        print("inside generate_vault_token")
        # Generate a unique random value
        return str(uuid.uuid4())


v1 = Vault()
print(v1.vault_path_token)
v1.deploy()
print(v1.vault_path_token)

v2 = Vault()
v2.deploy()
print(v2.vault_path_token)

Output:

None
inside generate_vault_token
ba5e5c53-f49c-4fb4-b5c4-22c4f945ec1d
ba5e5c53-f49c-4fb4-b5c4-22c4f945ec1d

You see we have same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for proof check, udpated

@openshift-ci openshift-ci bot removed the lgtm label Jan 7, 2025
Converting static method to class method

Updating class method to static

Adding documentation for the HCP using vSphere

Removing static function with just normal class function that updates token value

Update ocs_ci/utility/kms.py

making vault_path_token variable class variable

Signed-off-by: Shivam Durgbuns <[email protected]>
ocs_ci/utility/kms.py Show resolved Hide resolved
ocs_ci/utility/kms.py Show resolved Hide resolved
@shivamdurgbuns
Copy link
Member Author

Signed-off-by: Shivam Durgbuns <[email protected]>
@shivamdurgbuns shivamdurgbuns added the Verified Mark when PR was verified and log provided label Jan 9, 2025
@openshift-ci openshift-ci bot added the lgtm label Jan 9, 2025
Copy link

openshift-ci bot commented Jan 9, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: petr-balogh, shivamdurgbuns

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +147 to +149
if self.vault_path_token is None:
Vault.vault_path_token = self.generate_vault_token()
return Vault.vault_path_token
Copy link
Contributor

Choose a reason for hiding this comment

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

If I didn't miss anything, every time you will call get_vault_token from the firstly created Vault object, you will get new token - because you are checking it in the object parameterself.vault_path_token but updating it in the class variable Vault.vault_path_token. Is that intentional? Or did I miss something there?

Copy link
Contributor

Choose a reason for hiding this comment

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

The easiest solution here, would be to save the newly generated token to both Vault.vault_path_token and self.vault_path_token directly in the get_vault_token method, but it would be quite ugly solution I think.

The best solution might be to make self.vault_path_token a property, which would check and return the Vault.vault_path_token and prospectively call the self.generate_vault_token() if needed (the code of the property will be very similar as the get_vault_token - except it will use Vault.vault_path_token everywhere and the get_vault_token could be removed.)

Something like this:

@property
def vault_path_token(self):
    if Vault.vault_path_token is None:
        Vault.vault_path_token = self.generate_vault_token()
    return Vault.vault_path_token

and prospectively:

@vault_path_token.setter
def vault_path_token(self, value):
    Vault.vault_path_token = value

but the setter might not be needed.

And you will have to remove the line ~599:

self.vault_path_token = self.get_vault_token()

Copy link
Contributor

@dahorak dahorak left a comment

Choose a reason for hiding this comment

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

As mentioned above, the existing get_vault_token method will not work consistently and in some circumstances it might generate new value for for each execution.

@openshift-ci openshift-ci bot removed the lgtm label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S PR that changes 10-29 lines Verified Mark when PR was verified and log provided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants