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

fix(custom-targets): fix adding jmx-auth custom targets #1408

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Mar 14, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #1404

Description of the change:

This change allows the Custom Target creation post handler to use credentials in the dao directly (also check session credentials if any). This is because before, the handler would query existing targets in the database and find and use any credentials that match a target using the credentials match expression.

Tests not fixed yet just to confirm that this implementation is what is needed for the web-client to use.

How to manually test:

  1. Run the pr.
  2. Create a credential with a match expression that matches a custom target that you want to add (e.g. target.connectUrl == "service:jmx:rmi:///jndi/rmi://localhost:9091/jmxrmi")
  3. Go to topology view to add custom target.
  4. Use the connectUrl (e.g. service:jmx:rmi:///jndi/rmi://localhost:9091/jmxrmi), test the connection, and add the target and notice that it passes.
  5. Try with no existing credentials and see that the creation fails.

@maxcao13
Copy link
Member Author

maxcao13 commented Mar 14, 2023

Seems like as of now there are 4 cases:

  • dryRun=false && credentials=false
    • adds a target as normal
  • dryRun=true && credentials=false
    • doesnt add target but returns whether target can be created or not
  • dryRun=false && credentials=true
    • adds credentials to the database and adds a target
  • dryRun=true && credentials=true
    • doesnt add target but returns whether target can be created with the credentials or not

I think if we are using sessionCredentials and dryRun=false, the web-client should be careful to never supply the request with credentials since the backend does not know which settings the web-client is using.

Instead maybe if (for the web-client) we are using session credentials, we attach the credentials as jmx-auth headers, and if we are using backend credentials, we attach it as form body parameters like it is currently implemented?

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1408-94d357e5af522470171505dba28db6278f6f808e sh smoketest.sh

@andrewazores
Copy link
Member

the web-client should be careful to never supply the request with credentials

What does this mean? As in supplying the target creation test request with body paremeter credentials (which become stored credentials)?

@maxcao13
Copy link
Member Author

the web-client should be careful to never supply the request with credentials

What does this mean? As in supplying the target creation test request with body paremeter credentials (which become stored credentials)?

Yeah, that's what I meant, but I'm thinking not to do it this way and instead just use another query parameter which lets the backend know whether to save the supplied credentials to the db as storedCredentials or to save it as sessionCredentials.

@tthvo
Copy link
Member

tthvo commented Mar 14, 2023

Makes sense. Seems neater to use query params (saving some refactoring for frontend)...

@andrewazores
Copy link
Member

Seems fine to add a query param like storeCredentials. If it's true the provided credentials are stored in the keyring, otherwise they are only for the current request ("session").

@maxcao13
Copy link
Member Author

I will fix tests once everything is ok'd.

@andrewazores
Copy link
Member

Looks good.

@tthvo
Copy link
Member

tthvo commented Mar 14, 2023

Looks good to me too!

@tthvo
Copy link
Member

tthvo commented Mar 14, 2023

Tested with cryostatio/cryostat-web#907. Credentials are found and target creation form succeeds.

Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
Signed-off-by: Max Cao <macao@redhat.com>
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1408-6247a2e76d6c509faca53ffd2daf7b6ce53b78ee sh smoketest.sh

@andrewazores andrewazores merged commit 1b02283 into cryostatio:main Mar 14, 2023
@maxcao13 maxcao13 deleted the custom-target-creation-bug branch March 14, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Could not find credentials when creating custom targets
3 participants