fix VPA controller_fetcher node validation

[ZihanJiang96]

What type of PR is this?

/kind bug

Optionally add one or more of the following kinds if applicable:

/kind regression

What this PR does / why we need it:

regression introduced by this PR
if gk, err := key.groupKind() returns error, gk will be empty struct, the if condition will never become true

[k8s-ci-robot]

Hi @ZihanJiang96. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[ZihanJiang96]

Hi @jbartosik, could you please take a look? Thank you!

[Shubham82]

/ok-to-test

[ZihanJiang96]

Hi @kgolab and @voelzmo could you please review? 🙏

[voelzmo]

You're right, seems like the call to wellKnownController(gk.Kind) == node was only ever done when key.groupKind() returned an error – which also set gk to an empty struct and therefore made the call to wellKnownController(gk.Kind) == node useless.

Out of curiosity: Looking at the rest of the function, I don't think this should have caused any issues for you, though. The result of isWellKnownOrScalable should have been false in all these cases – or did you see an issue when using VPA that this didn't work?

/lgtm

[ZihanJiang96]

You're right, seems like the call to wellKnownController(gk.Kind) == node was only ever done when key.groupKind() returned an error – which also set gk to an empty struct and therefore made the call to wellKnownController(gk.Kind) == node useless.

Out of curiosity: Looking at the rest of the function, I don't think this should have caused any issues for you, though. The result of isWellKnownOrScalable should have been false in all these cases – or did you see an issue when using VPA that this didn't work?

/lgtm

it doesn't cause any issue at our runtime.
but isWellKnownOrScalable() will finally call getScaleForResource(), which will make a call to the k8s api-server. we want to reduce the unnecessery calls.

[voelzmo]

/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: voelzmo, ZihanJiang96

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:

• vertical-pod-autoscaler/OWNERS [voelzmo]

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