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

Implementing a generic Ingress based on Lua And A/B Testing Release #86

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

zmberg
Copy link
Member

@zmberg zmberg commented Sep 22, 2022

Signed-off-by: liheng.zms liheng.zms@alibaba-inc.com

Ⅰ. Describe what this PR does

  1. Implementing a generic Ingress based on Lua to support multiple ingress provider, e.g. nginx, aliyun-mse, aliyun-alb
  2. support a/b testing release, headers or cookie

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Merging #86 (65c86a6) into master (f21c3fb) will increase coverage by 2.90%.
The diff coverage is 57.66%.

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   16.11%   19.01%   +2.90%     
==========================================
  Files          30       33       +3     
  Lines        3482     3703     +221     
==========================================
+ Hits          561      704     +143     
- Misses       2821     2874      +53     
- Partials      100      125      +25     
Flag Coverage Δ
unittests 19.01% <57.66%> (+2.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/rollout/trafficrouting.go 0.00% <0.00%> (ø)
pkg/util/meta.go 0.00% <0.00%> (ø)
...ntroller/rollout/trafficrouting/ingress/ingress.go 61.74% <61.74%> (ø)
pkg/util/luamanager/lua.go 64.58% <64.58%> (ø)
...ntroller/rollout/trafficrouting/gateway/gateway.go 53.27% <70.00%> (+8.27%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zmberg zmberg changed the title rollout support A/B Testing API Implementing a generic Ingress based on Lua And A/B Testing Release Oct 13, 2022
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch 2 times, most recently from 235cb04 to 8da90a4 Compare October 14, 2022 10:24
if f.IsDir() {
return nil
}
data, err := ioutil.ReadFile(path)
Copy link

Choose a reason for hiding this comment

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

G304: Potential file inclusion via variable


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]


import (
"io/ioutil"
"k8s.io/klog/v2"
Copy link

Choose a reason for hiding this comment

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

goimports: File is not goimports-ed


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -0,0 +1,31 @@
annotations = obj.annotations
Copy link

Choose a reason for hiding this comment

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

💬 5 similar findings have been found in this PR


W111: setting non-standard global variable 'annotations'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 17
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 1
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 17
lua_configuration/trafficrouting_ingress/nginx.lua 1
lua_configuration/trafficrouting_ingress/nginx.lua 17

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Member Author

Choose a reason for hiding this comment

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

@sonatype-lift ignore

Copy link

Choose a reason for hiding this comment

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

I've recorded this as ignored for this pull request.
If you change your mind, just comment @sonatype-lift unignore.

@@ -0,0 +1,31 @@
annotations = obj.annotations
annotations["alb.ingress.kubernetes.io/canary"] = "true"
Copy link

Choose a reason for hiding this comment

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

💬 32 similar findings have been found in this PR


W112: mutating non-standard global variable 'annotations'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 3
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 5
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 6
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 7
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 10
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 20
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 22
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 25
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 27

Showing 10 of 32 findings. Visit the Lift Web Console to see all.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Member Author

Choose a reason for hiding this comment

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

@sonatype-lift ignore

Copy link

Choose a reason for hiding this comment

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

I've recorded this as ignored for this pull request.
If you change your mind, just comment @sonatype-lift unignore.

@@ -0,0 +1,31 @@
annotations = obj.annotations
Copy link

Choose a reason for hiding this comment

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

💬 38 similar findings have been found in this PR


W113: accessing undefined variable 'obj'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 8
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 10
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 12
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 14
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 16
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 18
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 20
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 22
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 23
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 25

Showing 10 of 38 findings. Visit the Lift Web Console to see all.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Member Author

Choose a reason for hiding this comment

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

@sonatype-lift ignore

Copy link

Choose a reason for hiding this comment

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

I've recorded this as ignored for this pull request.
If you change your mind, just comment @sonatype-lift unignore.

@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from 8da90a4 to 73ffa55 Compare October 17, 2022 11:24
return nil
}
var data []byte
data, err = ioutil.ReadFile(path)
Copy link

Choose a reason for hiding this comment

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

G304: Potential file inclusion via variable


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]


func init() {
luaConfigurationList = map[string]string{}
filepath.Walk("./lua_configuration", func(path string, f os.FileInfo, err error) error {
Copy link

Choose a reason for hiding this comment

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

SA4009: argument err is overwritten before first use


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from 73ffa55 to 2205cb7 Compare October 18, 2022 08:12
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from 2205cb7 to 7016e0a Compare October 26, 2022 05:55
@@ -0,0 +1,35 @@
annotations = obj.annotations
Copy link

Choose a reason for hiding this comment

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

💬 8 similar findings have been found in this PR


W111: setting non-standard global variable 'annotations'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 21
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 1
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 4
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 21
lua_configuration/trafficrouting_ingress/nginx.lua 1
lua_configuration/trafficrouting_ingress/nginx.lua 4
lua_configuration/trafficrouting_ingress/nginx.lua 21

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

then
annotations = {}
end
annotations["alb.ingress.kubernetes.io/canary"] = "true"
Copy link

Choose a reason for hiding this comment

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

💬 32 similar findings have been found in this PR


W112: mutating non-standard global variable 'annotations'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 7
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 8
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 9
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 10
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 11
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 14
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 24
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 26
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 29
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 31

Showing 10 of 32 findings. Visit the Lift Web Console to see all.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -0,0 +1,35 @@
annotations = obj.annotations
Copy link

Choose a reason for hiding this comment

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

💬 41 similar findings have been found in this PR


W113: accessing undefined variable 'obj'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 2
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 12
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 14
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 16
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 18
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 20
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 22
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 24
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 26
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 27

Showing 10 of 41 findings. Visit the Lift Web Console to see all.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@zmberg zmberg force-pushed the optimize_traffic_scheduling branch 2 times, most recently from c8df109 to d0c7c6d Compare November 8, 2022 07:34
"encoding/json"
"errors"

"github.com/yuin/gopher-lua"
Copy link

Choose a reason for hiding this comment

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

goimports: File is not goimports-ed


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@zmberg zmberg force-pushed the optimize_traffic_scheduling branch 3 times, most recently from a86836d to d1b14f9 Compare November 9, 2022 06:37
@zmberg zmberg closed this Nov 9, 2022
@zmberg zmberg reopened this Nov 9, 2022
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch 2 times, most recently from f309156 to be0c76c Compare November 10, 2022 05:58
@@ -0,0 +1,35 @@
annotations = {}
Copy link

Choose a reason for hiding this comment

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

💬 5 similar findings have been found in this PR


W111: setting non-standard global variable 'annotations'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 1
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 4
lua_configuration/trafficrouting_ingress/nginx.lua 1
lua_configuration/trafficrouting_ingress/nginx.lua 4

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@@ -0,0 +1,35 @@
annotations = {}
if ( obj.annotations )
Copy link

Choose a reason for hiding this comment

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

💬 5 similar findings have been found in this PR


W113: accessing undefined variable 'obj'


🔎 Expand here to view all instances of this finding
File Path Line Number
lua_configuration/trafficrouting_ingress/aliyun-alb.lua 4
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 2
lua_configuration/trafficrouting_ingress/aliyun-mse.lua 4
lua_configuration/trafficrouting_ingress/nginx.lua 2
lua_configuration/trafficrouting_ingress/nginx.lua 4

Visit the Lift Web Console to find more details in your report.


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from be0c76c to b8531f8 Compare November 10, 2022 07:26
@@ -0,0 +1,5 @@
# Ignore results from vendor directories
Copy link

Choose a reason for hiding this comment

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

Lift Configuration Added: Key: ignoreFiles

New value: vendor/
lua_configuration/


ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from 1ff521d to 015ab3f Compare November 17, 2022 09:22
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch 2 times, most recently from 9a4e9aa to 67714bd Compare November 21, 2022 06:53
@@ -141,7 +149,8 @@ type TrafficRouting struct {

// IngressTrafficRouting configuration for ingress controller to control traffic routing
type IngressTrafficRouting struct {
// ClassType refers to the class type of an `Ingress`, e.g. Nginx. Default is Nginx
// ClassType refers to the type of `Ingress`.
// current support nginx, aliyun-alb, aliyun-mse. default is nginx.
Copy link
Member

Choose a reason for hiding this comment

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

aliyun-mse is not in the lua_configuration

return false, nil
}
// First, set canary route 0 weight.
newAnnotations, err := r.executeLuaForCanary(canaryIngress.Annotations, utilpointer.Int32(0), nil)
Copy link
Member

Choose a reason for hiding this comment

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

plz make sure that we use zero weight or -1 weight

Copy link
Member Author

@zmberg zmberg Nov 22, 2022

Choose a reason for hiding this comment

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

Here is setting weight=0, because I want to set zero traffic to canary service first. And second delete the canary ingress.

func (r *gatewayController) buildDesiredHTTPRoute(rules []gatewayv1alpha2.HTTPRouteRule, weight *int32, matches []rolloutv1alpha1.HttpRouteMatch) []gatewayv1alpha2.HTTPRouteRule {
var desired []gatewayv1alpha2.HTTPRouteRule
// Only when finalize method parameter weight=-1, then we need to remove the canary route policy and restore to the original configuration
if weight != nil && *weight == -1 {
Copy link
Member

Choose a reason for hiding this comment

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

can we replace hard coded special value -1 with nil ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot, because step.weight=nil is valid parameter. weight=-1 specifically refers to the need to delete the canary configuration.

end
for _,match in pairs(obj.matches)
do
if ( not (match or match.headers) )
Copy link
Member

Choose a reason for hiding this comment

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

plz include a test that use json in the lua script

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>
@zmberg zmberg force-pushed the optimize_traffic_scheduling branch from 67714bd to aab4903 Compare November 22, 2022 02:27
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@furykerry
Copy link
Member

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 0c54037 into openkruise:master Nov 22, 2022
@zmberg zmberg deleted the optimize_traffic_scheduling branch December 20, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants