-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 agent panic in case of malformed objects retrieved from the kvstore, and improve validation #27237
Conversation
/test |
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.
LGTM! 🚀
I've left inline a non-blocking style improvement advice, feel free to do as you please.
if n.ClusterID != option.Config.ClusterID { | ||
if err := cmtypes.ValidateClusterID(n.ClusterID); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil |
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.
Possible style improvement:
if n.ClusterID != option.Config.ClusterID { | |
if err := cmtypes.ValidateClusterID(n.ClusterID); err != nil { | |
return err | |
} | |
} | |
return nil | |
if n.ClusterID == option.Config.ClusterID { | |
return nil | |
} | |
return cmtypes.ValidateClusterID(n.ClusterID) |
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.
Generally speaking, I agree that your proposal is cleaner. I'd personally prefer to preserve the current approach, though, as it simplifies the addition of further checks, and it is consistent with the services validation.
Currently, the backends of global services received from remote clusters are parsed using `MustParseAddrCluster`. Hence, a global service with an invalid backend IP will cause the crash of all agents. Let's avoid this using `ParseAddrCluster` and handling the error. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Add a validation function to prevent unmarshalling invalid global service objects retrieved from the kvstore, which could then possibly trigger unexpected behavior. Currently, we check that the cluster ID is in the valid range, and that IP addresses are valid. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Add a validation function to prevent unmarshalling invalid node objects retrieved from the kvstore, which could then possibly trigger unexpected behavior. Currently, we only check that the cluster ID is in the valid range. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
e300f61
to
f5816dc
Compare
The last push reverted a minor modification for better consistency with the previous implementation, with no functional differences. |
/test |
This PR adds extra validation to prevent processing invalid node and service objects retrieved from the kvstore, which could trigger unexpected behavior.