Update docs for ai.proto #10358
Update docs for ai.proto #10358
Conversation
...erence/api/github.com/solo-io/gloo/projects/gloo/api/v1/enterprise/options/ai/ai.proto.sk.md
Outdated
Show resolved
Hide resolved
…oo/api/v1/enterprise/options/ai/ai.proto.sk.md Co-authored-by: Nadine Spies <[email protected]>
|
Issues linked to changelog: |
|
Visit the preview URL for this PR (updated for commit 6bb807b): https://gloo-edge--pr10358-rlg-ai-proto-fjg0h6cb.web.app (expires Wed, 04 Dec 2024 17:07:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
...erence/api/github.com/solo-io/gloo/projects/gloo/api/v1/enterprise/options/ai/ai.proto.sk.md
Outdated
Show resolved
Hide resolved
| // Auth Token to use for the Azure OpenAI API | ||
| // This token will be placed into the `api-key` header | ||
| // The authorization token that the AI gateway uses to access the Azure OpenAI API. | ||
| // This token is automatically sent in the `api-key` header of the request. | ||
| oneof auth_token_source { |
There was a problem hiding this comment.
So, this is not a documentation issue but I just notice this is odd. Inside SingleAuthToken there is already a oneof auth_token_source. Here unlike the OpenAI (which I think is correct) has the oneof outside and then the oneof inside? @EItanya @npolshakova is this right?
Same for the others. If this is correct, should we change the OpenAI to match this so it's consistent?
There was a problem hiding this comment.
To be completely frank I don't remember my exact intention here. I vaguely remember wanting to separate situations where we fetched a token for the user, vs allowing them to provide one, but I don't seem to have followed through there with the Passthrough field, so it may be worth revisiting this decision
| message OpenAI { | ||
| oneof auth_token_source { | ||
| // The authorization token that the AI gateway uses to access the OpenAI API. | ||
| // This token is automatically sent in the `Authorization` header of the | ||
| // request and prefixed with `Bearer`. | ||
| SingleAuthToken auth_token = 1; | ||
| // re-use the token from the backend | ||
| // google.protobuf.Empty inherit_backend_token = 3; | ||
| } |
There was a problem hiding this comment.
@EItanya do we need a way to specify the model here?
There was a problem hiding this comment.
No, this is embedding, currently the model is always the same. It may become useful if we expose custom_host here so that the user can use different endpoints or models which support the OpenAI API
| uint32 grpc_port = 3; | ||
| // Whether or not to use a secure connection, true by default | ||
| // Whether to use a secure connection. Defaults to `true`. |
There was a problem hiding this comment.
This is a bit confusing and a bit against the convention where the default is always 0, "" or false. In the code, I also do not find where we set this to true by default (maybe I missed it). Maybe the original comment means secure connection by default? @EItanya can you confirm?
Also, it looks like this setting applies to both the http and grpc connections.
There was a problem hiding this comment.
Correct, it is the comment here that's confusing. The connection is secure by default
| message Webhook { | ||
| // Host to send the traffic to. | ||
| string host = 1; | ||
| // Port to send the traffic to | ||
| uint32 port = 2; | ||
| // Describes how to match a given string in HTTP headers. Match is case-sensitive. |
There was a problem hiding this comment.
Not a document issue. case-sensitive will never work because HTTP/1 can be camel case but HTTP/2 is always lower case. Also, envoy lower case the header before sending to ext_proc, so the user really need to enter all lower case here for it to work. So, case-sensitive match is incorrect. HTTP header name should be case-insensitive. Should we open an issue to fix this, @EItanya
There was a problem hiding this comment.
Envoy is all lower case anyway, so not much for us to do there anyway IMO
Co-authored-by: Andy Fong <[email protected]>
Updates the comments in the AI gateway proto to clean up the api ref doc.
BOT NOTES:
resolves https://github.com/solo-io/docs/issues/624