Skip to content

Commit c888339

Browse files
dgndgn
authored
Fix InUse detection in IstioRevisionTag controller (openshift-service-mesh#553)
* Fix InUse detection in IstioRevisionTag controller Also adds unit tests. Signed-off-by: Daniel Grimm <[email protected]> * Shorten variable name Signed-off-by: Daniel Grimm <[email protected]> * Add more tests Signed-off-by: Daniel Grimm <[email protected]> * Rewrite mapPodToReconcileRequest to reconcile all relevant revisions Signed-off-by: Daniel Grimm <[email protected]> --------- Signed-off-by: Daniel Grimm <[email protected]>
1 parent cb2f0d3 commit c888339

File tree

5 files changed

+396
-77
lines changed

5 files changed

+396
-77
lines changed

‎controllers/istiorevision/istiorevision_controller.go‎

Lines changed: 31 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -447,47 +447,18 @@ func (r *Reconciler) isRevisionReferenced(ctx context.Context, rev *v1alpha1.Ist
447447
}
448448

449449
func namespaceReferencesRevision(ns corev1.Namespace, rev *v1alpha1.IstioRevision) bool {
450-
return rev.Name == getReferencedRevisionFromNamespace(ns.Labels)
450+
return rev.Name == revision.GetReferencedRevisionFromNamespace(ns.Labels)
451451
}
452452

453453
func podReferencesRevision(pod corev1.Pod, ns corev1.Namespace, rev *v1alpha1.IstioRevision) bool {
454-
return rev.Name == getReferencedRevisionFromPod(pod.GetLabels(), pod.GetAnnotations(), ns.GetLabels())
455-
}
456-
457-
func getReferencedRevisionFromNamespace(labels map[string]string) string {
458-
if labels[constants.IstioInjectionLabel] == constants.IstioInjectionEnabledValue {
459-
return v1alpha1.DefaultRevision
454+
if rev.Name == revision.GetInjectedRevisionFromPod(pod.GetAnnotations()) {
455+
return true
460456
}
461-
revision := labels[constants.IstioRevLabel]
462-
if revision != "" {
463-
return revision
457+
if revision.GetReferencedRevisionFromNamespace(ns.Labels) == "" &&
458+
rev.Name == revision.GetReferencedRevisionFromPod(pod.GetLabels()) {
459+
return true
464460
}
465-
// TODO: if .Values.sidecarInjectorWebhook.enableNamespacesByDefault is true, then all namespaces except system namespaces should use the "default" revision
466-
467-
return ""
468-
}
469-
470-
func getReferencedRevisionFromPod(podLabels, podAnnotations, nsLabels map[string]string) string {
471-
// if pod was already injected, the revision that did the injection is specified in the istio.io/rev annotation
472-
revision := podAnnotations[constants.IstioRevLabel]
473-
if revision != "" {
474-
return revision
475-
}
476-
477-
// pod is marked for injection by a specific revision, but wasn't injected (e.g. because it was created before the revision was applied)
478-
revisionFromNamespace := getReferencedRevisionFromNamespace(nsLabels)
479-
if podLabels[constants.IstioSidecarInjectLabel] != "false" {
480-
if revisionFromNamespace != "" {
481-
return revisionFromNamespace
482-
}
483-
revisionFromPod := podLabels[constants.IstioRevLabel]
484-
if revisionFromPod != "" {
485-
return revisionFromPod
486-
} else if podLabels[constants.IstioSidecarInjectLabel] == "true" {
487-
return v1alpha1.DefaultRevision
488-
}
489-
}
490-
return ""
461+
return false
491462
}
492463

493464
func istiodDeploymentKey(rev *v1alpha1.IstioRevision) client.ObjectKey {
@@ -532,24 +503,41 @@ func (r *Reconciler) mapNamespaceToReconcileRequest(ctx context.Context, ns clie
532503
}
533504

534505
// Check if the namespace references an IstioRevision in its labels
535-
revision := getReferencedRevisionFromNamespace(ns.GetLabels())
536-
if revision != "" {
537-
requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: revision}})
506+
revisionName := revision.GetReferencedRevisionFromNamespace(ns.GetLabels())
507+
if revisionName != "" {
508+
requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: revisionName}})
538509
}
539510
return requests
540511
}
541512

513+
// mapPodToReconcileRequest will collect all referenced revisions from a pod and its namespace and trigger reconciliation
542514
func (r *Reconciler) mapPodToReconcileRequest(ctx context.Context, pod client.Object) []reconcile.Request {
543-
// TODO: rewrite getReferencedRevisionFromPod to use lazy loading to avoid loading the namespace if the pod references a revision directly
515+
revisionNames := []string{}
516+
revisionName := revision.GetInjectedRevisionFromPod(pod.GetAnnotations())
517+
if revisionName != "" {
518+
revisionNames = append(revisionNames, revisionName)
519+
} else {
520+
revisionName = revision.GetReferencedRevisionFromPod(pod.GetLabels())
521+
if revisionName != "" {
522+
revisionNames = append(revisionNames, revisionName)
523+
}
524+
}
544525
ns := corev1.Namespace{}
545526
err := r.Client.Get(ctx, types.NamespacedName{Name: pod.GetNamespace()}, &ns)
546527
if err != nil {
547528
return nil
548529
}
530+
revisionName = revision.GetReferencedRevisionFromNamespace(ns.GetLabels())
531+
if revisionName != "" {
532+
revisionNames = append(revisionNames, revisionName)
533+
}
549534

550-
revision := getReferencedRevisionFromPod(pod.GetLabels(), pod.GetAnnotations(), ns.GetLabels())
551-
if revision != "" {
552-
return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: revision}}}
535+
if len(revisionNames) > 0 {
536+
reqs := []reconcile.Request{}
537+
for _, revName := range revisionNames {
538+
reqs = append(reqs, reconcile.Request{NamespacedName: types.NamespacedName{Name: revName}})
539+
}
540+
return reqs
553541
}
554542
return nil
555543
}

‎controllers/istiorevision/istiorevision_controller_test.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,12 @@ func TestDetermineInUseCondition(t *testing.T) {
526526
matchesRevision: "",
527527
},
528528

529+
// pod annotations only
530+
{
531+
podAnnotations: map[string]string{"istio.io/rev": "default"},
532+
matchesRevision: "default",
533+
},
534+
529535
// namespace labels only
530536
{
531537
nsLabels: map[string]string{"istio-injection": "enabled"},
@@ -567,6 +573,11 @@ func TestDetermineInUseCondition(t *testing.T) {
567573
},
568574

569575
// ns and pod labels
576+
{
577+
nsLabels: map[string]string{"istio.io/rev": "my-rev"},
578+
podLabels: map[string]string{"sidecar.istio.io/inject": "true"},
579+
matchesRevision: "my-rev",
580+
},
570581
{
571582
nsLabels: map[string]string{"istio-injection": "enabled"},
572583
podLabels: map[string]string{"istio.io/rev": "default"},

‎controllers/istiorevisiontag/istiorevisiontag_controller.go‎

Lines changed: 12 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"github.com/istio-ecosystem/sail-operator/pkg/helm"
3131
"github.com/istio-ecosystem/sail-operator/pkg/kube"
3232
"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
33+
"github.com/istio-ecosystem/sail-operator/pkg/revision"
3334
admissionv1 "k8s.io/api/admissionregistration/v1"
3435
autoscalingv2 "k8s.io/api/autoscaling/v2"
3536
corev1 "k8s.io/api/core/v1"
@@ -363,7 +364,7 @@ func (r *Reconciler) isRevisionTagReferencedByWorkloads(ctx context.Context, tag
363364
return false, fmt.Errorf("failed to list pods: %w", err)
364365
}
365366
for _, pod := range podList.Items {
366-
if podReferencesRevisionTag(pod, tag) {
367+
if ns, found := nsMap[pod.Namespace]; found && podReferencesRevisionTag(pod, tag, ns) {
367368
log.V(2).Info("RevisionTag is referenced by Pod", "Pod", client.ObjectKeyFromObject(&pod))
368369
return true, nil
369370
}
@@ -386,52 +387,29 @@ func (r *Reconciler) isRevisionTagReferencedByWorkloads(ctx context.Context, tag
386387
}
387388

388389
func namespaceReferencesRevisionTag(ns corev1.Namespace, tag *v1alpha1.IstioRevisionTag) bool {
389-
return tag.Name == getReferencedRevisionFromNamespace(ns.Labels)
390+
return tag.Name == revision.GetReferencedRevisionFromNamespace(ns.Labels)
390391
}
391392

392-
func podReferencesRevisionTag(pod corev1.Pod, tag *v1alpha1.IstioRevisionTag) bool {
393-
return tag.Name == getReferencedRevisionTagFromPod(pod.GetLabels())
394-
}
395-
396-
func getReferencedRevisionFromNamespace(labels map[string]string) string {
397-
if labels[constants.IstioInjectionLabel] == constants.IstioInjectionEnabledValue {
398-
return v1alpha1.DefaultRevision
399-
}
400-
revision := labels[constants.IstioRevLabel]
401-
if revision != "" {
402-
return revision
403-
}
404-
// TODO: if .Values.sidecarInjectorWebhook.enableNamespacesByDefault is true, then all namespaces except system namespaces should use the "default" revision
405-
406-
return ""
407-
}
408-
409-
func getReferencedRevisionTagFromPod(podLabels map[string]string) string {
410-
// we only look at pod labels to identify injection intent, as the annotation only references the real revision name instead of the tag
411-
if podLabels[constants.IstioInjectionLabel] == constants.IstioInjectionEnabledValue {
412-
return v1alpha1.DefaultRevision
413-
} else if podLabels[constants.IstioSidecarInjectLabel] != "false" && podLabels[constants.IstioRevLabel] != "" {
414-
return podLabels[constants.IstioRevLabel]
415-
}
416-
417-
return ""
393+
func podReferencesRevisionTag(pod corev1.Pod, tag *v1alpha1.IstioRevisionTag, ns corev1.Namespace) bool {
394+
return revision.GetReferencedRevisionFromNamespace(ns.Labels) == "" &&
395+
tag.Name == revision.GetReferencedRevisionFromPod(pod.GetLabels())
418396
}
419397

420398
func (r *Reconciler) mapNamespaceToReconcileRequest(ctx context.Context, ns client.Object) []reconcile.Request {
421399
var requests []reconcile.Request
422400

423401
// Check if the namespace references an IstioRevisionTag in its labels
424-
revision := getReferencedRevisionFromNamespace(ns.GetLabels())
425-
if revision != "" {
426-
requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: revision}})
402+
tag := revision.GetReferencedRevisionFromNamespace(ns.GetLabels())
403+
if tag != "" {
404+
requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: tag}})
427405
}
428406
return requests
429407
}
430408

431409
func (r *Reconciler) mapPodToReconcileRequest(ctx context.Context, pod client.Object) []reconcile.Request {
432-
revision := getReferencedRevisionTagFromPod(pod.GetLabels())
433-
if revision != "" {
434-
return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: revision}}}
410+
tag := revision.GetReferencedRevisionFromPod(pod.GetLabels())
411+
if tag != "" {
412+
return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: tag}}}
435413
}
436414
return nil
437415
}

0 commit comments

Comments
 (0)