Canary labels copied to primary on promotion when include-label-prefix not defined

[ta924]

I have noticed that when a canary is promoted I'm seeing all the canary annotations copied over to the primary when include-label-prefix is not defined. It seems that * and when a value is not defined resulting in the same results. I tested the method in the util_test file with the below method and it fails since the strings.HasPrefix returns true when comparing any string against an empty string. Is this expected behavior or should there be an additional check to rule out empty strings?

For reference I'm utilizing 1.24.1, but I didn't see any changes to the methods that would eliminate this issues.

Primary after creation and before first analysis

NAME              READY   UP-TO-DATE   AVAILABLE   AGE    APP
podinfo-primary   1/1     1            1           522d   podinfo-primary

Primary after promotion

NAME              READY   UP-TO-DATE   AVAILABLE   AGE    APP
podinfo-primary   1/1     1            1           522d   podinfo

func TestIncludeLabelsNoIncludes(t *testing.T) {
         labels := map[string]string{
		"foo":   "foo-value",
		"bar":   "bar-value",
		"lorem": "ipsum",
	}
        includeLabelPrefix := []string{""}

	filteredLabels := includeLabelsByPrefix(labels, includeLabelPrefix)
	assert.Equal(t, map[string]string{}, filteredLabels)
}

---FAIL: TestIncludeLabelsNoIncludes (0.00s)

Expected :map[string]string{}
Actual   :map[string]string{"bar":"bar-value", "foo":"foo-value", "lorem":"ipsum"}

---

From main I would expect the below would default to an empty string

flag.StringVar(&includeLabelPrefix, "include-label-prefix", "", "List of prefixes of labels that are copied when creating primary deployments or daemonsets. Use * to include all.")

Then the split would create an array with one entry consisting of one entry

includeLabelPrefixArray := strings.Split(includeLabelPrefix, ",")

Followed by an always true from the HasPrefix method when comparing the string

       if includeLabelPrefix == "*" || strings.HasPrefix(key, includeLabelPrefix) {
	      filteredLabels[key] = value
	      break
       }

---

I also noticed if I set include-label-prefix to a value that doesn't match any prefix of the labels on the canary, all labels will be dropped on the primary after promotion.

[Jetstream30]

Can u please take me of this smarter then me trace me shit Sent from my iPhoneOn Apr 4, 2023, at 1:46 PM, ta924 ***@***.***> wrote: I have noticed that when a canary is promoted I'm seeing all the canary annotations copied over to the primary when include-label-prefix is not defined. It seems that * and when a value is not defined resulting in the same results. I tested the method in the util_test file with the below method and it fails since the strings.HasPrefix returns true when comparing any string against an empty string. Is this expected behavior or should there be an additional check to rule out empty strings? For reference I'm utilizing 1.24.1, but I didn't see any changes to the methods that would eliminate this issues. ` func TestIncludeLabelsNoIncludes(t *testing.T) { labels := map[string]string{ "foo": "foo-value", "bar": "bar-value", "lorem": "ipsum", } includeLabelPrefix := []string{""} filteredLabels := includeLabelsByPrefix(labels, includeLabelPrefix) /*assert.Equal(t, filteredLabels, map[string]string{ //"foo": "foo-value", //"lorem": "ipsum", // bar excluded })*/ assert.Equal(t, map[string]string{}, filteredLabels) } ` ` --- FAIL: TestIncludeLabelsNoIncludes (0.00s) Expected :map[string]string{} Actual :map[string]string{"bar":"bar-value", "foo":"foo-value", "lorem":"ipsum"} ` —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ta924]

@stefanprodan This might be as simple as removing the logic https://github.com/fluxcd/flagger/blob/main/pkg/canary/deployment_controller.go#L134-L136 and calling the makePrimaryLabels func with the filtered labels and following the same flow as the createPrimaryDeployment func. I was thinking something around the lines of

     filteredLabels := includeLabelsByPrefix(canary.ObjectMeta.Labels, c.includeLabelPrefix)
     //Remove this logic
     /*for k, v := range filteredLabels {
	  primaryCopy.ObjectMeta.Labels[k] = v
        }*/
     //Pass filtered labels for inclusion and retain the selector label with -primary suffix
     primaryCopy.ObjectMeta.Labels = makePrimaryLabels(filteredLabels, primaryLabelValue, label)

I still need to do some testing on strings.HasPrefix always returning true on an empty string

[ta924]

@stefanprodan I will submit a PR for review today, thanks for evaluating