Skip to content

Bug 2031049: Fix panic when PlatformStatus VSphere is nil #2865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Dec 17, 2021
Merged

Bug 2031049: Fix panic when PlatformStatus VSphere is nil #2865

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,12 @@ func getIgnitionHost(infraStatus *configv1.InfrastructureStatus) (string, error)
case configv1.OvirtPlatformType:
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIP, securePortStr)
case configv1.VSpherePlatformType:
if infraStatus.PlatformStatus.VSphere.APIServerInternalIP != "" {
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.VSphere.APIServerInternalIP, securePortStr)
if infraStatus.PlatformStatus.VSphere != nil {
Copy link
Contributor

@sinnykumari sinnykumari Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we haven't changed this code in a while, so wondering why we are hitting this bug now. My main concern is, is this something expected on vSphere to have infraStatus.PlatformStatus.VSphere nil or something is missing during vSphere cluster install. Also, can it cause any possible issue later with cluster where we are not adding securePortStr on a vSphere cluster?

Copy link
Contributor

@yuqi-zhang yuqi-zhang Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think this was due to the recent PR merge to manage user data secret, as a result of

129d6e4#diff-3e205577bec1a1d1711df8bfeff30e63f044b24cfbdc69bbd7d0052fdc881891

and

59f1381#diff-3e205577bec1a1d1711df8bfeff30e63f044b24cfbdc69bbd7d0052fdc881891

Which we didn't do in sync.go prior to this since we (presumably) didn't need to figure out the ignition port. I may have been a little hasty on merging that PR. Maybe we can revisit that whole section? Otherwise the fix here (although it would get us past the failure) will still cause problems for the managed secret.

Copy link
Contributor

@sinnykumari sinnykumari Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we may hit this bug on other platform handled in getIgnitionHost() right?
Jerry, since you have better context of #2827 , do you think it is better to revert the PR or check with PR author to see if we can fix the issue?

Copy link
Contributor

@yuqi-zhang yuqi-zhang Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zaneb

I think this is because some of the old ported code may not be updated with updated assumptions on what can be nil on what platform.

Copy link
Member

@zaneb zaneb Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no immediate plans to use the managed secret in vSphere, so I think we should go ahead with this fix.

It's not clear to me why infraStatus.PlatformStatus would be non-nil but infraStatus.PlatformStatus.VSphere nil on a vSphere cluster.

Copy link
Contributor

@yuqi-zhang yuqi-zhang Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I dug around for this, and we aren't exactly internally consistent on this either. Namely, we have 2 functions that query for this field,

  1. in the operator:

    machine-config-operator/pkg/operator/render.go

    Line 272 in b50080b

    func onPremPlatformAPIServerInternalIP(cfg mcfgv1.ControllerConfigSpec) (interface{}, error) {
    - this doesn't actually seem to have that exact detection
  2. in the template controller:

    machine-config-operator/pkg/controller/template/render.go

    Line 466 in aaa600e

    func onPremPlatformAPIServerInternalIP(cfg RenderConfig) (interface{}, error) {
    - This does have the extra detection and the reasoning.

I wonder why the operator one doesn't error. In any case I think we should probably look to de-duplicate the above 2 functions and then use the correct one here. At least we should be safe for the other platforms, so it's ok either way.

We can just merge this PR as is to unblock CI -> properly de-dupe as well.

if infraStatus.PlatformStatus.VSphere.APIServerInternalIP != "" {
ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.VSphere.APIServerInternalIP, securePortStr)
}
} else {
glog.Warning("Warning: PlatformStatus.VSphere should not be nil")
}
}
}
Expand Down