Skip to content

Commit

Permalink
Add unknown condition (#654)
Browse files Browse the repository at this point in the history 
* Add unknown condition

* Adjust tests

* Adjust condition test

* Add test to verify lastTransitionTime is updated as expected

---------

Co-authored-by: Tim Riffer <[email protected]>
  • Loading branch information
@barchw @triffer
barchw and triffer authored Mar 6, 2024
1 parent 015c56a commit 32cbedb
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 10 deletions.
1 change: 1 addition & 0 deletions api/v1alpha2/conditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func ConditionFromReason(reason ReasonWithMessage) *metav1.Condition {
var conditionReasons = map[ConditionReason]conditionMeta{
ConditionReasonReconcileSucceeded: {Type: ConditionTypeReady, Status: metav1.ConditionTrue, Message: ConditionReasonReconcileSucceededMessage},
ConditionReasonReconcileFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonReconcileFailedMessage},
ConditionReasonReconcileUnknown: {Type: ConditionTypeReady, Status: metav1.ConditionUnknown, Message: ConditionReasonReconcileUnknownMessage},
ConditionReasonValidationFailed: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonValidationFailedMessage},
ConditionReasonOlderCRExists: {Type: ConditionTypeReady, Status: metav1.ConditionFalse, Message: ConditionReasonOlderCRExistsMessage},

Expand Down
2 changes: 2 additions & 0 deletions api/v1alpha2/istio_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const (
// general
ConditionReasonReconcileSucceeded ConditionReason = "ReconcileSucceeded"
ConditionReasonReconcileSucceededMessage = "Reconciliation succeeded"
ConditionReasonReconcileUnknown ConditionReason = "ReconcileUnknown"
ConditionReasonReconcileUnknownMessage = "Module readiness is unknown. Either a reconciliation is progressing, or failed previously. Check status of other conditions"
ConditionReasonReconcileFailed ConditionReason = "ReconcileFailed"
ConditionReasonReconcileFailedMessage = "Reconciliation failed"
ConditionReasonValidationFailed ConditionReason = "ValidationFailed"
Expand Down
2 changes: 2 additions & 0 deletions controllers/istio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
return ctrl.Result{}, err
}

r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileUnknown))

err := validation.ValidateAuthorizers(istioCR)
if err != nil {
return r.terminateReconciliation(ctx, &istioCR, err, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonValidationFailed))
Expand Down
103 changes: 94 additions & 9 deletions controllers/istio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,13 +330,14 @@ var _ = Describe("Istio Controller", func() {
Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(2))

Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue))

Expect((*updatedIstioCR.Status.Conditions)[1].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[1].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Status).To(Equal(metav1.ConditionTrue))

})

It("Should return an error when update status to ready failed", func() {
Expand Down Expand Up @@ -376,6 +377,7 @@ var _ = Describe("Istio Controller", func() {
Expect(statusMock.updatedToReadyCalled).Should(BeTrue())
Expect(statusMock.setConditionCalled).Should(BeTrue())
Expect(statusMock.GetConditions()).Should(Equal([]operatorv1alpha2.ReasonWithMessage{
operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileUnknown),
operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded),
operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonIngressGatewayReconcileSucceeded),
operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileSucceeded),
Expand Down Expand Up @@ -545,13 +547,14 @@ var _ = Describe("Istio Controller", func() {
Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(2))

Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue))

Expect((*updatedIstioCR.Status.Conditions)[1].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[1].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonReconcileSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartSucceeded)))
Expect((*updatedIstioCR.Status.Conditions)[1].Status).To(Equal(metav1.ConditionTrue))

})

It("Should set an error status and do not requeue an Istio CR when an older Istio CR is present", func() {
Expand Down Expand Up @@ -770,6 +773,87 @@ var _ = Describe("Istio Controller", func() {
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonValidationFailed)))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))
})

It("should update lastTransitionTime of Ready condition when reason changed", func() {
// given
istioCR := &operatorv1alpha2.Istio{
ObjectMeta: metav1.ObjectMeta{
Name: istioCrName,
Namespace: testNamespace,
Finalizers: []string{
"istios.operator.kyma-project.io/istio-installation",
},
},
}

fakeClient := createFakeClient(istioCR)

By("Mocking Istio install reconciliation to fail")
reconcilerFailingOnIstioInstall := &IstioReconciler{
Client: fakeClient,
Scheme: getTestScheme(),
istioInstallation: &istioInstallationReconciliationMock{
err: described_errors.NewDescribedError(errors.New("test error"), "test error description"),
},
proxySidecars: &proxySidecarsReconciliationMock{},
istioResources: &istioResourcesReconciliationMock{},
ingressGateway: &ingressGatewayReconciliationMock{},
log: logr.Discard(),
statusHandler: status.NewStatusHandler(fakeClient),
reconciliationInterval: testReconciliationInterval,
}

_, err := reconcilerFailingOnIstioInstall.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: istioCrName}})

