Add dynamic ranges to generated communication matrix #275

shirmoran · 2025-11-27T11:54:52Z

This PR is adding the following changes:

As requested from the partenrs, the dynamic port ranges of the cluster (Linux dynamic\private ranges, kubelet node port dynamic range, host level services dynamic range), are added to the generated communication matrix in all of the supported formats. The dynamic ranges are extracted in the following ways:

Node port dynamic range: we are looking for a custom defined range (network.Spec.ServiceNodePortRange), if there is no range set, we use a default static range (30000-32767)
Linux dynamic\private range: we retrive the dynamic range by reading the host sysctl (/proc/sys/net/ipv4/ip_local_port_range).

We also have added the option of using a custom entries file that include ranges, so partners will be able to add their own ranges if needed.
The e2e tests have been modified the following:

EPS vs SS: in the comparsion between the matrices, we also check if host level open ports (from ss matrix) which don't have an EPS are in the range specified in the generated commatrix.
Doc vs EPS: in the comparison between the matrices, we also check if the ports in the generated commatrix which are not documented in the doc matrix, do appear in the doc ranges.

The custom entries samples had been modified to also include ranges.
commatrix.go file's unit tests had been modified to allow mocking of a debugpod in the tests. For that sense, we have added to the commatrixCreator struct a utils field similar to what's done in the ConnectionCheck struct.

openshift-ci · 2025-11-27T11:54:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shirmoran
Once this PR has been reviewed and has the lgtm label, please assign oribon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

shirmoran · 2025-11-27T11:55:27Z

/cc @yuvalk

cmd/generate/generate.go

pkg/commatrix-creator/commatrix.go

aabughosh

/lgtm

shirmoran · 2025-12-09T09:22:29Z

/retest

oribon · 2025-12-14T09:05:44Z

pkg/types/types.go

+	Matrix        []ComDetails
+	DynamicRanges []DynamicRange


why did we split the struct rather than using DynamicRanges inside the Matrix?
specifically, in the csv format it looks like a part of the matrix and in the json format it looks like a separate entity, which of these behaviors we would like to go with (because currently it looks a bit odd imo)?

Mainly because the needed fields are different when documenting dynamic ranges compared to single ports, so we tried going with a seperate entity. I agree the csv format doesn't represent well the new entity, and it is something we also discussed with @yuvalk. But since Nokia asked to add the ranges to the csv format we thought it is the best way to go. Though if you have any other suggestions I'll be happy to hear :)

openshift-ci · 2025-12-22T09:32:18Z

New changes are detected. LGTM label has been removed.

oribon · 2025-12-22T09:52:43Z

can you please split your commits into:

renaming Matrix field to Ports
the rest

oribon · 2025-12-23T10:21:21Z

pkg/commatrix-creator/commatrix.go

+	var customMatrix *types.ComMatrix
 	if cm.customEntriesPath != "" {
 		log.Debug("Loading custom entries from file")
-		customComDetails, err := cm.GetComDetailsListFromFile()
+		customMatrix, err = cm.GetComMatrixFromFile()
 		if err != nil {
 			log.Errorf("Failed adding custom entries: %s", err)
 			return nil, fmt.Errorf("failed adding custom entries: %s", err)
 		}
-		epSliceComDetails = append(epSliceComDetails, customComDetails...)
+		epSliceComDetails = append(epSliceComDetails, customMatrix.Ports...)
 	}


can we move this logic below (so it is the last as before)?
also, instead of initializing customMatrix outside, it would be cleaner to have an empty slice customDynamicRanges, populate it after parsing the customMatrix and appending it to the existing (instead of if customMatrix != nil && ..)

oribon · 2025-12-23T10:27:26Z

pkg/types/types.go

 	} `json:"containers"`
 }

