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

Added response header for traffic splitting feature #401

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

nastassia-dailidava
Copy link
Contributor

@nastassia-dailidava nastassia-dailidava commented Dec 8, 2023

closes allegro-internal/flex-roadmap#476

matb4r
matb4r previously approved these changes Dec 8, 2023
@@ -157,6 +157,7 @@ class CanaryProperties {

class TrafficSplittingProperties {
var zoneName = ""
var headerName = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add some default value here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this empty value just to have an option not to add this header, and it can be configured as default in application.yaml for example?

@@ -359,7 +359,8 @@ class EnvoyEgressRoutesFactory(
.withClusterWeight(routeSpec.clusterName, routeSpec.clusterWeights.main)
.withClusterWeight(
getAggregateClusterName(routeSpec.clusterName, properties),
routeSpec.clusterWeights.secondary
routeSpec.clusterWeights.secondary,
routeSpec.clusterName
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to pass any value to this header? How this header will be named? Maybe there should be a only boolean value which indicates if request was forwarded to secondary cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the header name is configured by this property headerName in SnpashotProperty, and the value I set as a cluster just so it can be possible to filter by it

Copy link
Contributor

@Ferdudas97 Ferdudas97 Dec 12, 2023

Choose a reason for hiding this comment

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

but it will be in <HeaderName>: <service-name> format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I add it the same way as all other headers are added: this.addResponseHeadersToAdd(buildHeader(key, value)) so I guess it will be in a correct format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I build HeaderValueOption which is envoy structure

Copy link
Contributor

Choose a reason for hiding this comment

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

so what is a purpose of this header? to know if the request was splited to secondary cluster? if that is true, imo the value should be a boolean value or something different than service-name.

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe for not splited traffic this values should be false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk I changed to boolean, but what would be a purpose of this tag with false value?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm :D

@nastassia-dailidava
Copy link
Contributor Author

@Ferdudas97 up 🌝

@nastassia-dailidava nastassia-dailidava merged commit 977567b into master Dec 12, 2023
6 checks passed
@nastassia-dailidava nastassia-dailidava deleted the flx-476 branch December 12, 2023 14:00
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.

4 participants