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

improve grouping with self-loops #98

Closed
adisos opened this issue Jul 6, 2023 · 5 comments
Closed

improve grouping with self-loops #98

adisos opened this issue Jul 6, 2023 · 5 comments
Assignees

Comments

@adisos
Copy link
Collaborator

adisos commented Jul 6, 2023

          ```suggestion

vsi3a-ky[10.240.30.5],vsi3b-ky[10.240.30.6],vsi3c-ky[10.240.30.4] => vsi3a-ky[10.240.30.5],vsi3b-ky[10.240.30.6],vsi3c-ky[10.240.30.4] : All Connections
vsi3a-ky[10.240.30.5],vsi3b-ky[10.240.30.6],vsi3c-ky[10.240.30.4] => vsi1-ky[10.240.10.4] : All Connections

This requires adding a self loop to each VSI, run the grouping algorithm, then remove left-over self loops.
Can be handled in a future PR.

_Originally posted by @zivnevo in https://github.com/np-guard/vpc-network-config-analyzer/pull/92#discussion_r1253989708_
            
@adisos adisos self-assigned this Jul 10, 2023
@ShiriMoran ShiriMoran assigned ShiriMoran and unassigned adisos Aug 15, 2023
@ShiriMoran
Copy link
Contributor

This requires adding a self loop to each VSI, run the grouping algorithm, then remove left-over self loops.
Can be handled in a future PR.

_Originally posted by @zivnevo in https://github.com/np-guard/vpc-network-config-analyzer/pull/92#discussion_r1253989708_
            

@zivnevo adding a self loop before running the algorithm seems wrong.
Consider subnets a,b,c,d with the following connections:

a => b
a => c
d => c
d => b

With the current algorithm, after grouping src we'll get

a => b,c
d => b,c

and after grouping dst on the result we'll get
a,d => b,c

but if we add self loops before running the alg after grouping src we'll get:

a => a,b,c
d => b,c,d

and grouping dst will group nothing.

I'm working on a different solution.

@zivnevo
Copy link
Member

zivnevo commented Aug 21, 2023

My initial thought was to try both with and without self-loops and see which gives better results. Another option is to add the self loops just after some initial grouping was done.
If you have a one-shot idea, this might be better. Please share you algorithm here.

@ShiriMoran
Copy link
Contributor

ShiriMoran commented Aug 21, 2023

Actually I'm afraid that if we choose the direction of adding self loops, grouping and comparing than what is really required is to check all the possible combinations of subnets/vsis with and without self loops, which is not reasonable for a couple of reasons.
E.g., consider the following example:

a => b
a => c
b => c

To gain the optimal grouping a self loop should be added only to b, which yields
a, b => b, c

I have another solution in mind, if you wish we can discuss or I can put it here before I write the code.

@ShiriMoran
Copy link
Contributor

ShiriMoran commented Aug 21, 2023

Proposed algorithm:

  • execute the current grouping algorithm for grouping, assume w.l.o.g we grouped destinations
  • For each two lines, compute their distance
  • merge all lines whose distance is an empty set; the claim below guarantees that the result is coherent

The distance between two GroupedConnLine is defined as following:
Let l_1 be a line with source s_1 and dest d_1 and let l_2 be a line with source s_2 and dest d_2.
l_1 / l_2 is the subnets in d_1 that are not in d_2 minus the single subnet in s_1 if |s_1| = 1

The distance between lines l_1 and l_2 is l_1 / l_2 union l_2 / l_2

claim: if the distance between line l_1 and l_2 is empty and the distance between lines l_2 and l_3 is zero then so is the distance between l_1 and l_3

@ShiriMoran
Copy link
Contributor

solved by #152

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

No branches or pull requests

3 participants