Skip to content

Commit

Permalink
fix(controller): set creationTimestamp of ancestors of KongUpstreamPo…
Browse files Browse the repository at this point in the history 
…licy status (#6767)

* fix: set creationTimestamp of ancestors of KongUpstreamPolicy status

* address comments
  • Loading branch information
@randmonkey
randmonkey authored Dec 4, 2024
1 parent d9241d8 commit e513e04
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 16 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ Adding a new version? You'll need three changes:
point to a `Gateway` as they can point to other kinds of resources like
`Service` when used in service mesh solutions.
[#6692](https://github.com/Kong/kubernetes-ingress-controller/pull/6692)
- Set `creationTimestamp` of ancestor in ancestor status of `KongUpstreamPolicy`
to make sure the order of ancestors in the status is deterministic to fix
the issue where the status of `KongUpstreamPolicy` is continuously updated.
[#6767](https://github.com/Kong/kubernetes-ingress-controller/pull/6767)

### Added

Expand Down
14 changes: 13 additions & 1 deletion internal/controllers/configuration/kongupstreampolicy_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ func (r *KongUpstreamPolicyReconciler) buildAncestorsStatus(
ancestorKind: upstreamPolicyAncestorKindService,
acceptedCondition: acceptedCondition,
programmedCondition: programmedCondition,
creationTimestamp: service.CreationTimestamp,
})
}
for _, serviceFacade := range serviceFacades {
Expand All @@ -322,6 +323,7 @@ func (r *KongUpstreamPolicyReconciler) buildAncestorsStatus(
ancestorKind: upstreamPolicyAncestorKindKongServiceFacade,
acceptedCondition: acceptedCondition,
programmedCondition: programmedCondition,
creationTimestamp: serviceFacade.CreationTimestamp,
})
}

