Fix bug in MergeAddons and test

[nckturner]

I was a little bit confused by the desired behavior of the AddonMenu merge in channels so I decided to write a test to better understand it. Can someone help me understand the expected behavior after a merge of two AddonMenus, like what happens in apply_channel.go? Unless my understanding is incorrect, (or I've got a bug in the test) it seems like the merge behavior is incorrect, or maybe just confusing.

--- FAIL: Test_MergeAddons (0.00s)
    addons_test.go:223: Unexpected AddonMenu merge result,
        Merged:
        AddonMenu{
          Addon{Name: a, Version: 1.0.0, KubernetesVersion: >=1.18.0, Id: k8s-1.18},
        }
        
        Expected:
        AddonMenu{
          Addon{Name: a, Version: 1.0.1, KubernetesVersion: >=1.18.0, Id: k8s-1.18},
        }
        
    addons_test.go:223: Unexpected AddonMenu merge result,
        Merged:
        AddonMenu{
          Addon{Name: a, Version: 1.0.0, KubernetesVersion: >=1.18.0, Id: k8s-1.18},
        }
        
        Expected:
        AddonMenu{
          Addon{Name: a, Version: 1.0.1, KubernetesVersion: >=1.18.0, Id: k8s-1.18},
        }
        
FAIL
FAIL	k8s.io/kops/channels/pkg/channels	0.016s
FAIL

This test passes with a change in addon.go from:

if existing.ChannelVersion().replaces(v.ChannelVersion()) {

to:

if v.ChannelVersion().replaces(existing.ChannelVersion()) {

So either my intuitive understanding of the MergeAddons function is off or this is a bug?

[ayberk]

This looks like an actual bug because replaces has to be called from the new version by passing the existing as a parameter. It should be if v.ChannelVersion().replaces(existing.ChannelVersion()) indeed.

[rifelpet]

Reading over the ChannelVersion.replaces code, I'm inclined to agree that this should be flipped around. Given that, I don't understand how new Kops versions could still be applying addons with updated versions. Most of this code hasn't been touched in over a year, some of it 4 years. How has this bug not been noticed until now?

/cc @justinsb for input

[k8s-ci-robot]

@rifelpet: GitHub didn't allow me to request PR reviews from the following users: for, input.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Reading over the ChannelVersion.replaces code, I'm inclined to agree that this should be flipped around. Given that, I don't understand how new Kops versions could still be applying addons with updated versions. Most of this code hasn't been touched in over a year, some of it 4 years. How has this bug not been noticed until now?

/cc @justinsb for input

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[nckturner]

I've added a commit that modifies the check, as it does seem to make sense as the correct merge behavior, but agree with @rifelpet that its strange this has never been detected. I think its because the usages I see for MergeAddons() are where we merge the addons from the virtual file system (i.e. S3) with those from what appears to be a local filesystem location. I'm not sure this code is even used during normal operations. Further down in apply_channel.go there is additional update code, which I think is the main update logic: https://github.com/kubernetes/kops/blob/master/channels/pkg/cmd/apply_channel.go#L136-L202

[hakman]

These should make staticcheck happy.

[nckturner]

@hakman thanks for cleaning that up

[hakman]

@hakman thanks for cleaning that up

No pb. Let's mention this next office hours and maybe get a quick review from @justinsb .

[hakman]

/retest

[justinsb]

Great find & fix - thanks @nckturner

I think we've got away with this in the past because historically we've only normally had a single manifest with our addons, so we likely haven't really gone down this codepath.

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, nckturner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment