Merge pull request #653 from mattmoyer/fix-impersonation-test-flake

Fix another impersonation test flake related to `kubectl logs --tail` line count + test refactor.

test/integration/concierge_impersonation_proxy_test.go

@@ -980,7 +980,9 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			logLinesCount := 10
 			stdout, err = runKubectl(t, kubeconfigPath, envVarsWithProxy, "logs", "--namespace", env.ConciergeNamespace, conciergePod.Name, fmt.Sprintf("--tail=%d", logLinesCount))
 			require.NoError(t, err, `"kubectl logs" failed`)
-			require.Equalf(t, logLinesCount, strings.Count(stdout, "\n"), "wanted %d newlines in kubectl logs output:\n%s", logLinesCount, stdout)
+			// Expect _approximately_ logLinesCount lines in the output
+			// (we can't match 100% exactly due to https://github.com/kubernetes/kubernetes/issues/72628).
+			require.InDeltaf(t, logLinesCount, strings.Count(stdout, "\n"), 1, "wanted %d newlines in kubectl logs output:\n%s", logLinesCount, stdout)
 
 			// run the kubectl attach command
 			namespaceName := createTestNamespace(t, adminClient)
@@ -1198,44 +1200,55 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			t.Skip("only running when the cluster is meant to be using LoadBalancer services")
 		}
 
-		applyAnnotations := func(mutateAnnotationsFunc func(map[string]string)) {
+		// Grab the state of the CredentialIssuer prior to this test, so we can restore things back afterwards.
-			var expectedAnnotations map[string]string
+		previous, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
+		require.NoError(t, err)
+
+		applyCredentialIssuerAnnotations := func(annotations map[string]string) {
 			require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error {
-				newCredentialIssuer, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
+				issuer, err := adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Get(ctx, credentialIssuerName(env), metav1.GetOptions{})
 				if err != nil {
 					return err
 				}
-				mutateAnnotationsFunc(newCredentialIssuer.Spec.ImpersonationProxy.Service.Annotations)
+				updated := issuer.DeepCopy()
-				expectedAnnotations = newCredentialIssuer.Spec.ImpersonationProxy.Service.Annotations
+				updated.Spec.ImpersonationProxy.Service.Annotations = annotations
-				_, err = adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Update(ctx, newCredentialIssuer, metav1.UpdateOptions{})
+				if equality.Semantic.DeepEqual(issuer, updated) {
+					return nil
+				}
+
+				t.Logf("updating CredentialIssuer with spec.impersonationProxy.service.annotations: %v", annotations)
+				_, err = adminConciergeClient.ConfigV1alpha1().CredentialIssuers().Update(ctx, updated, metav1.UpdateOptions{})
 				return err
 			}))
-			t.Logf("updated CredentialIssuer with annotations %v", expectedAnnotations)
+		}
 
-			// Wait until the annotation shows up on the load balancer
+		waitForServiceAnnotations := func(annotations map[string]string) {
 			library.RequireEventuallyWithoutError(t, func() (bool, error) {
 				service, err := adminClient.CoreV1().Services(env.ConciergeNamespace).Get(ctx, impersonationProxyLoadBalancerName(env), metav1.GetOptions{})
 				if err != nil {
 					return false, err
 				}
 				t.Logf("found Service %s of type %s with annotations: %s", service.Name, service.Spec.Type, service.Annotations)
-				return equality.Semantic.DeepEqual(service.Annotations, expectedAnnotations), nil
+				return equality.Semantic.DeepEqual(service.Annotations, annotations), nil
 			}, 30*time.Second, 100*time.Millisecond)
 		}
 
-		// Set a new annotation and expect it to appear on the Service.
+		// Whatever happens, set the annotations back to the original value and expect the Service to be updated.
-		newAnnotationKey := "pinniped.dev/test-" + library.RandHex(t, 8)
+		t.Cleanup(func() {
-		newAnnotationValue := "test-" + library.RandHex(t, 8)
+			t.Log("reverting CredentialIssuer back to previous configuration")
-		applyAnnotations(func(annotations map[string]string) {
+			applyCredentialIssuerAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations)
-			annotations[newAnnotationKey] = newAnnotationValue
+			waitForServiceAnnotations(previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations)
 		})
 
-		// Remove the annotation and expect it to be removed from the Service
+		// Set a new annotation in the CredentialIssuer spec.impersonationProxy.service.annotations field.
-		t.Cleanup(func() {
+		newAnnotationKey := "pinniped.dev/test-" + library.RandHex(t, 8)
-			applyAnnotations(func(annotations map[string]string) {
+		newAnnotationValue := "test-" + library.RandHex(t, 8)
-				delete(annotations, newAnnotationKey)
+		updatedAnnotations := previous.Spec.ImpersonationProxy.Service.DeepCopy().Annotations
-			})
+		updatedAnnotations[newAnnotationKey] = newAnnotationValue
-		})
+		applyCredentialIssuerAnnotations(updatedAnnotations)
+
+		// Expect it to be applied to the Service.
+		waitForServiceAnnotations(updatedAnnotations)
 	})
 
 	t.Run("running impersonation proxy with ClusterIP service", func(t *testing.T) {