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

Check to make sure only one AWX token exists #331

Closed
wants to merge 7 commits into from

Conversation

Alex-Izquierdo
Copy link
Contributor

@Alex-Izquierdo Alex-Izquierdo commented Aug 2, 2023

Recreates the original PR #280, which for some reason it can not be reopened after the migration to a public repo.

Only grab the first token. So checking to make sure we have at least one token makes sense.
Jira: https://issues.redhat.com/browse/AAP-12376

@Alex-Izquierdo Alex-Izquierdo requested a review from a team as a code owner August 2, 2023 10:05
@mkanoor
Copy link
Contributor

mkanoor commented Aug 2, 2023

@Alex-Izquierdo I think we should also fix this related issue

controller_info = ControllerInfo(

Since it blindly assumes that there is a token available when the ansible-rulebook comes back on the websocket channel looking for parameters and the missing token causes an exception to be thrown. For now I think the assumption is you always need a token as we make more changes we can parse the rulebook and see if it has run_job_template action and then prompt for a token but even in that case we need this code path should be protected. There is a stack trace for this code in many JIRA tickets, the server dying ends up closing the websocket connection and the ansible rulebook ends saying error exchanging web socket data. https://issues.redhat.com/browse/AAP-11677 https://issues.redhat.com/browse/AAP-11681

Since we have the activation instance here we should mark it as broken from the server side itself if the token is missing. The server provides the token to ansible-rulebook.

rulesets, extra_var = await self.get_resources(message.activation_id)

Since we have the activation id we should be able to set it to Failure from the server side itself instead of having the ansible-rulebook say that web socket connection closed.

@mkanoor
Copy link
Contributor

mkanoor commented Aug 2, 2023

@Alex-Izquierdo There needs to be another check here for the token

instance = ActivateRulesets().activate(

I think we need to figure out the token based on the user_id that created the activation since if multiple users create the activations we need to find on whose behalf the worker is running the activation.
user_id=activation_instance.activation.user_id

Maybe this function
def get_awx_token(self, message):
can come out and be placed in a shared location and be called from multiple places. We can pass in the activation id instead of the message. It could also throw an exception saying "No tokens defined"

I think there are at least 3 places we need to block it

  1. From the UI side when the activation is created the first time (exists)
Screenshot 2023-08-02 at 4 42 26 PM
  1. When the activation is started by a worker before we call K8S or podman to run the activation (missing)
  2. When the ansible-rulebook comes on the websocket channel asking for the payload (missing)

I think the UI also has code to ensure that only 1 token is created I think this PR is guarding on the backend so that a REST API call doesn't end up adding an extra token.

@Alex-Izquierdo
Copy link
Contributor Author

Hi @mkanoor I totally agree with all that you say, we must fix the usage of the token. But it looks like that is out of the scope of this PR which its goal is to prevent to have more than one token. I propose to create a new issue or update some existing one (like this one ) and address all these issues in a different PR.

@@ -70,7 +71,7 @@ class NoControllerToken(APIException):
class TooManyControllerTokens(APIException):
status_code = 422
Copy link
Member

Choose a reason for hiding this comment

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

Should we use this status.HTTP_422_UNPROCESSABLE_ENTITY instead of just hard-coded integer? Or we could inherit from Unprocessable class on line 48 and customize detail field since they fall under the same error code. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

@@ -109,7 +94,7 @@ def test_create_token_missing_field(client: APIClient, user: models.User):


@pytest.mark.django_db(transaction=True)
def test_create_token_duplicate_name(client: APIClient, user: models.User):
def test_create_token_to_many_tokens(client: APIClient, user: models.User):
Copy link
Member

Choose a reason for hiding this comment

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

Possibly a typo: _to_many_token ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

likely :)

@Dostonbek1
Copy link
Member

Except for the above minor comments, LGTM. Works as expected.
Screenshot 2023-08-09 at 5 00 25 PM

@Alex-Izquierdo
Copy link
Contributor Author

This PR is on hold until a final decision is made on how to address this issue.

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