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

[Bug]: Rest APIs not working with TLS enabled #36724

Open
1 task done
nish112022 opened this issue Oct 9, 2024 · 12 comments
Open
1 task done

[Bug]: Rest APIs not working with TLS enabled #36724

nish112022 opened this issue Oct 9, 2024 · 12 comments
Assignees
Labels
kind/bug Issues or changes related a bug triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@nish112022
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Environment

- Milvus version:2.4.x
- Deployment mode(standalone or cluster): standalone
- MQ type(rocksmq, pulsar or kafka):    
- SDK version(e.g. pymilvus v2.0.0rc2):
- OS(Ubuntu or CentOS): 
- CPU/Memory: 
- GPU: 
- Others:

Current Behavior

When I run this

curl --request POST --url "http://localhost:19530/v2/vectordb/collections/describe"      --header "Authorization: Bearer 'root:milvus'"      --header "accept:
 application/json"      --header "content-type: application/json" -d '{
"dbName": "default",
"collectionName": "hello_milvus"
}'

It works, giving me the collection details.However, when I run the same command after enabling TLS mode=1 I get the error that application/grpc expected not application json

Link to thread:https://discord.com/channels/1160323594396635310/1293419285045182516

Expected Behavior

The command should work

curl --request POST --url "http://localhost:19531/v2/vectordb/collections/describe" --header "Authorization: Bearer 'admin:3kMjCMzfL25w'" --header "accept: application/json" --header "content-type: application/json" --cacert ./milvus-rest.cert -d '{ "dbName": "default", "collectionName": "hello_milvus" }'

Steps To Reproduce

Enable TLSMode=1
Run Milvus
Give Curl command

Milvus Log

No response

Anything else?

No response

@nish112022 nish112022 added kind/bug Issues or changes related a bug needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 9, 2024
@yanliang567
Copy link
Contributor

/assign @haorenfsa
does we need -secure in the request in this case?
/unassign

@yanliang567 yanliang567 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 10, 2024
@yanliang567 yanliang567 added this to the 2.4.13 milestone Oct 10, 2024
@haorenfsa
Copy link
Contributor

haorenfsa commented Oct 10, 2024

Hi @nish112022, thank you for the feedback. After tlsMode enabled, we should use the scheme https:// instead of http:// in url. You can try use the script below:

curl --request POST --url "https://localhost:19530/v2/vectordb/collections/describe"      --header "Authorization: Bearer 'root:milvus'"      --header "accept:
 application/json"      --header "content-type: application/json" -d '{
"dbName": "default",
"collectionName": "hello_milvus"
}'

@nish112022
Copy link
Contributor Author

MyBad @haorenfsa ,I should have explained a bit more.When I use the script you gave me , I get this error:
Screenshot 2024-10-10 at 4 24 10 PM

When I give the cert as -cacert field I get this error:application/grpc expected not application json
Screenshot 2024-10-10 at 4 23 24 PM

Below are the parameters I use for my hello_milvus.py.The same parameters should work here as well.

connections.connect(
  alias='defa',
  secure=True, server_name='localhost',
  server_pem_path='/root/milvus_2.4/milvus/configs/cert/server.pem',
  user='root',
  password='Milvus',
  host='localhost',
  port='19530',
)

@haorenfsa
Copy link
Contributor

@nish112022 Oh, I get it. It seems to be the issue of golang's mux server when serving https & gRPC over TLS on the same port.

For now you can walk around by configuring the http server to another port (for example, 8080) in milvus configuration:

proxy:
  http:
    port: 8080

@haorenfsa
Copy link
Contributor

or you may also try force your client to use http/1.1, without changing configuration.
curl --http1.1 ( same thing needs to be done with your http lib if you want to write code to access it later)
Since ALPN protocol decides which handler will handle the request, h2 for gRPC and http/1.1 for restful.
image

@nish112022
Copy link
Contributor Author

It seems forcing http1.1 doesn't work as well.To me it seems due to APLN. not being able to select the configuration correctly.
Screenshot 2024-10-10 at 5 08 52 PM

@haorenfsa
Copy link
Contributor

@nish112022 Yes, it's a bug in server side. by the way I just noticed that in the newest release, @chyezh has forbidden enable tls when enable restful & grpc in the same port.

e34fa04#diff-6e86fc33d8695678bc197f60ce811f0a362cb3034cda0cc03823b5bc8822920eR75

I think we should fix this instead of forbidding the usage, in a way similar to this: https://ahmet.im/blog/grpc-http-mux-go/

@haorenfsa
Copy link
Contributor

@nish112022 For now only this works for me. We'll fix this later. Thank you again for feedback.

proxy:
  http:
    port: 8080

@nish112022
Copy link
Contributor Author

@nish112022 Yes, it's a bug in server side. by the way I just noticed that in the newest release, @chyezh has forbidden enable tls when enable restful & grpc in the same port.

e34fa04#diff-6e86fc33d8695678bc197f60ce811f0a362cb3034cda0cc03823b5bc8822920eR75

I think we should fix this instead of forbidding the usage, in a way similar to this: https://ahmet.im/blog/grpc-http-mux-go/

Yes, you are correct

@yanliang567 yanliang567 modified the milestones: 2.4.13, 2.4.14 Oct 15, 2024
@yanliang567 yanliang567 modified the milestones: 2.4.14, 2.4.16 Nov 14, 2024
@yanliang567 yanliang567 modified the milestones: 2.4.16, 2.4.17, 2.4.18 Nov 21, 2024
@nish112022
Copy link
Contributor Author

@haorenfsa So, I tried some approaches and there are essentially 2.

  1. There is a property called as NextProtos in here, where in if I specify this particular order.It uses http1 for the rest endpoint and h2 for the grpc because of the order here.
    tlsConfig := &tls.Config{
    Certificates: []tls.Certificate{cert},
    NextProtos: []string{"http/1.1", "h2"}, // Enable HTTP/2 and HTTP/1.1
    }

    The issue with this approach is that it relies on differentiating requests based on the protocol (http/1.1 vs h2). If HTTP/1.1 is deprecated in the future, this logic may fail, as both gRPC and HTTP endpoints would need to share HTTP/2 (h2)

  2. I tried the approach from the link you provided.I can separate the grpc and http requests even when both use h2 with tls. I will be attaching the code below.Let me know what do you think about this.I believe this should the approach we should take here.
    multiple_split.txt

@xiaofan-luan
Copy link
Collaborator

@nish112022
feel free to submit a fix and we'd definitely like to take it if applicable

@nish112022
Copy link
Contributor Author

/assign @nish112022

@yanliang567 yanliang567 removed this from the 2.4.18 milestone Dec 24, 2024
@yanliang567 yanliang567 modified the milestones: 2.4.19, 2.4.20 Dec 24, 2024
@yanliang567 yanliang567 modified the milestones: 2.4.20, 2.4.21 Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Issues or changes related a bug triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants