-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Fix diff panic for malformed kubeconfig #1581
Conversation
If the kubeconfig context name was not indented properly, it could lead to a map lookup failing, and then a subsequent unchecked nil value.
Does the PR have any schema changes?Looking good! No breaking changes found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test coverage for these failure cases?
if activeContext == nil { | ||
return &clientapi.Cluster{} | ||
} | ||
activeClusterName := activeContext.Cluster | ||
|
||
activeCluster := config.Clusters[activeClusterName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the same issue occur here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it. Good catch.
activeClusterName := config.Contexts[currentContext].Cluster | ||
activeContext := config.Contexts[currentContext] | ||
if activeContext == nil { | ||
return &clientapi.Cluster{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we don't report up a useful error about what caused this. Should this function return a useful error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's possible to tell if this is actually an error since this runs during preview and Outputs may not yet be fully resolved. I suspect that it might lead to erroneous warnings.
This particular case was a bit odd because the outdented YAML still parsed cleanly, but changed the semantics of the configuration. This change fixes the panic, and now correctly shows a replacement in the preview. This should be sufficient for a user to notice that something is going wrong if they weren't expecting a change.
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
Proposed changes
If the kubeconfig context name was not indented
properly, it could lead to a map lookup failing, and
then a subsequent unchecked nil value.
Related issues (optional)
Fix #1543