Skip to content

Commit 22d710f

Browse files
luksaluksa
committed
Check IstioCNI status for IstioRevisions that depend on IstioCNI
Previously, when an IstioRevision depended on IstioCNI, either because the user enabled the use of IstioCNI through values.pilot.cni.enabled or because the platform profile enables it, the IstioRevision resource would show as healthy even though workloads would fail because IstioCNI hasn't been deployed. This commit adds a new `DependenciesHealthy` condition to `IstioRevision` and makes `IstioRevision.status.state` and the `STATUS` column report the fact that IstioCNI is missing or unhealthy. This makes it easier for users to see that their setup is misconfigured. Signed-off-by: Marko Lukša <[email protected]>
1 parent 96ed08e commit 22d710f

File tree

9 files changed

+396
-31
lines changed

9 files changed

+396
-31
lines changed

‎api/v1/istio_types.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,23 @@ const (
234234
IstioReasonReadinessCheckFailed IstioConditionReason = "ReadinessCheckFailed"
235235
)
236236

237+
const (
238+
// IstioConditionDependenciesHealthy signifies whether the dependencies required by this Istio are healthy.
239+
// For example, an Istio with spec.values.pilot.cni.enabled=true requires the IstioCNI resource to be deployed
240+
// and ready for the Istio revision to be considered healthy. The DependenciesHealthy condition is used to indicate that
241+
// the IstioCNI resource is healthy.
242+
IstioConditionDependenciesHealthy IstioConditionType = "DependenciesHealthy"
243+
244+
// IstioReasonIstioCNINotFound indicates that the IstioCNI resource is not found.
245+
IstioReasonIstioCNINotFound IstioConditionReason = "IstioCNINotFound"
246+
247+
// IstioReasonIstioCNINotHealthy indicates that the IstioCNI resource is not healthy.
248+
IstioReasonIstioCNINotHealthy IstioConditionReason = "IstioCNINotHealthy"
249+
250+
// IstioReasonDependencyCheckFailed indicates that the status of the dependencies could not be ascertained.
251+
IstioReasonDependencyCheckFailed IstioConditionReason = "DependencyCheckFailed"
252+
)
253+
237254
const (
238255
// IstioReasonHealthy indicates that the control plane is fully reconciled and that all components are ready.
239256
IstioReasonHealthy IstioConditionReason = "Healthy"

‎api/v1/istiorevision_types.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,23 @@ const (
168168
IstioRevisionReasonUsageCheckFailed IstioRevisionConditionReason = "UsageCheckFailed"
169169
)
170170

171+
const (
172+
// IstioRevisionConditionDependenciesHealthy signifies whether the dependencies required by this IstioRevision are healthy.
173+
// For example, an IstioRevision with spec.values.pilot.cni.enabled=true requires the IstioCNI resource to be deployed
174+
// and ready for the Istio revision to be considered healthy. The DependenciesHealthy condition is used to indicate that
175+
// the IstioCNI resource is healthy.
176+
IstioRevisionConditionDependenciesHealthy IstioRevisionConditionType = "DependenciesHealthy"
177+
178+
// IstioRevisionReasonIstioCNINotFound indicates that the IstioCNI resource is not found.
179+
IstioRevisionReasonIstioCNINotFound IstioRevisionConditionReason = "IstioCNINotFound"
180+
181+
// IstioRevisionReasonIstioCNINotHealthy indicates that the IstioCNI resource is not healthy.
182+
IstioRevisionReasonIstioCNINotHealthy IstioRevisionConditionReason = "IstioCNINotHealthy"
183+
184+
// IstioRevisionDependencyCheckFailed indicates that the status of the dependencies could not be ascertained.
185+
IstioRevisionDependencyCheckFailed IstioRevisionConditionReason = "DependencyCheckFailed"
186+
)
187+
171188
const (
172189
// IstioRevisionReasonHealthy indicates that the control plane is fully reconciled and that all components are ready.
173190
IstioRevisionReasonHealthy IstioRevisionConditionReason = "Healthy"

‎controllers/istio/istio_controller.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,7 @@ func (r *Reconciler) determineStatus(ctx context.Context, istio *v1.Istio, recon
257257
} else if err == nil {
258258
status.SetCondition(convertCondition(rev.Status.GetCondition(v1.IstioRevisionConditionReconciled)))
259259
status.SetCondition(convertCondition(rev.Status.GetCondition(v1.IstioRevisionConditionReady)))
260+
status.SetCondition(convertCondition(rev.Status.GetCondition(v1.IstioRevisionConditionDependenciesHealthy)))
260261
status.State = convertConditionReason(rev.Status.State)
261262
} else {
262263
activeRevisionGetFailed := func(conditionType v1.IstioConditionType) v1.IstioCondition {
@@ -326,6 +327,8 @@ func convertConditionType(condition v1.IstioRevisionCondition) v1.IstioCondition
326327
return v1.IstioConditionReconciled
327328
case v1.IstioRevisionConditionReady:
328329
return v1.IstioConditionReady
330+
case v1.IstioRevisionConditionDependenciesHealthy:
331+
return v1.IstioConditionDependenciesHealthy
329332
default:
330333
panic(fmt.Sprintf("can't convert IstioRevisionConditionType: %s", condition.Type))
331334
}
@@ -345,6 +348,12 @@ func convertConditionReason(reason v1.IstioRevisionConditionReason) v1.IstioCond
345348
return v1.IstioReasonReconcileError
346349
case v1.IstioRevisionReasonRemoteIstiodNotReady:
347350
return v1.IstioReasonRemoteIstiodNotReady
351+
case v1.IstioRevisionReasonIstioCNINotHealthy:
352+
return v1.IstioReasonIstioCNINotHealthy
353+
case v1.IstioRevisionReasonIstioCNINotFound:
354+
return v1.IstioReasonIstioCNINotFound
355+
case v1.IstioRevisionDependencyCheckFailed:
356+
return v1.IstioReasonDependencyCheckFailed
348357
default:
349358
panic(fmt.Sprintf("can't convert IstioRevisionConditionReason: %s", reason))
350359
}

‎controllers/istio/istio_controller_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ func TestDetermineStatus(t *testing.T) {
264264
Conditions: []v1.IstioRevisionCondition{
265265
{Type: v1.IstioRevisionConditionReconciled, Status: toConditionStatus(reconciled)},
266266
{Type: v1.IstioRevisionConditionReady, Status: toConditionStatus(ready)},
267+
{Type: v1.IstioRevisionConditionDependenciesHealthy, Status: toConditionStatus(true)},
267268
{Type: v1.IstioRevisionConditionInUse, Status: toConditionStatus(inUse)},
268269
},
269270
},
@@ -329,6 +330,12 @@ func TestDetermineStatus(t *testing.T) {
329330
Reason: v1.IstioRevisionReasonHealthy,
330331
Message: "ready message",
331332
},
333+
{
334+
Type: v1.IstioRevisionConditionDependenciesHealthy,
335+
Status: metav1.ConditionTrue,
336+
Reason: v1.IstioRevisionReasonHealthy,
337+
Message: "dependencies healthy message",
338+
},
332339
},
333340
},
334341
},
@@ -375,6 +382,12 @@ func TestDetermineStatus(t *testing.T) {
375382
Reason: v1.IstioReasonHealthy,
376383
Message: "ready message",
377384
},
385+
{
386+
Type: v1.IstioConditionDependenciesHealthy,
387+
Status: metav1.ConditionTrue,
388+
Reason: v1.IstioReasonHealthy,
389+
Message: "dependencies healthy message",
390+
},
378391
},
379392
ActiveRevisionName: istioKey.Name,
380393
Revisions: v1.RevisionSummary{
@@ -407,6 +420,10 @@ func TestDetermineStatus(t *testing.T) {
407420
Type: v1.IstioConditionReady,
408421
Status: metav1.ConditionTrue,
409422
},
423+
{
424+
Type: v1.IstioConditionDependenciesHealthy,
425+
Status: metav1.ConditionTrue,
426+
},
410427
},
411428
ActiveRevisionName: istioKey.Name,
412429
Revisions: v1.RevisionSummary{
@@ -638,6 +655,11 @@ func TestUpdateStatus(t *testing.T) {
638655
Message: "ready message",
639656
LastTransitionTime: *oneMinuteAgo,
640657
},
658+
{
659+
Type: v1.IstioConditionDependenciesHealthy,
660+
Status: metav1.ConditionTrue,
661+
LastTransitionTime: *oneMinuteAgo,
662+
},
641663
},
642664
ActiveRevisionName: istioKey.Name,
643665
},
@@ -667,6 +689,11 @@ func TestUpdateStatus(t *testing.T) {
667689
Message: "ready message",
668690
LastTransitionTime: *oneMinuteAgo,
669691
},
692+
{
693+
Type: v1.IstioRevisionConditionDependenciesHealthy,
694+
Status: metav1.ConditionTrue,
695+
LastTransitionTime: *oneMinuteAgo,
696+
},
670697
},
671698
},
672699
},
@@ -687,6 +714,10 @@ func TestUpdateStatus(t *testing.T) {
687714
Reason: v1.IstioReasonHealthy,
688715
Message: "ready message",
689716
},
717+
{
718+
Type: v1.IstioConditionDependenciesHealthy,
719+
Status: metav1.ConditionTrue,
720+
},
690721
},
691722
ActiveRevisionName: istioKey.Name,
692723
},

