-
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-214603 Changes for accepting Gateway with some valid and some invalid listener #1550
Conversation
118cf9f
to
27b4a10
Compare
7b5d94f
to
aa1cb49
Compare
aa1cb49
to
1d1b3d2
Compare
9d9fee0
to
b05f9ba
Compare
Ut run result:
|
|
||
gwStatus := akogatewayapiobjects.GatewayApiLister().GetGatewayToGatewayStatusMapping(gateway.Namespace + "/" + gateway.Name) | ||
for i, listener := range gateway.Spec.Listeners { | ||
if gwStatus.Listeners[i].Conditions[0].Type == string(gatewayv1.ListenerConditionAccepted) && gwStatus.Listeners[i].Conditions[0].Status == "False" { |
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.
we should check if len(gwStatus.Listeners) >= i before this to avoid out of index error. Also consider moving this check in a function since this is called in multiple places.
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.
During ingestion layer, while validating each and every listener, its status update is enqueued to Status layer queue. If that status update takes time or didn;t go through, we might end up processing invalid listeners.
Should we introduce some retry if status for that listener is not there? or should we take out dependency on status update and introduce some internal structure to manage valid and invalid listeners?
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.
We have already introduced an internal structure, GetGatewayToGatewayStatusMapping function is taking value from that map
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.
Did len(gwStatus.Listeners) >= i change
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: In DequeueIngestion, we have logic
'''
if !IsHTTPRouteValid(key, httpRoute) {
return
}
'''
If route changes from valid to partial invalid, how are we deleting existing objects?
return | ||
} | ||
listRoutes, err := validateReferredHTTPRoute(key, allowedRoutesAll, gw) | ||
if err != nil { | ||
utils.AviLog.Errorf("Validation of Referred HTTPRoutes Failed due to error : %s", err.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.
Nit: Failed
--> failed
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
ako-gateway-api/k8s/validator.go
Outdated
@@ -242,8 +250,8 @@ func isValidListener(key string, gateway *gatewayv1.Gateway, gatewayStatus *gate | |||
} | |||
name := string(certRef.Name) | |||
cs := utils.GetInformers().ClientSet | |||
secretObj, err := cs.CoreV1().Secrets(gateway.ObjectMeta.Namespace).Get(context.TODO(), name, metav1.GetOptions{}) | |||
if err != nil || secretObj == nil { | |||
_, err := cs.CoreV1().Secrets(gateway.ObjectMeta.Namespace).Get(context.TODO(), name, metav1.GetOptions{}) |
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.
we can avoid cs
variable creation
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
|
||
gwStatus := akogatewayapiobjects.GatewayApiLister().GetGatewayToGatewayStatusMapping(gateway.Namespace + "/" + gateway.Name) | ||
for i, listener := range gateway.Spec.Listeners { | ||
if gwStatus.Listeners[i].Conditions[0].Type == string(gatewayv1.ListenerConditionAccepted) && gwStatus.Listeners[i].Conditions[0].Status == "False" { |
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.
During ingestion layer, while validating each and every listener, its status update is enqueued to Status layer queue. If that status update takes time or didn;t go through, we might end up processing invalid listeners.
Should we introduce some retry if status for that listener is not there? or should we take out dependency on status update and introduce some internal structure to manage valid and invalid listeners?
} | ||
|
||
func TestMultipleHttpRoutesWithValidAndInvalidGatewayListeners(t *testing.T) { | ||
gatewayName := "gateway-hr-09" |
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.
Use incremental naming convention
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
}, 25*time.Second).Should(gomega.Equal(true)) | ||
|
||
integrationtest.CreateSVC(t, DEFAULT_NAMESPACE, svcName, corev1.ProtocolTCP, corev1.ServiceTypeClusterIP, false) | ||
integrationtest.CreateEP(t, DEFAULT_NAMESPACE, svcName, false, false, "1.2.3") |
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.
Use CreateEPorEPS and DelEPorEPS for rest of the unit tests cases
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
} | ||
nodes := aviModel.(*avinodes.AviObjectGraph).GetAviEvhVS() | ||
return len(nodes[0].EvhNodes) | ||
}, 50*time.Second).Should(gomega.Equal(2)) |
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.
Use retry timeout everywhere
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
akogatewayapitests.UpdateGateway(t, gatewayName, DEFAULT_NAMESPACE, gatewayClassName, nil, listeners) | ||
|
||
g.Eventually(func() bool { | ||
gateway, err := akogatewayapitests.GatewayClient.GatewayV1().Gateways(DEFAULT_NAMESPACE).Get(context.TODO(), gatewayName, metav1.GetOptions{}) |
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.
we are using informer client in the code, shouldnt we be using the informer client here as well? The informer may be out of date but these cases will pass because kubernetes client is updated
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.
As discussed, it will be taken up later
//apimeta.FindStatusCondition(gateway.Status.Conditions, string(gatewayv1.GatewayConditionAccepted)) != nil | ||
}, 30*time.Second).Should(gomega.Equal(true)) | ||
|
||
gateway, _ := akogatewayapitests.GatewayClient.GatewayV1().Gateways(DEFAULT_NAMESPACE).Get(context.TODO(), gatewayName, metav1.GetOptions{}) |
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.
same 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.
As discussed, it will be taken up later
} | ||
return len(nodes[0].EvhNodes[1].VHMatches) | ||
}, 30*time.Second).Should(gomega.Equal(2)) | ||
time.Sleep(20 * time.Second) |
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 are we using constant sleep 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.
Done
|
||
hostnames := []gatewayv1.Hostname{"foo-8080.com", "foo-8081.com"} | ||
akogatewayapitests.SetupHTTPRoute(t, httpRouteName, DEFAULT_NAMESPACE, parentRefs, hostnames, rules) | ||
time.Sleep(10 * time.Second) |
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.
use eventually case instead of constant sleep
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.
It cannot be replaced as this one, will not lead to any object being created which can be checked in eventually block
listeners[1].Protocol = "HTTPS" | ||
akogatewayapitests.UpdateGateway(t, gatewayName, DEFAULT_NAMESPACE, gatewayClassName, nil, listeners) | ||
g.Eventually(func() bool { | ||
gateway, err := akogatewayapitests.GatewayClient.GatewayV1().Gateways(DEFAULT_NAMESPACE).Get(context.TODO(), gatewayName, metav1.GetOptions{}) |
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.
use informer client since gateway update may update the etcd but not the informer cache
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.
As discussed, it will be taken up later
parentRefs = akogatewayapitests.GetParentReferencesV1([]string{gatewayName}, DEFAULT_NAMESPACE, []int32{ports[1]}) | ||
akogatewayapitests.SetupHTTPRoute(t, httpRoute2Name, DEFAULT_NAMESPACE, parentRefs, hostnames, rules) | ||
listeners[0].Protocol = "HTTPS" | ||
time.Sleep(10 * time.Second) |
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.
avoid using time.Sleep and instead use eventually case
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.
It cannot be replaced as this one, will not lead to any object being created which can be checked in eventually block
conditionMap := make(map[string][]metav1.Condition) | ||
|
||
for _, port := range ports { | ||
conditions := make([]metav1.Condition, 0, 1) |
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.
instead of make and append this can be refactored to
conditions := []metav1.Condition{ {Type: ..., Reason...} }
conditionMap[fmt.Sprintf("%s-%d", gatewayName, port)] = 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
@akshayhavile , it will return only if HTTPRoute transitions from valid/partially valid to completely invalid. We were not doing anything for this even before. Existing object deletion will happen upon reboot |
4ad5046
to
b967b9b
Compare
Ut run results:
|
build ako |
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 : Do we have test case which creates httproute first and then gateway?
@@ -75,10 +76,20 @@ func (o *AviObjectGraph) BuildGatewayParent(gateway *gatewayv1.Gateway, key stri | |||
return parentVsNode | |||
} | |||
|
|||
func IsListenerInvalid(gwStatus *gatewayv1.GatewayStatus, listenerIndex int) bool { | |||
if len(gwStatus.Listeners) >= int(listenerIndex) && gwStatus.Listeners[listenerIndex].Conditions[0].Type == string(gatewayv1.ListenerConditionAccepted) && gwStatus.Listeners[listenerIndex].Conditions[0].Status == "False" { |
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.
condition should be >
instead of >=
to avoid out of bound error. From caller, index will always be in range but still we should change in case if it gets reused somewhere.
8c52b6b
to
c96eb45
Compare
c96eb45
to
edd9cff
Compare
Fresh Ut run results:
|
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.
LGTM
This PR contains following changes:
Testing status:
Unit Test Cases Added and updated older ones to support this
Manually tested this PR end to end.
Following Transition cases are working as expected without reboot:
Following Transition cases are working with reboot:
TODO:
Run FTs