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.

Sign up for GitHub

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

Jump to bottom

feat(auth): remove integrated auth, delegate to proxy #209

Merged
merged 34 commits into from
Dec 21, 2023
Merged

feat(auth): remove integrated auth, delegate to proxy #209

merged 34 commits into from
Dec 21, 2023

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Dec 13, 2023 •
edited
Loading

Welcome to Cryostat3! 👋

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 all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

Related to #208
Depends on cryostatio/cryostat-web#1181

How to manually test:

  1. Check out src/main/webui to -web PR above, then build cryostat3 container
  2. smoketest.bash -OXgtb
  3. After some time, http://localhost:8080 (note 8080, not 8181) should open in your browser. Use user:pass to log in. This is the new authenticating reverse proxy setup which handles authentication/authorization before the user reaches the actual application. The Cryostat server is still listening on port 8181, but you should not be able to reach that anymore - it is bound to localhost only and not exposed from the compose network to your host machine. You can also do ex. http --auth=user:pass :8080/api/v3/targets the same way you would previously have done http --auth=user:pass :8181/api/v3/targets. In any case, this change should be more or less transparent, other than the login screen looking different.

@github-actions GitHub Actions
Copy link

This PR/issue depends on:

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Dec 13, 2023
@andrewazores andrewazores added feat New feature or request safe-to-test and removed needs-triage Needs thorough attention from code reviewers labels Dec 13, 2023
@andrewazores
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Workflow started at 12/14/2023, 10:42:07 AM. View Actions Run.

@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7211175159

1 similar comment
@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7211175159

@andrewazores
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Workflow started at 12/14/2023, 11:47:41 AM. View Actions Run.

@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7211943785

1 similar comment
@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7211943785

@andrewazores andrewazores mentioned this pull request Dec 15, 2023
Merged
7 tasks
@andrewazores
Copy link
Member Author

/build_test

@github-actions GitHub Actions
Copy link

Workflow started at 12/15/2023, 4:42:45 PM. View Actions Run.

@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7227326532

1 similar comment
@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7227326532

@andrewazores andrewazores mentioned this pull request Dec 18, 2023
Merged
7 tasks
@andrewazores
Copy link
Member Author

andrewazores commented Dec 19, 2023 •
edited
Loading

/build_test

might not work due to #216

@github-actions GitHub Actions
Copy link

Workflow started at 12/19/2023, 4:07:00 PM. View Actions Run.

@github-actions GitHub Actions
Copy link

CI build and push: At least one test failed ❌
https://github.com/cryostatio/cryostat3/actions/runs/7267574753

@github-actions GitHub Actions
Copy link

CI build and push: All tests pass ✅
https://github.com/cryostatio/cryostat3/actions/runs/7267574753

ebaron
ebaron approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Looks good. I noticed the integration tests failed, but it looked like something to do with the web-client. I'm assuming that needs to be fixed once both pieces are merged?

@andrewazores
Copy link
Member Author

I think the tests should pass when both this PR and cryostatio/cryostat-web#1181 are merged - they are mutually dependent.

@andrewazores
Copy link
Member Author

Otherwise, there are currently unrelated CI test failures: #216

@mwangggg
Copy link
Member

miwang@unused-10-15-17-161 ~/W/cryostat (main)> podman ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 400d44328e35 quay.io/cryostat/cryostat-db:latest postgres -c fsync... 20 hours ago Up 20 hours 0.0.0.0:37317->5432/tcp gracious_murdock 9f5ac74463ce quay.io/cryostat/cryostat-db:latest postgres -c fsync... 20 hours ago Up 20 hours 0.0.0.0:34817->5432/tcp gallant_moser e763385abad5 quay.io/cryostat/cryostat-db:latest postgres -c fsync... 20 hours ago Up 20 hours 0.0.0.0:38865->5432/tcp interesting_chandrasekhar 1e002c603591 quay.io/cryostat/cryostat-db:latest postgres -c fsync... 20 hours ago Up 20 hours 0.0.0.0:34821->5432/tcp lucid_poitras 2c5fe61acbc0 quay.io/cryostat/cryostat-db:latest postgres -c fsync... 20 hours ago Up 20 hours 0.0.0.0:45959->5432/tcp youthful_elgamal e7e66a69b8e1 quay.io/cryostat/cryostat-db:latest postgres -c fsync... 32 minutes ago Up 32 minutes 0.0.0.0:38837->5432/tcp dreamy_booth
I can see that the cryostat-db containers are still up and running even after I stop the smoketest.bash script. Should there be something in cleanup that stops them?

@andrewazores
Copy link
Member Author

I noticed the lingering database containers before, but it seems to me these get left behind by test runs (mvn test or mvn verify) rather than the smoketest compose setup. Could you double check what's leaving them behind? It should be safe to clean those up with podman kill and podman rm manually first.

@mwangggg
Copy link
Member

Ah yes I just checked and it's not because of the smoketest.bash script

@andrewazores
Copy link
Member Author

See #221

mwangggg
mwangggg approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

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

looks good to me

@andrewazores andrewazores merged commit cb0ceb7 into cryostatio:main Dec 21, 2023
6 of 7 checks passed
@andrewazores andrewazores deleted the auth-proxy branch December 21, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwangggg mwangggg mwangggg approved these changes

@ebaron ebaron ebaron approved these changes

@aali309 aali309 Awaiting requested review from aali309

Assignees
No one assigned
Labels
feat New feature or request safe-to-test
Projects
No open projects
3.0.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewazores @mwangggg @ebaron