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.

Sign up for GitHub

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

aws public subnet explainability #775

Merged
merged 8 commits into from
Aug 14, 2024
Merged

aws public subnet explainability #775

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ddd9e24
add explainability
haim-kermany Aug 12, 2024
f1ce4bb
mock
haim-kermany Aug 12, 2024
18c03be
IsPrivate
haim-kermany Aug 12, 2024
838df4f
not public
haim-kermany Aug 12, 2024
9a26ae2
small i
haim-kermany Aug 12, 2024
6ad743e
Merge branch 'main' into aws-explainability
haim-kermany Aug 13, 2024
1b747d4
Merge branch 'main' into aws-explainability
haim-kermany Aug 13, 2024
7ff64a0
Merge branch 'main' into aws-explainability
haim-kermany Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/awsvpc/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (rc *AWSresourcesContainer) getSubnetsConfig(
if err != nil {
return nil, err
}
subnet.SetIsPublic(*subnetObj.MapPublicIpOnLaunch)
subnet.SetIsPrivate(!*subnetObj.MapPublicIpOnLaunch)
}
return vpcInternalAddressRange, nil
}
Expand Down Expand Up @@ -398,7 +398,7 @@ func (rc *AWSresourcesContainer) getIgwConfig(
subnets := vpc.Subnets()
// only public subnets can be a subnet of igw:
subnets = slices.DeleteFunc(subnets, func(s *commonvpc.Subnet) bool {
return !s.IsPublic()
return s.IsPrivate()
})

if len(subnets) == 0 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/commonvpc/drawioGenerator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (s *Subnet) GenerateDrawioTreeNode(gen *vpcmodel.DrawioGenerator) drawio.Tr
zone, _ := s.Zone()
zoneTn := gen.TreeNode(zone).(*drawio.ZoneTreeNode)
subnetTn := drawio.NewSubnetTreeNode(zoneTn, s.Name(), s.Cidr, "")
subnetTn.SetIsPublic(s.isPublic)
subnetTn.SetIsPrivate(s.IsPrivate())
return subnetTn
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/commonvpc/vpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ type Subnet struct {
VPCnodes []vpcmodel.Node `json:"-"`
Cidr string `json:"-"`
IPblock *ipblock.IPBlock `json:"-"`
// isPublic is relevant only for aws, the user set for each subnet if it public (i.e. - has access to the internet)
isPublic bool `json:"-"`
// isPrivate is relevant only for aws, the user set for each subnet if it public (i.e. - has access to the internet)
isPrivate bool `json:"-"`
}

func (s *Subnet) CIDR() string {
Expand All @@ -144,11 +144,11 @@ func (s *Subnet) Nodes() []vpcmodel.Node {
func (s *Subnet) AddressRange() *ipblock.IPBlock {
return s.IPblock
}
func (s *Subnet) IsPublic() bool {
return s.isPublic
func (s *Subnet) IsPrivate() bool {
return s.isPrivate
}
func (s *Subnet) SetIsPublic(isPublic bool) {
s.isPublic = isPublic
func (s *Subnet) SetIsPrivate(isPrivate bool) {
s.isPrivate = isPrivate
}

type Vsi struct {
Expand Down
2 changes: 1 addition & 1 deletion pkg/drawio/drawio_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ func createNetworkAws() SquareTreeNodeInterface {
for i := 0; i < len(nis); i++ {
zone := NewZoneTreeNode(vpc, "zone1")
subnet := NewSubnetTreeNode(zone, "subnet2", "cidr1", "acl1")
subnet.SetIsPublic(i <= 1)
subnet.SetIsPrivate(i > 1)
nis[i] = NewNITreeNode(subnet, "ni20")
sg.AddIcon(nis[i])
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/drawio/squareTreeNode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ type SubnetTreeNode struct {
groupSquares []SquareTreeNodeInterface
cidr string
acl string
isPublic bool
isPrivate bool
}

func NewSubnetTreeNode(parent *ZoneTreeNode, name, cidr, acl string) *SubnetTreeNode {
Expand Down Expand Up @@ -273,8 +273,8 @@ func (tn *SubnetTreeNode) nonGroupingIcons() []IconTreeNodeInterface {
}
return nis
}
func (tn *SubnetTreeNode) IsPublic() bool { return tn.isPublic }
func (tn *SubnetTreeNode) SetIsPublic(isPublic bool) { tn.isPublic = isPublic }
func (tn *SubnetTreeNode) IsPrivate() bool { return tn.isPrivate }
func (tn *SubnetTreeNode) SetIsPrivate(isPrivate bool) { tn.isPrivate = isPrivate }

// /////////////////////////////////////////////////////////////////////////////////////
// GroupSquareTreeNode is a tree node that represents a group of icons that share the same connectivity
Expand Down
6 changes: 3 additions & 3 deletions pkg/drawio/styles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,10 @@ func (stl *templateStyles) representingType(tn TreeNodeInterface) reflect.Type {
if stl.provider != common.AWS || reflect.TypeOf(tn).Elem() != reflect.TypeOf(SubnetTreeNode{}) {
return reflect.TypeOf(tn).Elem()
}
if tn.(*SubnetTreeNode).IsPublic() {
return reflect.TypeOf(publicSubnetTreeNode{})
if tn.(*SubnetTreeNode).IsPrivate() {
return reflect.TypeOf(privateSubnetTreeNode{})
}
return reflect.TypeOf(privateSubnetTreeNode{})
return reflect.TypeOf(publicSubnetTreeNode{})
}

func (stl *templateStyles) Image(tn TreeNodeInterface) string {
Expand Down
1 change: 1 addition & 0 deletions pkg/vpcmodel/abstractVPC.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ type VPC interface {
type Subnet interface {
NodeSet
CIDR() string
IsPrivate() bool
}

// LoadBalancer is elaboration of a NodeSet - the nodes are the private IPs of the load balancer
Expand Down
13 changes: 13 additions & 0 deletions pkg/vpcmodel/explainabilityPrint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,13 @@ func respondDetailsHeader(d *detailedConn) string {
}
}

func isAtPrivateSubnet(endpoint EndpointElem) bool {
if internalNode, ok := endpoint.(InternalNodeIntf); ok {
return internalNode.Subnet().IsPrivate()
}
return false
}

// after all data is gathered, generates the actual string to be printed
func (g *groupedConnLine) explainPerCaseStr(c *VPCConfig, src, dst EndpointElem,
connQuery, crossVpcConnection *connection.Set, ingressBlocking, egressBlocking, loadBalancerBlocking bool,
Expand All @@ -206,6 +213,12 @@ func (g *groupedConnLine) explainPerCaseStr(c *VPCConfig, src, dst EndpointElem,
case crossVpcRouterRequired(src, dst) && crossVpcRouter != nil && crossVpcConnection.IsEmpty():
return fmt.Sprintf("%vAll connections will be blocked since transit gateway denies route from source to destination"+tripleNLVars,
noConnection, headerPlusPath, details)
case isAtPrivateSubnet(dst) && src.IsExternal():
Copy link
Contributor

Choose a reason for hiding this comment

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

The cases in which we provide a single line explaining why there is no connection should be minimized see #655. In this specific case we can provide full explainability and integrate the private subnet in the path and details.

We could defer the explanation part of this PR to latter PRs - I can take responsibility to this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will be resolved in another PR, by our explainability expert.
added a bullet to the relevant issue #756

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps better remove it all together

return fmt.Sprintf("%v\tthe subnet of %v is not public and does not enable inbound external connectivity\n",
noConnection, dst.Name())
case isAtPrivateSubnet(src) && dst.IsExternal():
return fmt.Sprintf("%v\tThe dst is external but the subnet of %v is not public\n",
noConnection, src.Name())
case externalRouter == nil && src.IsExternal():
return fmt.Sprintf("%v\tThere is no resource enabling inbound external connectivity\n", noConnection)
case externalRouter == nil && dst.IsExternal():
Expand Down
17 changes: 10 additions & 7 deletions pkg/vpcmodel/grouping_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ func (mockVPC *mockVPCIntf) GenerateDrawioTreeNode(gen *DrawioGenerator) drawio.
func (mockVPC *mockVPCIntf) ShowOnSubnetMode() bool { return true }

type mockNetIntf struct {
cidr string
isPublic bool
name string
subnet Subnet
cidr string
isExternal bool
name string
subnet Subnet
}

func (m *mockNetIntf) CidrOrAddress() string {
Expand All @@ -49,10 +49,10 @@ func (m *mockNetIntf) Subnet() Subnet {
}

func (m *mockNetIntf) IsInternal() bool {
return !m.isPublic
return !m.isExternal
}
func (m *mockNetIntf) IsPublicInternet() bool {
return m.isPublic
return m.isExternal
}
func (m *mockNetIntf) AbstractedToNodeSet() NodeSet {
return nil
Expand Down Expand Up @@ -84,7 +84,7 @@ func (m *mockNetIntf) RegionName() string {
func (m *mockNetIntf) GenerateDrawioTreeNode(gen *DrawioGenerator) drawio.TreeNodeInterface {
return nil
}
func (m *mockNetIntf) IsExternal() bool { return m.isPublic }
func (m *mockNetIntf) IsExternal() bool { return m.isExternal }
func (m *mockNetIntf) ShowOnSubnetMode() bool { return false }

func (m *mockNetIntf) VPC() VPCResourceIntf {
Expand Down Expand Up @@ -123,6 +123,9 @@ func (m *mockSubnet) AddressRange() *ipblock.IPBlock {
func (m *mockSubnet) CIDR() string {
return m.cidr
}
func (m *mockSubnet) IsPrivate() bool {
return true
}
func (m *mockSubnet) Connectivity() *ConnectivityResult {
return nil
}
Expand Down
32 changes: 16 additions & 16 deletions pkg/vpcmodel/semanticDiff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,17 @@ func configSimpleIPAndSubnetDiff() (subnetConfigConn1, subnetConfigConn2 *config
cfg1.Subnets = append(cfg1.Subnets, &mockSubnet{nil, "10.1.20.0/22", "subnet1", nil},
&mockSubnet{nil, "10.2.20.0/22", "subnet2", nil})
cfg1.Nodes = append(cfg1.Nodes,
&mockNetIntf{cidr: "1.2.3.0/30", name: "public1-1", isPublic: true},
&mockNetIntf{cidr: "250.2.4.0/24", name: "public1-2", isPublic: true},
&mockNetIntf{cidr: "200.2.4.0/24", name: "public1-3", isPublic: true})
&mockNetIntf{cidr: "1.2.3.0/30", name: "public1-1", isExternal: true},
&mockNetIntf{cidr: "250.2.4.0/24", name: "public1-2", isExternal: true},
&mockNetIntf{cidr: "200.2.4.0/24", name: "public1-3", isExternal: true})

cfg2 := &VPCConfig{}
cfg2.Subnets = append(cfg2.Subnets, &mockSubnet{nil, "10.1.20.0/22", "subnet1", nil},
&mockSubnet{nil, "10.2.20.0/22", "subnet2", nil})
cfg2.Nodes = append(cfg2.Nodes,
&mockNetIntf{cidr: "1.2.3.0/26", name: "public2-1", isPublic: true},
&mockNetIntf{cidr: "250.2.4.0/30", name: "public2-2", isPublic: true},
&mockNetIntf{cidr: "200.2.4.0/24", name: "public1-3", isPublic: true})
&mockNetIntf{cidr: "1.2.3.0/26", name: "public2-1", isExternal: true},
&mockNetIntf{cidr: "250.2.4.0/30", name: "public2-2", isExternal: true},
&mockNetIntf{cidr: "200.2.4.0/24", name: "public1-3", isExternal: true})

// cfg1 cfg2
// <subnet2, public1-1> and <subnet2, public2-1> are comparable
Expand Down Expand Up @@ -272,22 +272,22 @@ func TestSimpleIPAndSubnetDiffGrouping(t *testing.T) {
func configSimpleVsisDiff() (configConn1, configConn2 *configConnectivity) {
cfg1 := &VPCConfig{}
cfg1.Nodes = append(cfg1.Nodes,
&mockNetIntf{name: "vsi0", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi1", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi2", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi3", isPublic: false, cidr: ""},
&mockNetIntf{cidr: "1.2.3.0/30", name: "public1-1", isPublic: true})
&mockNetIntf{name: "vsi0", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi1", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi2", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi3", isExternal: false, cidr: ""},
&mockNetIntf{cidr: "1.2.3.0/30", name: "public1-1", isExternal: true})

cfg1.Subnets = append(cfg1.Subnets, &mockSubnet{nil, "10.0.20.0/22", "subnet0", []Node{cfg1.Nodes[0], cfg1.Nodes[1],
cfg1.Nodes[2], cfg1.Nodes[3]}})

cfg2 := &VPCConfig{}
cfg2.Nodes = append(cfg2.Nodes,
&mockNetIntf{name: "vsi1", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi2", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi3", isPublic: false, cidr: ""},
&mockNetIntf{name: "vsi4", isPublic: false, cidr: ""},
&mockNetIntf{cidr: "1.2.3.0/26", name: "public2-1", isPublic: true})
&mockNetIntf{name: "vsi1", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi2", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi3", isExternal: false, cidr: ""},
&mockNetIntf{name: "vsi4", isExternal: false, cidr: ""},
&mockNetIntf{cidr: "1.2.3.0/26", name: "public2-1", isExternal: true})

cfg2.Subnets = append(cfg2.Subnets, &mockSubnet{nil, "10.0.20.0/22", "subnet0", []Node{cfg2.Nodes[0], cfg2.Nodes[1],
cfg2.Nodes[2], cfg2.Nodes[3]}})
Expand Down