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

818 rule str format #826

Merged
merged 75 commits into from
Aug 22, 2024
Merged

818 rule str format #826

merged 75 commits into from
Aug 22, 2024

Conversation

ShiriMoran
Copy link
Contributor

No description provided.

Comment on lines +18 to +20
direction: outbound, id: id:152, remote: 0.0.0.0/0, local: 0.0.0.0/0, conns: protocol: all
network ACL acl2-ky allows connection with the following allow rules
index: 2, direction: outbound , src: 10.240.20.0/24 , dst: 10.240.10.0/24, conn: all, action: allow
direction: outbound, name: acl2-out-3, priority: 3, action: allow, source: 10.240.20.0/24 , destination: 10.240.10.0/24, conn: all
Copy link
Member

Choose a reason for hiding this comment

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

In SG we have conns: protocol: all, while in NACL we have conn: all.
Better be consistent (I prefer traffic: all for both)

Copy link
Contributor Author

@ShiriMoran ShiriMoran Aug 22, 2024

Choose a reason for hiding this comment

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

This is not related to this PR.
It has to do with the following workaround temp code from detailedConn.go (which seems not to work in all cases)
Lets open a separate issue and handle it properly in connectionSet

// string adds * to non-responsive TCP components of the connection
// for cosmetic reasons remove the protocol word from cubes prints
func (d *detailedConn) string() string {
...
	// todo: remove "protocol" from the original cube printing funcs
	return strings.ReplaceAll(strings.Join(resStrSlice, "; "), "protocol: ", "")
}

Copy link
Member

Choose a reason for hiding this comment

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

ok. @haim-kermany didn't you implement some shortened version of connection details for drawio?

Copy link
Contributor

Choose a reason for hiding this comment

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

not anything sophisticated:
func (data *templateData) SvgShortLabel(tn TreeNodeInterface) string {
// the connection label is created in another package,
// so, instead of creating a short version, we edit the long version here:
label := data.SvgLabel(tn)
label = strings.ReplaceAll(label, "protocol:", "")
if !strings.Contains(label, "src-ports:") {
label = strings.ReplaceAll(label, "dst-ports:", "")
}
if len(label) > maxConnLabelSize {
return label[0:maxConnLabelSize-len(threeDots)] + threeDots
}
return label
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of having ad-hoc code in multiple places as we do now, lets have the proper printing function in connectionSet
(My code is similar to Haim's and for some reason it does not work in all cases. I thing debugging this ad-hoc string oriented ad-hoc code is a waste of time -better fix it properly)

Copy link
Member

Choose a reason for hiding this comment

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

Please open an issue in models repo

Copy link
Contributor Author

@ShiriMoran ShiriMoran Aug 22, 2024

Choose a reason for hiding this comment

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

you already did here

@ShiriMoran ShiriMoran requested a review from zivnevo August 22, 2024 05:43
@ShiriMoran ShiriMoran merged commit 4d4f27f into main Aug 22, 2024
4 checks passed
@ShiriMoran ShiriMoran deleted the 818_ruleStr_format branch August 22, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove rule index from explainability & lint, use rule name
3 participants