-
Notifications
You must be signed in to change notification settings - Fork 57
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
AV-206859 Setting ResolvedRef Condition in HTTProute #1558
base: master
Are you sure you want to change the base?
Conversation
Ut run result:
|
1ab68e5
to
7283ee5
Compare
_, namespace, name := lib.ExtractTypeNameNamespace(routeTypeNsName) | ||
httpRoute, err := akogatewayapilib.AKOControlConfig().GatewayApiInformers().HTTPRouteInformer.Lister().HTTPRoutes(namespace).Get(name) | ||
if err != nil { | ||
utils.AviLog.Debugf("key: %s, msg: Unable to extract the HTTPRoute object %s for BackendRef validation", key, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Warn or Error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
7283ee5
to
7e51bfe
Compare
@@ -295,6 +296,9 @@ func (hr *httpRoute) ParseRouteConfig() *RouteConfig { | |||
//Default 0 | |||
backend.Port = int32(*ruleBackend.Port) | |||
} | |||
if ruleBackend.BackendRef.Kind != nil { | |||
backend.Kind = string(*ruleBackend.Kind) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we by default add Services?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not add anything for Kind, as in if we keep kind as "", by default Service gets added as kind
map[string][]string{"RequestHeaderModifier": {"add", "remove", "replace"}}, | ||
[][]string{{"avisvc", "default", "8080", "1"}}, nil) | ||
kind := gatewayv1.Kind("InvalidKind") | ||
rule.BackendRefs[0].BackendRef.Kind = &kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have additional unit test with two backends, one invalid and one valid and then check for Conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
7e51bfe
to
ae7bd8b
Compare
for _, httpbackend := range rule.Backends { | ||
isValidBackend, resolvedRefConditionforBackend := validateBackendReference(key, *httpbackend.Backend, httpRoute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should include this logic as part of ParseRouteConfig
instead of writing down over here. Advantage: We will separate of parsing (and setting of httproute object) from logic of populating AviController objects. Also we will save one Informer call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -764,8 +764,8 @@ func TestHTTPRouteBackendServiceCDC(t *testing.T) { | |||
return 0 | |||
} | |||
nodes := aviModel.(*avinodes.AviObjectGraph).GetAviEvhVS() | |||
return len(nodes[0].EvhNodes[0].PoolGroupRefs[0].Members) | |||
}, 25*time.Second).Should(gomega.Equal(1)) | |||
return len(nodes[0].EvhNodes[0].PoolGroupRefs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have .Members condition also (Which is removed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
5364ef9
to
2021ef1
Compare
Ut run result:
|
ako-gateway-api/nodes/route_model.go
Outdated
routeConfigRule.Backends = append(routeConfigRule.Backends, httpBackend) | ||
isValidBackend, resolvedRefConditionforBackend := validateBackendReference(key, *backend, hr) | ||
if isValidBackend { | ||
httpBackend.IsValid = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkoshtavmware: We do not require IsValid
field as we are appending valid Backends only to routeConfigRule.Backends
which is being parsed in Graph layer function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
ako-gateway-api/nodes/route_model.go
Outdated
} else { | ||
hasInvalidBackend = true | ||
resolvedRefCondition = resolvedRefConditionforBackend | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why continue is required over here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
build ako |
2021ef1
to
e20fd14
Compare
Unit Test run:
|
e20fd14
to
102e1ed
Compare
AV-206859 Setting ResolvedRef Condition in HTTProute
Testing Status:
Manual Testing done
Unit TestCase added