Skip to content

Supply x-envoy-downstream-service-node HTTP header to the upstream when needed#1021

Merged
RomanDzhabarov merged 4 commits into
masterfrom
canary_header_downstream
May 26, 2017
Merged

Supply x-envoy-downstream-service-node HTTP header to the upstream when needed#1021
RomanDzhabarov merged 4 commits into
masterfrom
canary_header_downstream

Conversation

@RomanDzhabarov
Copy link
Copy Markdown
Member

This works similar to the x-envoy-downstream-cluster-name and can be used for various purposes by upstream.

Conflicts:
	source/common/http/headers.h
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

@RomanDzhabarov The implementation LGTM, but I'm curious why you went for a fixed x-envoy-downstream-canary header instead of x-envoy-downstream-service-node, i.e. it seems this loses information. Is there some internal upstream expectation that treats this header specially?

Out of the scope of this PR, I was thinking this might be another use for #1016 if we had %SERVICE_NODE% as a header variable.

@RomanDzhabarov
Copy link
Copy Markdown
Member Author

@htuch The use case we have in mind is to have fault filters on the upstream ingress and do fault injections only for canary downstream nodes. I need to detect whether downstream is canary or not (so I wanted to be explicit).
x-envoy-downstream-service-node certainly covers this scenario, but also we have XFF which can help to identify the downstream IP etc (if we care about specific node id).

I can proceed, either way, let me know.

@htuch
Copy link
Copy Markdown
Member

htuch commented May 26, 2017

I'm wondering whether using a fixed canary value for node ID here is overloading the node name concept. It seems that service-node is used to identify a node uniquely for the purposes of API fetch (e.g. CDS, RDS). Will there always be a single canary node or might there be canary-0, canary-1, etc?

@RomanDzhabarov
Copy link
Copy Markdown
Member Author

I'd vote that canary is not just a single node, we should not base our assumption on having just one canary.

Here is what i think would be better to satisfy our needs.
Pass service-node and then on the ingress side make sure that the right expectations are set wrt to canaries.

I'll do a change shortly.

@RomanDzhabarov RomanDzhabarov changed the title Supply x-envoy-downstream-canary HTTP header to the upstream when needed Supply x-envoy-downstream-service-node HTTP header to the upstream when needed May 26, 2017
htuch
htuch previously approved these changes May 26, 2017
@RomanDzhabarov RomanDzhabarov merged commit 8d72607 into master May 26, 2017
@RomanDzhabarov RomanDzhabarov deleted the canary_header_downstream branch May 30, 2017 19:19
jpsim pushed a commit that referenced this pull request Nov 28, 2022
…#1021)

Description: this PR attempts to fix two related crashes. By hooking up to iOS's notification system we attempt to terminate the engine earlier, and allow all objects necessary to be available fixing #1035. Potentially fixes #831.
Risk Level: med - changes termination sequence
Testing: added test

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
…#1021)

Description: this PR attempts to fix two related crashes. By hooking up to iOS's notification system we attempt to terminate the engine earlier, and allow all objects necessary to be available fixing #1035. Potentially fixes #831.
Risk Level: med - changes termination sequence
Testing: added test

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
mathetake added a commit that referenced this pull request Mar 3, 2026
**Description**

There was a bug in the translation of OpenAI->OpenAI layer where the
body mutation is broken when forceBodyMutation = true. forceBodyMutation
becomes true when stream_options is not present in the original body and
we lacked the test case for it.

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

Closes #1017

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
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.

2 participants