Fix bug in challenge scheduler causing invalid results

Previously, we shared a single backing slice when
performing filter operations on slices, causing issues
when we perform sorting operations on that same
underlying slice.

Signed-off-by: James Munnelly <james@munnelly.eu>

pkg/controller/acmechallenges/scheduler/scheduler.go

@@ -124,7 +124,7 @@ func (s *Scheduler) determineChallengeCandidates(allChallenges []*cmapi.Challeng
 
 	// If there are any already in-progress challenges for a domain and type,
 	// filter them out.
-	candidates := quickFilterChallenges(dedupedCandidates, func(ch *cmapi.Challenge) bool {
+	candidates := filterChallenges(dedupedCandidates, func(ch *cmapi.Challenge) bool {
 		for _, inPCh := range inProgress {
 			if compareChallenges(ch, inPCh) == 0 {
 				glog.V(6).Infof("There is already a challenge processing for domain %q (type %q)", ch.Spec.DNSName, ch.Spec.Type)
@@ -149,7 +149,7 @@ func sortChallengesByTimestamp(chs []*cmapi.Challenge) {
 // notProcessingChallenges will filter out challenges from the given slice
 // that have status.processing set to true.
 func notProcessingChallenges(chs []*cmapi.Challenge) []*cmapi.Challenge {
-	return quickFilterChallenges(chs, func(ch *cmapi.Challenge) bool {
+	return filterChallenges(chs, func(ch *cmapi.Challenge) bool {
 		return !ch.Status.Processing
 	})
 }
@@ -157,7 +157,7 @@ func notProcessingChallenges(chs []*cmapi.Challenge) []*cmapi.Challenge {
 // processingChallenges will filter out challenges from the given slice
 // that have status.processing set to false.
 func processingChallenges(chs []*cmapi.Challenge) []*cmapi.Challenge {
-	return quickFilterChallenges(chs, func(ch *cmapi.Challenge) bool {
+	return filterChallenges(chs, func(ch *cmapi.Challenge) bool {
 		return ch.Status.Processing
 	})
 }
@@ -165,20 +165,18 @@ func processingChallenges(chs []*cmapi.Challenge) []*cmapi.Challenge {
 // incompleteChallenges will filter out challenges from the given slice
 // that are in a 'final' state
 func incompleteChallenges(chs []*cmapi.Challenge) []*cmapi.Challenge {
-	return quickFilterChallenges(chs, func(ch *cmapi.Challenge) bool {
+	return filterChallenges(chs, func(ch *cmapi.Challenge) bool {
 		return !acme.IsFinalState(ch.Status.State)
 	})
 }
 
-// https://github.com/golang/go/wiki/SliceTricks#Filtering-without-allocating
+func filterChallenges(chs []*cmapi.Challenge, fn func(ch *cmapi.Challenge) bool) []*cmapi.Challenge {
-func quickFilterChallenges(chs []*cmapi.Challenge, fn func(ch *cmapi.Challenge) bool) []*cmapi.Challenge {
+	ret := []*cmapi.Challenge{}
-	ret := chs[:0]
 	for _, ch := range chs {
 		if fn(ch) {
 			ret = append(ret, ch)
 		}
 	}
-
 	return ret
 }
 

pkg/controller/acmechallenges/scheduler/scheduler_test.go

@@ -227,6 +227,33 @@ func TestScheduleN(t *testing.T) {
 					gen.SetChallengeType("http01")),
 			},
 		},
+		// this test case replicates a failure seen in CI
+		{
+			name: "schedule a challenge when other challenges are already in progress",
+			n:    5,
+			challenges: []*cmapi.Challenge{
+				gen.Challenge("test1-0",
+					gen.SetChallengeDNSName("rvrko.certmanager.kubernetes.network"),
+					gen.SetChallengeType("dns-01"),
+					gen.SetChallengeWildcard(true)),
+				gen.Challenge("test1-1",
+					gen.SetChallengeDNSName("rvrko.certmanager.kubernetes.network"),
+					gen.SetChallengeType("dns-01"),
+					gen.SetChallengeWildcard(false),
+					// the non-wildcard version *is* processing
+					gen.SetChallengeProcessing(true)),
+				gen.Challenge("should-schedule",
+					gen.SetChallengeDNSName("aodob.certmanager.kubernetes.network"),
+					gen.SetChallengeType("dns-01"),
+					gen.SetChallengeWildcard(true)),
+			},
+			expected: []*cmapi.Challenge{
+				gen.Challenge("should-schedule",
+					gen.SetChallengeDNSName("aodob.certmanager.kubernetes.network"),
+					gen.SetChallengeType("dns-01"),
+					gen.SetChallengeWildcard(true)),
+			},
+		},
 		{
 			name: "don't schedule when total number of scheduled challenges exceeds global maximum",
 			n:    5,

test/unit/gen/challenge.go

@@ -64,6 +64,12 @@ func SetChallengePresented(p bool) ChallengeModifier {
 	}
 }
 
+func SetChallengeWildcard(p bool) ChallengeModifier {
+	return func(ch *v1alpha1.Challenge) {
+		ch.Spec.Wildcard = p
+	}
+}
+
 func SetChallengeState(s v1alpha1.State) ChallengeModifier {
 	return func(ch *v1alpha1.Challenge) {
 		ch.Status.State = s