Expect(err).To(HaveOccurred())

updatedIstioCR := operatorv1alpha2.Istio{}
Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)).Should(Succeed())

Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))

By("Verifying that Istio CR has Condition Ready with False")
Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonIstioInstallUninstallFailed)))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))

firstNotReadyTransitionTime := (*updatedIstioCR.Status.Conditions)[0].LastTransitionTime

By("Mocking Istio resources reconciliation to fail")
reconcilerFailingOnIstioResources := &IstioReconciler{
Client: fakeClient,
Scheme: getTestScheme(),
istioInstallation: &istioInstallationReconciliationMock{},
proxySidecars: &proxySidecarsReconciliationMock{},
istioResources: &istioResourcesReconciliationMock{
err: described_errors.NewDescribedError(errors.New("test error"), "test error description"),
},
ingressGateway: &ingressGatewayReconciliationMock{},
log: logr.Discard(),
statusHandler: status.NewStatusHandler(fakeClient),
reconciliationInterval: testReconciliationInterval,
}

// when
_, err = reconcilerFailingOnIstioResources.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: istioCrName}})

// then
Expect(err).To(HaveOccurred())
updatedIstioCR = operatorv1alpha2.Istio{}
Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR)).Should(Succeed())

Expect(updatedIstioCR.Status.Conditions).ToNot(BeNil())
Expect(*updatedIstioCR.Status.Conditions).To(HaveLen(1))

By("Verifying that the condition lastTransitionTime is also updated when only the reason has changed")
Expect((*updatedIstioCR.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeReady)))
Expect((*updatedIstioCR.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonCRsReconcileFailed)))
Expect((*updatedIstioCR.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse))

secondNotReadyTransitionTime := (*updatedIstioCR.Status.Conditions)[0].LastTransitionTime
Expect(secondNotReadyTransitionTime.Compare(firstNotReadyTransitionTime.Time) >= 0).To(BeTrue())
})
})
})

Expand All @@ -785,14 +869,15 @@ func (i *ingressGatewayReconciliationMock) Reconcile(_ context.Context) describe
}

type istioResourcesReconciliationMock struct {
err described_errors.DescribedError
}

func (i *istioResourcesReconciliationMock) AddReconcileResource(_ istio_resources.Resource) istio_resources.ResourcesReconciliation {
return i
}

func (i *istioResourcesReconciliationMock) Reconcile(_ context.Context, _ operatorv1alpha2.Istio) described_errors.DescribedError {
return nil
return i.err
}

type shouldFailClient struct {
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/features/istio/production/main-suite/istio_installation.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ Feature: Installing and uninstalling Istio module

Scenario: Installation of Istio module with default values
Given Istio CR "istio-sample" from "istio_cr_template" is applied in namespace "kyma-system"
Then Istio CR "istio-sample" in namespace "kyma-system" has status "Ready"
Then Istio CR "istio-sample" in namespace "kyma-system" has condition with reason "ReconcileUnknown" of type "Ready" and status "Unknown"
And Istio CR "istio-sample" in namespace "kyma-system" has status "Ready"
And Istio CR "istio-sample" in namespace "kyma-system" has condition with reason "ReconcileSucceeded" of type "Ready" and status "True"
And "proxy" has "requests" set to cpu - "10m" and memory - "192Mi"
And "proxy" has "limits" set to cpu - "1000m" and memory - "1024Mi"
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/steps/istio_cr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"fmt"
"github.com/pkg/errors"
"os"
"path"
"text/template"
Expand Down Expand Up @@ -70,6 +71,9 @@ func IstioCRInNamespaceHasStatusCondition(ctx context.Context, name, namespace,
if err != nil {
return err
}
if cr.Status.Conditions == nil {
return errors.New("No condition was present on Istio CR")
}
for _, condition := range *cr.Status.Conditions {
if condition.Reason == reason && condition.Type == conditionType && string(condition.Status) == status {
return nil
Expand Down

0 comments on commit 32cbedb

Please sign in to comment.