‎controllers/istiorevision/istiorevision_controller.go

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ import (
5757
"istio.io/istio/pkg/ptr"
5858
)
5959

60+
const istioCniName = "default"
61+
6062
// Reconciler reconciles an IstioRevision object
6163
type Reconciler struct {
6264
client.Client
@@ -213,6 +215,8 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
213215
// The handler triggers the reconciliation of the referenced IstioRevision CR so that its InUse condition is updated.
214216
revisionTagHandler := wrapEventHandler(logger, handler.EnqueueRequestsFromMapFunc(r.mapRevisionTagToReconcileRequest))
215217

218+
istioCniHandler := wrapEventHandler(logger, handler.EnqueueRequestsFromMapFunc(r.mapIstioCniToReconcileRequests))
219+
216220
return ctrl.NewControllerManagedBy(mgr).
217221
WithOptions(controller.Options{
218222
LogConstructor: func(req *reconcile.Request) logr.Logger {
@@ -258,6 +262,9 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
258262
Watches(&admissionv1.MutatingWebhookConfiguration{}, ownedResourceHandler).
259263
Watches(&admissionv1.ValidatingWebhookConfiguration{}, ownedResourceHandler, builder.WithPredicates(validatingWebhookConfigPredicate())).
260264

265+
// +lint-watches:ignore: IstioCNI (not found in charts, but this controller needs to watch it to update the IstioRevision status)
266+
Watches(&v1.IstioCNI{}, istioCniHandler).
267+
261268
// +lint-watches:ignore: ValidatingAdmissionPolicy (TODO: fix this when CI supports golang 1.22 and k8s 1.30)
262269
// +lint-watches:ignore: ValidatingAdmissionPolicyBinding (TODO: fix this when CI supports golang 1.22 and k8s 1.30)
263270
// +lint-watches:ignore: CustomResourceDefinition (prevents `make lint-watches` from bugging us about CRDs)
@@ -269,6 +276,8 @@ func (r *Reconciler) determineStatus(ctx context.Context, rev *v1.IstioRevision,
269276
reconciledCondition := r.determineReconciledCondition(reconcileErr)
270277
readyCondition, err := r.determineReadyCondition(ctx, rev)
271278
errs.Add(err)
279+
dependenciesHealthyCondition, err := r.determineDependenciesHealthyCondition(ctx, rev)
280+
errs.Add(err)
272281

273282
inUseCondition, err := r.determineInUseCondition(ctx, rev)
274283
errs.Add(err)
@@ -277,8 +286,9 @@ func (r *Reconciler) determineStatus(ctx context.Context, rev *v1.IstioRevision,
277286
status.ObservedGeneration = rev.Generation
278287
status.SetCondition(reconciledCondition)
279288
status.SetCondition(readyCondition)
289+
status.SetCondition(dependenciesHealthyCondition)
280290
status.SetCondition(inUseCondition)
281-
status.State = deriveState(reconciledCondition, readyCondition)
291+
status.State = deriveState(reconciledCondition, readyCondition, dependenciesHealthyCondition)
282292
return status, errs.Error()
283293
}
284294

@@ -298,11 +308,11 @@ func (r *Reconciler) updateStatus(ctx context.Context, rev *v1.IstioRevision, re
298308
return errs.Error()
299309
}
300310

301-
func deriveState(reconciledCondition, readyCondition v1.IstioRevisionCondition) v1.IstioRevisionConditionReason {
302-
if reconciledCondition.Status != metav1.ConditionTrue {
303-
return reconciledCondition.Reason
304-
} else if readyCondition.Status != metav1.ConditionTrue {
305-
return readyCondition.Reason
311+
func deriveState(conditions ...v1.IstioRevisionCondition) v1.IstioRevisionConditionReason {
312+
for _, c := range conditions {
313+
if c.Status != metav1.ConditionTrue {
314+
return c.Reason
315+
}
306316
}
307317
return v1.IstioRevisionReasonHealthy
308318
}
@@ -375,6 +385,43 @@ func (r *Reconciler) determineReadyCondition(ctx context.Context, rev *v1.IstioR
375385
return c, nil
376386
}
377387

388+
func (r *Reconciler) determineDependenciesHealthyCondition(ctx context.Context, rev *v1.IstioRevision) (v1.IstioRevisionCondition, error) {
389+
if revision.DependsOnIstioCNI(rev) {
390+
cni := v1.IstioCNI{}
391+
if err := r.Client.Get(ctx, client.ObjectKey{Name: istioCniName}, &cni); err != nil {
392+
if apierrors.IsNotFound(err) {
393+
return v1.IstioRevisionCondition{
394+
Type: v1.IstioRevisionConditionDependenciesHealthy,
395+
Status: metav1.ConditionFalse,
396+
Reason: v1.IstioRevisionReasonIstioCNINotFound,
397+
Message: "IstioCNI resource does not exist",
398+
}, nil
399+
}
400+
401+
return v1.IstioRevisionCondition{
402+
Type: v1.IstioRevisionConditionDependenciesHealthy,
403+
Status: metav1.ConditionUnknown,
404+
Reason: v1.IstioRevisionDependencyCheckFailed,
405+
Message: fmt.Sprintf("failed to get IstioCNI status: %v", err),
406+
}, fmt.Errorf("get failed: %w", err)
407+
}
408+
409+
if cni.Status.State != v1.IstioCNIReasonHealthy {
410+
return v1.IstioRevisionCondition{
411+
Type: v1.IstioRevisionConditionDependenciesHealthy,
412+
Status: metav1.ConditionFalse,
413+
Reason: v1.IstioRevisionReasonIstioCNINotHealthy,
414+
Message: "IstioCNI resource status indicates that the component is not healthy",
415+
}, nil
416+
}
417+
}
418+
419+
return v1.IstioRevisionCondition{
420+
Type: v1.IstioRevisionConditionDependenciesHealthy,
421+
Status: metav1.ConditionTrue,
422+
}, nil
423+
}
424+
378425
func (r *Reconciler) determineInUseCondition(ctx context.Context, rev *v1.IstioRevision) (v1.IstioRevisionCondition, error) {
379426
c := v1.IstioRevisionCondition{Type: v1.IstioRevisionConditionInUse}
380427

@@ -550,6 +597,21 @@ func (r *Reconciler) mapRevisionTagToReconcileRequest(ctx context.Context, revis
550597
return nil
551598
}
552599

600+
// mapIstioCniToReconcileRequests returns reconcile requests for all IstioRevisions that depend on IstioCNI
601+
func (r *Reconciler) mapIstioCniToReconcileRequests(ctx context.Context, _ client.Object) []reconcile.Request {
602+
list := v1.IstioRevisionList{}
603+
if err := r.Client.List(ctx, &list); err != nil {
604+
return nil
605+
}
606+
var reqs []reconcile.Request
607+
for _, rev := range list.Items {
608+
if revision.DependsOnIstioCNI(&rev) {
609+
reqs = append(reqs, reconcile.Request{NamespacedName: types.NamespacedName{Name: rev.Name}})
610+
}
611+
}
612+
return reqs
613+
}
614+
553615
// ignoreStatusChange returns a predicate that ignores watch events where only the resource status changes; if
554616
// there are any other changes to the resource, the event is not ignored.
555617
// This ensures that the controller doesn't reconcile the entire IstioRevision every time the status of an owned

‎controllers/istiorevision/istiorevision_controller_test.go

Lines changed: 48 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -205,47 +205,70 @@ func TestValidate(t *testing.T) {
205205

206206
func TestDeriveState(t *testing.T) {
207207
testCases := []struct {
208-
name string
209-
reconciledCondition v1.IstioRevisionCondition
210-
readyCondition v1.IstioRevisionCondition
211-
expectedState v1.IstioRevisionConditionReason
208+
name string
209+
conditions []v1.IstioRevisionCondition
210+
expectedState v1.IstioRevisionConditionReason
212211
}{
213212
{
214-
name: "healthy",
215-
reconciledCondition: newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
216-
readyCondition: newCondition(v1.IstioRevisionConditionReady, metav1.ConditionTrue, ""),
217-
expectedState: v1.IstioRevisionReasonHealthy,
213+
name: "healthy",
214+
conditions: []v1.IstioRevisionCondition{
215+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
216+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionTrue, ""),
217+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionTrue, ""),
218+
},
219+
expectedState: v1.IstioRevisionReasonHealthy,
218220
},
219221
{
220-
name: "not reconciled",
221-
reconciledCondition: newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionFalse, v1.IstioRevisionReasonReconcileError),
222-
readyCondition: newCondition(v1.IstioRevisionConditionReady, metav1.ConditionTrue, ""),
223-
expectedState: v1.IstioRevisionReasonReconcileError,
222+
name: "not reconciled",
223+
conditions: []v1.IstioRevisionCondition{
224+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionFalse, v1.IstioRevisionReasonReconcileError),
225+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionTrue, ""),
226+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionTrue, ""),
227+
},
228+
expectedState: v1.IstioRevisionReasonReconcileError,
224229
},
225230
{
226-
name: "not ready",
227-
reconciledCondition: newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
228-
readyCondition: newCondition(v1.IstioRevisionConditionReady, metav1.ConditionFalse, v1.IstioRevisionReasonIstiodNotReady),
229-
expectedState: v1.IstioRevisionReasonIstiodNotReady,
231+
name: "not ready",
232+
conditions: []v1.IstioRevisionCondition{
233+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
234+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionFalse, v1.IstioRevisionReasonIstiodNotReady),
235+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionTrue, ""),
236+
},
237+
expectedState: v1.IstioRevisionReasonIstiodNotReady,
230238
},
231239
{
232-
name: "readiness unknown",
233-
reconciledCondition: newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
234-
readyCondition: newCondition(v1.IstioRevisionConditionReady, metav1.ConditionUnknown, v1.IstioRevisionReasonReadinessCheckFailed),
235-
expectedState: v1.IstioRevisionReasonReadinessCheckFailed,
240+
name: "readiness unknown",
241+
conditions: []v1.IstioRevisionCondition{
242+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
243+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionUnknown, v1.IstioRevisionReasonReadinessCheckFailed),
244+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionTrue, ""),
245+
},
246+
expectedState: v1.IstioRevisionReasonReadinessCheckFailed,
236247
},
237248
{
238-
name: "not reconciled nor ready",
239-
reconciledCondition: newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionFalse, v1.IstioRevisionReasonReconcileError),
240-
readyCondition: newCondition(v1.IstioRevisionConditionReady, metav1.ConditionFalse, v1.IstioRevisionReasonIstiodNotReady),
241-
expectedState: v1.IstioRevisionReasonReconcileError, // reconcile reason takes precedence over ready reason
249+
name: "not reconciled nor ready",
250+
conditions: []v1.IstioRevisionCondition{
251+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionFalse, v1.IstioRevisionReasonReconcileError),
252+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionFalse, v1.IstioRevisionReasonIstiodNotReady),
253+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionTrue, ""),
254+
},
255+
expectedState: v1.IstioRevisionReasonReconcileError, // reconcile reason takes precedence over ready reason
256+
},
257+
{
258+
name: "dependencies not ready",
259+
conditions: []v1.IstioRevisionCondition{
260+
newCondition(v1.IstioRevisionConditionReconciled, metav1.ConditionTrue, ""),
261+
newCondition(v1.IstioRevisionConditionReady, metav1.ConditionTrue, ""),
262+
newCondition(v1.IstioRevisionConditionDependenciesHealthy, metav1.ConditionFalse, v1.IstioRevisionReasonIstioCNINotHealthy),
263+
},
264+
expectedState: v1.IstioRevisionReasonIstioCNINotHealthy,
242265
},
243266
}
244267

245268
for _, tc := range testCases {
246269
t.Run(tc.name, func(t *testing.T) {
247270
g := NewWithT(t)
248-
result := deriveState(tc.reconciledCondition, tc.readyCondition)
271+
result := deriveState(tc.conditions...)
249272
g.Expect(result).To(Equal(tc.expectedState))
250273
})
251274
}

0 commit comments

Comments
 (0)