Skip to content

feat: add ownedBy and createdAt fields to AIGatewayRoute#620

Merged
mathetake merged 2 commits intoenvoyproxy:mainfrom
kerthcet:feat/add-owner
May 28, 2025
Merged

feat: add ownedBy and createdAt fields to AIGatewayRoute#620
mathetake merged 2 commits intoenvoyproxy:mainfrom
kerthcet:feat/add-owner

Conversation

@kerthcet
Copy link
Copy Markdown
Contributor

@kerthcet kerthcet commented May 18, 2025 •
edited
Loading

Commit Message

This commit added new fields CreatedAt & OwnedBy to AIGatewayRoute as well as the internal Backend, so people can customize the output of OpenAI-compatible APIs.

We set the fields at AIGatewayRouteRule level, the main two reasons here:

  • I believe for most of the cases, we only match one model at the Matches fields, so working at AIGatewayRouteRule seems not bad as the model:modelOwner = 1:1
  • for different backends, the modelOwner:backends = 1:n, because backends just means different route paths for one model, so working at AIGatewayRouteRule level also makes sense.

One shortcoming here is in fallover mode, like we set different backends for one model service, e.g. Envoy AI Gateway + openai, then we will have the same modelOwner for them, it's a bit confusing.

But I persist on this just because when querying the /models list, let's assume that we'll have two models sharing the same modelOwner (different with the current implementation):

  • even the enduser gets two models, but actually they can not select which backend to query, the only knob for them is the model name
  • the fallover backend, the openai here may never be queried, as a result of this, return it to the user seems useless

Related Issues/PRs (if applicable)

Fixes: #588

Special notes for reviewers (if applicable)

None

@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 18, 2025 •
edited
Loading

API definitions only now. cc @mathetake for feedbacks.

@kerthcet kerthcet changed the title Add ownedBy and createdAt fields to AIGatewayRouteRuleBackendRef feat: Add ownedBy and createdAt fields to AIGatewayRouteRuleBackendRef May 18, 2025
@mathetake
Copy link
Copy Markdown
Member

mathetake commented May 21, 2025

sorry i've been sick & I needed to focus on the important refactoring in #629 ... will check in tomorrow

mathetake
mathetake reviewed May 21, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

i didn't give much thought but am i understanding correctly that this assumes that "model maps to a unique backend? my concern is mainly about this comment #588 (comment)

@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 21, 2025 •
edited
Loading

i didn't give much thought but am i understanding correctly that this assumes that "model maps to a unique backend? my concern is mainly about this comment #588 (comment)

Yes, for different backends, like the platform and OpenAI, the owner may differ. Since the Owner works at backend level, for fallback examples, it still works as expected, what we do is just ad different owner for each backend, for example:

 backendRefs: 
   - name: provider-fallback-always-failing-upstream  # This is the primary backend and trying to speak TLS, which always fails. 
     # ownedBy: # default to Envoy AI Gateway if not specified
   - name: provider-fallback-aws 
     ownedBy: "aws"

RE:

For fallback case, what I think a bit tricky here is when querying the model list, we have no idea whether the services(backends) is ready or not, which means we have no idea who's exactly the model owner among all the backends (the same for all kinds of multi-backends, like weight based routing). So mapping the modelOwner:backend = 1:1 seems useless, I may think more about this.

@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 21, 2025

I think there're three approaches:

  • header level: this will match with the owner one by one, but it requires to extend the AIGatewayRouteRuleMatch.Headers to include the Owner field but seems a little weird
  • backend level: as we implemented in this PR, however, we may have same model with different owners in /models request. But actually there's no difference between them when querying the chat api since we only need the model name.
  • rule level: we'll add these fields to AIGatewayRouteRule and different backends will share the same Owner, not a perfect idea but seems not bad.

@kerthcet kerthcet mentioned this pull request May 22, 2025
Merged
mathetake pushed a commit that referenced this pull request May 22, 2025
@kerthcet
feat: move Backends back to RouteRule (#633)
762be89 
**Commit Message**

The backends and headers in filter config are M:N, with
#599, we swapped out the
backend from the config.rules, leading to the lost mapping relationship
between them. With this PR, we'll move the backends back to the
config.rules which is more straightforward.

**Related Issues/PRs (if applicable)**


Related PR: #620,
#599

**Special notes for reviewers (if applicable)**

None

---------

Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet kerthcet changed the title feat: Add ownedBy and createdAt fields to AIGatewayRouteRuleBackendRef feat: add ownedBy and createdAt fields to AIGatewayRoute May 23, 2025
@kerthcet kerthcet force-pushed the feat/add-owner branch 3 times, most recently from 96ca665 to c3ecedd Compare May 23, 2025 10:58
kerthcet
kerthcet commented May 23, 2025
View reviewed changes
internal/apischema/openai/openai.go
return err
}
*(*time.Time)(t) = time.Unix(q, 0)
*(*time.Time)(t) = time.Unix(q, 0).UTC()
Copy link
Copy Markdown
Contributor Author

@kerthcet kerthcet May 23, 2025

Choose a reason for hiding this comment

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

Change to UTC to make sure the output is comparable in tests.

@kerthcet kerthcet marked this pull request as ready for review May 23, 2025 11:02
@kerthcet kerthcet requested a review from a team as a code owner May 23, 2025 11:02
@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 23, 2025

Ping @mathetake for review, thanks a lot.

mathetake
mathetake reviewed May 23, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

minor comments, but I think this looks good.

any further comments ? @envoyproxy/ai-gateway-maintainers

@kerthcet
Add OwnedBy and Created to AIGatewayRoute
55636e4 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet
Update field names
a572ff4 
Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 24, 2025

Comments addressed, PTAL.

@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 28, 2025

Kindly ping.

@mathetake mathetake merged commit 36aa538 into envoyproxy:main May 28, 2025
17 checks passed
@mathetake
Copy link
Copy Markdown
Member

mathetake commented May 28, 2025

thanks for your work @kerthcet !

@kerthcet
Copy link
Copy Markdown
Contributor Author

kerthcet commented May 28, 2025

Thanks @mathetake

@mathetake mathetake mentioned this pull request May 29, 2025
Merged
mathetake added a commit that referenced this pull request May 30, 2025
@mathetake
test: refactors tests/extproc (#651)
177c414 
**Commit Message**

`fakeBackends` were utilized before #620 but after that, it's
effectively not used anymore. This refactors tests/extproc around on the
backend definition so that it will be clearer that the always-failing
backend is configured explicitly.

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@kerthcet kerthcet mentioned this pull request Jun 2, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mathetake mathetake mathetake approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Configurations for owned_by and created in /v1/models ep

2 participants

@kerthcet @mathetake