Expand Down Expand Up @@ -416,8 +418,18 @@ func (r *KongUpstreamPolicyReconciler) buildPolicyStatus(
ancestorsStatus []ancestorStatus,
) (gatewayapi.PolicyStatus, error) {
// Sort the ancestors by creation timestamp and keep only the oldest ones.
// If multiple ancestors have the same creation timestamp, sort by kind, namespace and name.
sort.Slice(ancestorsStatus, func(i, j int) bool {
return ancestorsStatus[i].creationTimestamp.Before(&ancestorsStatus[j].creationTimestamp)
if !ancestorsStatus[i].creationTimestamp.Equal(&ancestorsStatus[j].creationTimestamp) {
return ancestorsStatus[i].creationTimestamp.Before(&ancestorsStatus[j].creationTimestamp)
}
if ancestorsStatus[i].ancestorKind != ancestorsStatus[j].ancestorKind {
return ancestorsStatus[i].ancestorKind < ancestorsStatus[j].ancestorKind
}
if ancestorsStatus[i].namespacedName.Namespace != ancestorsStatus[j].namespacedName.Namespace {
return ancestorsStatus[i].namespacedName.Namespace < ancestorsStatus[j].namespacedName.Namespace
}
return ancestorsStatus[i].namespacedName.Name < ancestorsStatus[j].namespacedName.Name
})
if len(ancestorsStatus) > maxNAncestors {
r.Log.Info("status has more ancestors than the Gateway API permits, the newest ones will be ignored",
Expand Down
88 changes: 73 additions & 15 deletions internal/controllers/configuration/kongupstreampolicy_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
testNamespace = "test"
)

now := metav1.Now()

testCases := []struct {
name string
kongUpstreamPolicy kongv1beta1.KongUpstreamPolicy
Expand All @@ -44,7 +46,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
updated bool
}{
{
name: "2 services referencing the same policy, all accepted. Status update.",
name: "3 services referencing the same policy, all accepted. Status update.",
kongUpstreamPolicy: kongv1beta1.KongUpstreamPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: policyName,
Expand All @@ -59,7 +61,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&corev1.Service{
Expand All @@ -70,7 +72,19 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
Time: now.Add(10 * time.Second),
},
},
},
&corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "svc-3",
Namespace: testNamespace,
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Time{
Time: now.Add(5 * time.Second),
},
},
},
Expand All @@ -97,6 +111,13 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
},
},
},
{
BackendRef: gatewayapi.BackendRef{
BackendObjectReference: gatewayapi.BackendObjectReference{
Name: "svc-3",
},
},
},
},
},
},
Expand All @@ -106,6 +127,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
objectsConfiguredInDataPlane: true,
expectedKongUpstreamPolicyStatus: gatewayapi.PolicyStatus{
Ancestors: []gatewayapi.PolicyAncestorStatus{
// Order of ancestors should be svc-1 - svc-3 - svc-2 for the order of creationTimestamp.
{
AncestorRef: gatewayapi.ParentReference{
Group: lo.ToPtr(gatewayapi.Group("core")),
Expand All @@ -127,6 +149,27 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
},
},
},
{
AncestorRef: gatewayapi.ParentReference{
Group: lo.ToPtr(gatewayapi.Group("core")),
Kind: lo.ToPtr(gatewayapi.Kind("Service")),
Namespace: lo.ToPtr(gatewayapi.Namespace(testNamespace)),
Name: gatewayapi.ObjectName("svc-3"),
},
ControllerName: gatewaycontroller.GetControllerName(),
Conditions: []metav1.Condition{
{
Type: string(gatewayapi.PolicyConditionAccepted),
Status: metav1.ConditionTrue,
Reason: string(gatewayapi.PolicyReasonAccepted),
},
{
Type: string(gatewayapi.GatewayConditionProgrammed),
Status: metav1.ConditionTrue,
Reason: string(gatewayapi.GatewayReasonProgrammed),
},
},
},
{
AncestorRef: gatewayapi.ParentReference{
Group: lo.ToPtr(gatewayapi.Group("core")),
Expand Down Expand Up @@ -214,7 +257,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&corev1.Service{
Expand All @@ -224,9 +267,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
},
CreationTimestamp: now,
},
},
&gatewayapi.HTTPRoute{
Expand Down Expand Up @@ -323,7 +364,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&corev1.Service{
Expand All @@ -334,7 +375,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
kongv1beta1.KongUpstreamPolicyAnnotationKey: anotherPolicyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
Time: now.Add(10 * time.Second),
},
},
},
Expand Down Expand Up @@ -411,7 +452,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&corev1.Service{
Expand All @@ -422,7 +463,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
kongv1beta1.KongUpstreamPolicyAnnotationKey: anotherPolicyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
Time: now.Add(10 * time.Second),
},
},
},
Expand Down Expand Up @@ -503,7 +544,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&incubatorv1alpha1.KongServiceFacade{
Expand All @@ -514,7 +555,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
Time: now.Add(10 * time.Second),
},
},
},
Expand Down Expand Up @@ -599,7 +640,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
Annotations: map[string]string{
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Now(),
CreationTimestamp: now,
},
},
&incubatorv1alpha1.KongServiceFacade{
Expand All @@ -610,7 +651,7 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) {
kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName,
},
CreationTimestamp: metav1.Time{
Time: metav1.Now().Add(10 * time.Second),
Time: now.Add(10 * time.Second),
},
},
},
Expand Down Expand Up @@ -952,6 +993,23 @@ func TestBuildPolicyStatus(t *testing.T) {
},
},
},
{
name: "ordered by kind, namespace and name if their creationTimestamp is the same",
ancestorsStatuses: []ancestorStatus{
serviceStatus("svc-1", now),
serviceStatus("svc-2", now),
serviceFacadeStatus("svc-facade-1", now),
serviceFacadeStatus("svc-facade-2", now),
},
expected: gatewayapi.PolicyStatus{
Ancestors: []gatewayapi.PolicyAncestorStatus{
serviceFacadeExpectedPolicyAncestorStatus("svc-facade-1"),
serviceFacadeExpectedPolicyAncestorStatus("svc-facade-2"),
serviceExpectedPolicyAncestorStatus("svc-1"),
serviceExpectedPolicyAncestorStatus("svc-2"),
},
},
},
{
name: "more ancestors than allowed - keeps only maxNAncestors oldest ones",
ancestorsStatuses: func() []ancestorStatus {
Expand Down

0 comments on commit e513e04

Please sign in to comment.