Skip to content
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

feat: support static routes #49

Merged
merged 1 commit into from
Sep 10, 2024
Merged

feat: support static routes #49

merged 1 commit into from
Sep 10, 2024

Conversation

rollandf
Copy link
Member

@rollandf rollandf commented Sep 3, 2024

Support optional static routes in the CNI CmdAdd result.

@coveralls
Copy link

coveralls commented Sep 3, 2024

Coverage Status

coverage: 70.056% (+0.5%) from 69.594%
when pulling 9c6939d on rollandf:routes
into 692d0ea on Mellanox:main.

Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

Great work! I added few minor comments

@@ -45,6 +45,10 @@ type IPPoolSpec struct {
Gateway string `json:"gateway,omitempty"`
// selector for nodes, if empty match all nodes
NodeSelector *corev1.NodeSelector `json:"nodeSelector,omitempty"`
// if true, add gateway as default gateway in the routes list
DefaultGateway *bool `json:"defautGateway,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to use a pointer here? It looks like that "false" is a safe default

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Yury, simple boolean should be enough I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


// validateRoutes validate routes:
// - dst is a valid CIDR
func validateRoutes(routes []Route, fldPath *field.Path) field.ErrorList {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should also validate that routes are from the same address family (ipv4, ipv6) as the pool

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

func validateRoutes(routes []Route, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
for i, r := range routes {
_, _, err := net.ParseCIDR(r.Dst)
Copy link
Collaborator

@ykulazhenkov ykulazhenkov Sep 3, 2024

Choose a reason for hiding this comment

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

net.ParserCIDR parses 0.0.0.0/0 as a valid CIDR. Do we want to filter default GW here to make sure that we will not create duplicate default route entries in case if DefaultGateway is set to true and 0.0.0.0/0 is explicitly added to the routes list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@vasrem vasrem left a comment

Choose a reason for hiding this comment

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

LGTM

Just one comment to address with the bool. The rest are mostly nitpicks.

api/v1alpha1/cidrpool_test.go Outdated Show resolved Hide resolved
@@ -45,6 +45,10 @@ type IPPoolSpec struct {
Gateway string `json:"gateway,omitempty"`
// selector for nodes, if empty match all nodes
NodeSelector *corev1.NodeSelector `json:"nodeSelector,omitempty"`
// if true, add gateway as default gateway in the routes list
DefaultGateway *bool `json:"defautGateway,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Yury, simple boolean should be enough I think.

pkg/ipam-node/handlers/allocate.go Outdated Show resolved Hide resolved
pkg/ipam-node/handlers/allocate.go Show resolved Hide resolved
@rollandf rollandf force-pushed the routes branch 2 times, most recently from 9c34adc to f614767 Compare September 4, 2024 12:22
Copy link
Collaborator

@vasrem vasrem left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

The PR look good. LGTM. One small comment

}

// RouteSlice is a type alias for a slice of Route
type RouteSlice []Route
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Do we need this type and functions below? It looks like it is not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Support optional static routes in the CNI CmdAdd result.

Signed-off-by: Fred Rolland <[email protected]>
Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

LGTM

@ykulazhenkov ykulazhenkov merged commit 413c1eb into Mellanox:main Sep 10, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants