From 74ecf8d978e3a4cd569d8355a01b6da144100c70 Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <liggitt@google.com>
Date: Thu, 29 Nov 2018 10:55:38 -0500
Subject: [PATCH] Improve reconcile output to explain what changes are being
 made

---
 pkg/kubectl/cmd/auth/BUILD                    |  1 +
 pkg/kubectl/cmd/auth/reconcile.go             | 62 +++++++++++++++++--
 .../rbac/reconciliation/reconcile_role.go     |  5 +-
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/pkg/kubectl/cmd/auth/BUILD b/pkg/kubectl/cmd/auth/BUILD
index 9be5ff830a..03b02da1b9 100644
--- a/pkg/kubectl/cmd/auth/BUILD
+++ b/pkg/kubectl/cmd/auth/BUILD
@@ -25,6 +25,7 @@ go_library(
         "//staging/src/k8s.io/api/rbac/v1alpha1:go_default_library",
         "//staging/src/k8s.io/api/rbac/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/cli-runtime/pkg/genericclioptions:go_default_library",
         "//staging/src/k8s.io/cli-runtime/pkg/genericclioptions/printers:go_default_library",
diff --git a/pkg/kubectl/cmd/auth/reconcile.go b/pkg/kubectl/cmd/auth/reconcile.go
index e2dafca743..a9267d2b82 100644
--- a/pkg/kubectl/cmd/auth/reconcile.go
+++ b/pkg/kubectl/cmd/auth/reconcile.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/spf13/cobra"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/klog"
 
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -192,7 +193,7 @@ func (o *ReconcileOptions) RunReconcile() error {
 			if err != nil {
 				return err
 			}
-			o.PrintObject(result.Role.GetObject(), o.Out)
+			o.printResults(result.Role.GetObject(), nil, nil, result.MissingRules, result.ExtraRules, result.Operation, result.Protected)
 
 		case *rbacv1.ClusterRole:
 			reconcileOptions := reconciliation.ReconcileRoleOptions{
@@ -207,7 +208,7 @@ func (o *ReconcileOptions) RunReconcile() error {
 			if err != nil {
 				return err
 			}
-			o.PrintObject(result.Role.GetObject(), o.Out)
+			o.printResults(result.Role.GetObject(), nil, nil, result.MissingRules, result.ExtraRules, result.Operation, result.Protected)
 
 		case *rbacv1.RoleBinding:
 			reconcileOptions := reconciliation.ReconcileRoleBindingOptions{
@@ -223,7 +224,7 @@ func (o *ReconcileOptions) RunReconcile() error {
 			if err != nil {
 				return err
 			}
-			o.PrintObject(result.RoleBinding.GetObject(), o.Out)
+			o.printResults(result.RoleBinding.GetObject(), result.MissingSubjects, result.ExtraSubjects, nil, nil, result.Operation, result.Protected)
 
 		case *rbacv1.ClusterRoleBinding:
 			reconcileOptions := reconciliation.ReconcileRoleBindingOptions{
@@ -238,7 +239,7 @@ func (o *ReconcileOptions) RunReconcile() error {
 			if err != nil {
 				return err
 			}
-			o.PrintObject(result.RoleBinding.GetObject(), o.Out)
+			o.printResults(result.RoleBinding.GetObject(), result.MissingSubjects, result.ExtraSubjects, nil, nil, result.Operation, result.Protected)
 
 		case *rbacv1beta1.Role,
 			*rbacv1beta1.RoleBinding,
@@ -258,3 +259,56 @@ func (o *ReconcileOptions) RunReconcile() error {
 		return nil
 	})
 }
+
+func (o *ReconcileOptions) printResults(object runtime.Object,
+	missingSubjects, extraSubjects []rbacv1.Subject,
+	missingRules, extraRules []rbacv1.PolicyRule,
+	operation reconciliation.ReconcileOperation,
+	protected bool) {
+
+	o.PrintObject(object, o.Out)
+
+	caveat := ""
+	if protected {
+		caveat = ", but object opted out (rbac.authorization.kubernetes.io/autoupdate: false)"
+	}
+	switch operation {
+	case reconciliation.ReconcileNone:
+		return
+	case reconciliation.ReconcileCreate:
+		fmt.Fprintf(o.ErrOut, "\treconciliation required create%s\n", caveat)
+	case reconciliation.ReconcileUpdate:
+		fmt.Fprintf(o.ErrOut, "\treconciliation required update%s\n", caveat)
+	case reconciliation.ReconcileRecreate:
+		fmt.Fprintf(o.ErrOut, "\treconciliation required recreate%s\n", caveat)
+	}
+
+	if len(missingSubjects) > 0 {
+		fmt.Fprintf(o.ErrOut, "\tmissing subjects added:\n")
+		for _, s := range missingSubjects {
+			fmt.Fprintf(o.ErrOut, "\t\t%+v\n", s)
+		}
+	}
+	if o.RemoveExtraSubjects {
+		if len(extraSubjects) > 0 {
+			fmt.Fprintf(o.ErrOut, "\textra subjects removed:\n")
+			for _, s := range extraSubjects {
+				fmt.Fprintf(o.ErrOut, "\t\t%+v\n", s)
+			}
+		}
+	}
+	if len(missingRules) > 0 {
+		fmt.Fprintf(o.ErrOut, "\tmissing rules added:\n")
+		for _, r := range missingRules {
+			fmt.Fprintf(o.ErrOut, "\t\t%+v\n", r)
+		}
+	}
+	if o.RemoveExtraPermissions {
+		if len(extraRules) > 0 {
+			fmt.Fprintf(o.ErrOut, "\textra rules removed:\n")
+			for _, r := range extraRules {
+				fmt.Fprintf(o.ErrOut, "\t\t%+v\n", r)
+			}
+		}
+	}
+}
diff --git a/pkg/registry/rbac/reconciliation/reconcile_role.go b/pkg/registry/rbac/reconciliation/reconcile_role.go
index 8197b9f759..e46848dd52 100644
--- a/pkg/registry/rbac/reconciliation/reconcile_role.go
+++ b/pkg/registry/rbac/reconciliation/reconcile_role.go
@@ -194,7 +194,10 @@ func computeReconciledRole(existing, expected RuleOwner, removeExtraPermissions
 	}
 
 	// Compute extra and missing rules
-	_, result.ExtraRules = validation.Covers(expected.GetRules(), existing.GetRules())
+	// Don't compute extra permissions if expected and existing roles are both aggregated
+	if expected.GetAggregationRule() == nil || existing.GetAggregationRule() == nil {
+		_, result.ExtraRules = validation.Covers(expected.GetRules(), existing.GetRules())
+	}
 	_, result.MissingRules = validation.Covers(existing.GetRules(), expected.GetRules())
 
 	switch {