+func (dr *DynamicRange) ToString() string {


nit: rename to Ports / PortRange

I think it might be confusing with Ports being the name of the ComMatrix ComDetails list.
Also, I think ToString might be a good expression for what the method is doing - representing dynamic range struct as a string. What do you think?

when I see a ToString / String method I assume I'm getting a string representation of an object (which could identify it). returning only the ports seems a bit too narrow, considering multiple "DynamicRanges" can return the same

Yeah okay I see your point.
What do you think about "ToPortRangeString"?

sounds good (I'd omit the "To" though, but both are fine)

oribon · 2025-12-23T10:28:10Z

pkg/types/types.go

 	}

+	// Append dynamic ranges as rows under ComDetails headers.
+	if len(m.DynamicRanges) > 0 {


nit: redundant if

oribon · 2025-12-23T10:30:17Z

pkg/types/types.go

 	}

+	for _, dr := range m.DynamicRanges {
+		rangeStr := fmt.Sprintf("%d-%d", dr.MinPort, dr.MaxPort)


nit: can we use dr.ToString() here?

oribon · 2025-12-23T10:31:20Z

pkg/types/types.go

+		if err := json.Unmarshal(content, &cm); err == nil {
+			return &cm, nil
 		}
+		return nil, fmt.Errorf("failed to parse JSON as ComMatrix")


can you wrap with the err here?

oribon · 2025-12-23T10:38:11Z

pkg/types/types.go

+		port, err := strconv.Atoi(portStr)
+		if err != nil {
+			return nil, fmt.Errorf("invalid port %q: %w", portStr, err)
+		}
+		details = append(details, ComDetails{
+			Direction: r.Direction,
+			Protocol:  r.Protocol,
+			Port:      port,
+			Namespace: r.Namespace,
+			Service:   r.Service,
+			Pod:       r.Pod,
+			Container: r.Container,
+			NodeGroup: r.NodeGroup,
+			Optional:  r.Optional,
+		})


same (wrap in helper)

oribon · 2025-12-23T10:39:55Z

pkg/types/types.go

+// parsePortRange parses strings like "MIN MAX" or "MIN-MAX" into numeric bounds.
+func parsePortRange(s string) (int, int, error) {


can you split this into 2 functions? (one per format)

Just want to make sure I understood:

a function parsing "MIN MAX" to range

a function parsing "MIN-MAX" to range

If so, it seems similar to me with the same purpose, not sure why we should seperate. Can you please elaborate?

otoh it's easier to debug, is more readable and it forces the caller to be aware of what they are passing

overall, it's worth investing a few lines of code to have a more readable (and explicit) behavior, instead of allowing a function to have multiple behaviors hidden (e.g, what happens when you want to support MIN~MAX, would the existing function become longer?)

I see your point. Thanks for the note!

oribon · 2025-12-23T10:40:26Z

pkg/utils/utils.go

 		return fmt.Errorf("failed deleting namespace %s: %v", namespace, err)
 	}
-
+	if pollErr := wait.PollUntilContextTimeout(context.TODO(), time.Second, 2*time.Minute, true, func(ctx context.Context) (bool, error) {


what is this change (how is it related to this pr)?

When adding the linux dynamic ranges to the matrix we create a namespace that has to be deleted afterwards. This logic is needed after the deletion, we used also when deleting a namespace in the generate.go file (see relevant deleted lines).
As I relalized we need to use the logic related to the deletion in both scenarios I thought it will be more logical to include it in the function. Though I agree maybe this change should be in a seperate commit or PR. Which approach is better in your opinnion?

I think it makes sense having it in a different commit here

oribon · 2025-12-23T10:41:31Z

pkg/dynamic-ranges/dynamic-ranges.go

+// parsePortRange parses "MIN MAX" or "MIN-MAX" formatted ranges.
+func parsePortRange(s string) (int, int, error) {


can this be shared somehow instead of duplicating?

Yep you're right, I missed that, thanks!

oribon · 2025-12-23T12:14:08Z

pkg/dynamic-ranges/dynamic-ranges.go

+	for i := range dr {
+		dr[i].MinPort = minPort
+		dr[i].MaxPort = maxPort
+	}


not sure I understand this logic, why would we want to replace this way instead of explicitly building the range?

The Idea was to keep all other fields of types.KubeletNodePortDefaultDynamicRange (as dr is set to in line 53) and just change the MinPort and MaxPort field if network.Spec.ServiceNodePortRange is set.
Do you think there is a better way of doing so?

I'd either:

redeclare the bits explicitly

wrap the "kubelet node ports" in a function (and have default vars for the min/max ports)

the existing way just seems too hacky, and relies on the structure of a var (declared in a different file) which is not so intuitive

As we will be adding dynamice ranges to the matrix, we want to make it more clear that there are two parts of the matrix - ports and ranges, rather than the communication matrix and additional ranges.

oribon · 2025-12-31T09:44:37Z

cmd/communication-matrix/communication-matrix.csv

what is this file (and dir) for?

It shouldn't be here. I'll remove it, thanks!

oribon · 2025-12-31T09:49:48Z

pkg/commatrix-creator/commatrix_test.go

+	testNetwork = &configv1.Network{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "cluster",
+		},
+		Spec: configv1.NetworkSpec{
+			ServiceNodePortRange: "30000-32767",
+		},
+	}


can you change this to non-default values, and have another test where the field is not specified to see we consume the "default" var? (also it seems the test doesn't do anything with these and the linux values?)

The test doesn't though in the creation of the commatrix we do try to get those values. But we should have desiganted tests for that I agree. I will add them

Though I think we should create a new file for tests under dynamic-ranges, and there we will test the functions getting the ranges, here we will only mock the behavior the enable the creation of the commatrix (as we do now). What do you think?

oribon · 2025-12-31T10:14:33Z

pkg/dynamic-ranges/dynamic-ranges.go

can we add some unit tests around the functions here and types.go?

This PR is adding the following changes: As requested from the partenrs, the dynamic port ranges of the cluster (Linux dynamic\private ranges, kubelet node port dynamic range, host level services dynamic range), are added to the generated communication matrix in all of the supported formats. The dynamic ranges are extracted in the following ways: Node port dynamic range: we are looking for a custom defined range (network.Spec.ServiceNodePortRange), if there is no range set, we use a default static range (30000-32767) Linux dynamic\private range: we retrive the dynamic range by reading the host sysctl (/proc/sys/net/ipv4/ip_local_port_range). We also have added the option of using a custom entries file that include ranges, so partners will be able to add their own ranges if needed. The e2e tests have been modified the following: EPS vs SS: in the comparsion between the matrices, we also check if host level open ports (from ss matrix) which don't have an EPS are in the range specified in the generated commatrix. Doc vs EPS: in the comparison between the matrices, we also check if the ports in the generated commatrix which are not documented in the doc matrix, do appear in the doc ranges. The custom entries samples had been modified to also include ranges. commatrix.go file's unit tests had been modified to allow mocking of a debugpod in the tests. For that sense, we have added to the commatrixCreator struct a utils field similar to what's done in the ConnectionCheck struct.

As we use this logic every time we delete a namespces it makes more sense to have it inside the function.

openshift-ci bot requested review from SchSeba and aabughosh November 27, 2025 11:54

openshift-ci bot requested a review from yuvalk November 27, 2025 11:55

shirmoran force-pushed the add-dynamic-range-to-commatrix branch 5 times, most recently from ed8384d to 4316ac0 Compare December 3, 2025 15:09

aabughosh reviewed Dec 4, 2025

View reviewed changes

cmd/generate/generate.go Outdated Show resolved Hide resolved

aabughosh reviewed Dec 4, 2025

View reviewed changes

pkg/commatrix-creator/commatrix.go Outdated Show resolved Hide resolved

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 4316ac0 to f28b674 Compare December 8, 2025 08:45

shirmoran requested a review from aabughosh December 8, 2025 08:45

aabughosh reviewed Dec 8, 2025

View reviewed changes

openshift-ci bot assigned aabughosh Dec 8, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2025

oribon reviewed Dec 14, 2025

View reviewed changes

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from f28b674 to a960623 Compare December 22, 2025 09:32

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2025

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from a960623 to 9cb4066 Compare December 22, 2025 09:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2025

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 9cb4066 to 26f013c Compare December 22, 2025 09:51

shirmoran force-pushed the add-dynamic-range-to-commatrix branch 3 times, most recently from bcadc0a to 988d969 Compare December 22, 2025 10:25

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 988d969 to bcb56a4 Compare December 22, 2025 11:02

oribon reviewed Dec 23, 2025

View reviewed changes

shirmoran force-pushed the add-dynamic-range-to-commatrix branch 4 times, most recently from e96bcc7 to 0265d67 Compare December 25, 2025 09:01

Change ComMatrix struct Matrix field name to Ports

a1bf58e

As we will be adding dynamice ranges to the matrix, we want to make it more clear that there are two parts of the matrix - ports and ranges, rather than the communication matrix and additional ranges.

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 0265d67 to 6195fd3 Compare December 25, 2025 11:46

shirmoran requested a review from oribon December 29, 2025 10:16

oribon reviewed Dec 31, 2025

View reviewed changes

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 6195fd3 to 055c8c2 Compare January 8, 2026 13:32

shirmoran added 2 commits January 8, 2026 15:35

Add poll logic to DeleteNamespace func

1cd3d7a

As we use this logic every time we delete a namespces it makes more sense to have it inside the function.

shirmoran force-pushed the add-dynamic-range-to-commatrix branch from 055c8c2 to 1cd3d7a Compare January 8, 2026 13:35

shirmoran requested a review from oribon January 8, 2026 13:58

		// parsePortRange parses strings like "MIN MAX" or "MIN-MAX" into numeric bounds.
		func parsePortRange(s string) (int, int, error) {

		// parsePortRange parses "MIN MAX" or "MIN-MAX" formatted ranges.
		func parsePortRange(s string) (int, int, error) {

Add dynamic ranges to generated communication matrix #275

Are you sure you want to change the base?

Add dynamic ranges to generated communication matrix #275

Uh oh!

Conversation

shirmoran commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 27, 2025

Uh oh!

shirmoran commented Nov 27, 2025

Uh oh!

Uh oh!

Uh oh!

aabughosh left a comment

Choose a reason for hiding this comment

Uh oh!

shirmoran commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 22, 2025

Uh oh!

oribon commented Dec 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shirmoran Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oribon Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shirmoran commented Nov 27, 2025 •

edited

Loading

shirmoran Dec 24, 2025 •

edited

Loading

oribon Dec 24, 2025 •

edited

Loading