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

Issue 1131 | Drop keycloak #1132

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Conversation

IceKhan13
Copy link
Member

@IceKhan13 IceKhan13 commented Nov 29, 2023

Summary

Drop keycloak

Details and comments

Closes #1131

@IceKhan13 IceKhan13 mentioned this pull request Nov 29, 2023
@psschwei
Copy link
Collaborator

Before merging, I'd like to test this against local Kubernetes too... (I'll run on kind once the PR is out of draft)

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

I had almost the same so I'm fine with this PR, @IceKhan13 . What I would do is simplify it a bit, let keycloak stuff and remove everything in another PR, what do you think?

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

We have tons of configurations related with keycloak that it will be better review in apart from this I think.

@IceKhan13
Copy link
Member Author

We have tons of configurations related with keycloak that it will be better review in apart from this I think.

Do you mean configurations in helm charts?

What I would do is simplify it a bit

most of the changes in just notebooks diffs :)
if you look at code lines it will be like 50 lines of change, which is simple, I would say 😄

I do not mind if you close this PR and push your changes, I was just sitting in a meeting and decided to take this one.

@IceKhan13 IceKhan13 marked this pull request as ready for review November 29, 2023 19:22
@IceKhan13 IceKhan13 requested a review from Tansito November 29, 2023 19:22
@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

Do you mean configurations in helm charts?

Mostly yeah, but around applications too. Something that we can clean in the next PR.

most of the changes in just notebooks diffs :)
if you look at code lines it will be like 50 lines of change, which is simple, I would say 😄

I do not mind if you close this PR and push your changes, I was just sitting in a meeting and decided to take this one.

Yeah, no it's fine. Just to mention in case you were thinking to include to remove all the configuration stuff. That maybe we can let it for another PR.

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

Should we remove this from client too, @IceKhan13 ?:

https://github.com/Qiskit-Extensions/quantum-serverless/blob/main/client/quantum_serverless/core/provider.py#L455

@IceKhan13
Copy link
Member Author

Oh, let me remove from client to. YOu are right.

@IceKhan13 IceKhan13 marked this pull request as draft November 29, 2023 19:29
@IceKhan13 IceKhan13 marked this pull request as ready for review November 29, 2023 19:55
@psschwei
Copy link
Collaborator

psschwei commented Nov 29, 2023

Got an error when running on Kind:

QuantumServerlessException: Cannot verify token.

(did a pip install . on client, so should be using the most recent updates...)

edit: error was when running this block:

serverless = ServerlessProvider(
    token=os.environ.get("GATEWAY_TOKEN", "awesome_token"),
    host=os.environ.get("GATEWAY_HOST", "http://localhost:8010"),
)
serverless

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

Got an error when running on Kind:

QuantumServerlessException: Cannot verify token.

Did you change the authentication mechanism @psschwei ? Now auth mechanism must be mock_token for when you want to test something.

@psschwei
Copy link
Collaborator

Did you change the authentication mechanism

No, wasn't expecting that I'd need to (I assumed it would work out of the box)... let me try again

@psschwei
Copy link
Collaborator

hmmm... still fails with

 {
    "name": "SETTINGS_AUTH_MECHANISM",
    "value": "mock_token"
  },

@IceKhan13
Copy link
Member Author

@psschwei did you rebuild gateway image?

@psschwei
Copy link
Collaborator

@psschwei did you rebuild gateway image?

yes, built from source

@psschwei
Copy link
Collaborator

envvar is being picked up inside the gateway pod...

I have no name!@gateway-7b444889b6-dkjh8:/usr/src/app$ python
Python 3.9.16 (main, May 23 2023, 14:24:31) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.environ.get("SETTINGS_AUTH_MECHANISM")
'mock_token'
>>> 

@IceKhan13
Copy link
Member Author

🤔

@psschwei
Copy link
Collaborator

looks like the keycloak pod is still there... let me try deleting that (or maybe I should wait for the helm config update in the next PR...)

@IceKhan13
Copy link
Member Author

keycloak chart should not affect it by any means

@psschwei
Copy link
Collaborator

does the postgres username matter? looks like I was using bn_keycloak, but not sure if that user is gone now...

@IceKhan13
Copy link
Member Author

nope, it does not matter

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

Helm should continue being the same with the difference in the auth.mechanism value I think 🤔

@Tansito Tansito self-requested a review November 29, 2023 21:18
@psschwei
Copy link
Collaborator

Ok, I figured out the error... for reference, this was the code block:

serverless = ServerlessProvider(
    token=os.environ.get("GATEWAY_TOKEN", "awesome_token"),
    host=os.environ.get("GATEWAY_HOST", "http://localhost:8010"),
)
serverless

Can anyone spot the problem? 😄

@Tansito
Copy link
Member

Tansito commented Nov 29, 2023

Damn port?

@psschwei
Copy link
Collaborator

Damn port?

You win 😆

@IceKhan13
Copy link
Member Author

ref: #1133

@IceKhan13 IceKhan13 requested a review from psschwei November 29, 2023 21:36
@IceKhan13
Copy link
Member Author

IceKhan13 commented Nov 29, 2023

Tested it with rancher locally too. Got the same error like Paul :) ref above

Copy link
Collaborator

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

Worked for me on Kubernetes, code changes look good

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

It looks for me good too. Tomorrow I can upload helm configuration.

@IceKhan13
Copy link
Member Author

Tomorrow I can upload helm configuration.

Perfect!

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.

Drop keycloak support
3 participants