search

np-guard / vpc-network-config-analyzer

A tool for analyzing the configured network connectivity of VPCs as specified by various VPC resources
Apache License 2.0
7 stars 0 forks source link

Explain: improvements in messages #541

Closed kyorav closed 5 months ago

kyorav commented 5 months ago

A few comments on wording:

Originally posted by @kyorav in https://github.com/np-guard/vpc-network-config-analyzer/issues/538#issuecomment-2061187132

ShiriMoran commented 5 months ago

A few comments on wording:

* Avoid using language that refers to the code. Examples I encountered:
  "ignoring floatingIP public-gw1: target.ResourceType public_gateway is not supported (only networkInterfaceResourceType supported)". I don't understand the warning (I did not use the name of a public gateway as src or dst so what is this warning about?) so I can't suggest a new phrasing. However, none of the following should be used: floatingIP, target.ResourceType, networkInterfaceResourceType

This specific message has nothing to do with explainabiliy - it is printed as part of the parsing, I believe. Perhaps we should open another issue for this - also maybe avoid printing it e.g. in explainability @adisos

adisos commented 5 months ago

A few comments on wording:

* Avoid using language that refers to the code. Examples I encountered:
  "ignoring floatingIP public-gw1: target.ResourceType public_gateway is not supported (only networkInterfaceResourceType supported)". I don't understand the warning (I did not use the name of a public gateway as src or dst so what is this warning about?) so I can't suggest a new phrasing. However, none of the following should be used: floatingIP, target.ResourceType, networkInterfaceResourceType

This specific message has nothing to do with explainabiliy - it is printed as part of the parsing, I believe. Perhaps we should open another issue for this - also maybe avoid printing it e.g. in explainability @adisos

This is indeed not part of explainability output. It's a warning (printed by the logger, while parsing) , for certain resources being ignored/skipped from the input config object.

If you run with -quiet , such warnings will not be printed.

ShiriMoran commented 5 months ago

A few comments on wording:

* Do not print CRN values in the middle of a sentence because real CRNs are very long. Instead, print the error message and then add: "The CRN of the offending resources: \n \t [CRN1] \n \t [CRN2] \n"

Done

* Avoid using language that refers to the code. Examples I encountered:
  "ignoring floatingIP public-gw1: target.ResourceType public_gateway is not supported (only networkInterfaceResourceType supported)". I don't understand the warning (I did not use the name of a public gateway as src or dst so what is this warning about?) so I can't suggest a new phrasing. However, none of the following should be used: floatingIP, target.ResourceType, networkInterfaceResourceType

Please be more specific here @kyorav as said the above example is not relevant to explainability

* "The following connection exists between vsi1[10.240.10.4] and vsi2[10.240.20.4]: All Connections" -- replace "All Connections" with "all connection types" or "all protocols" (in this and all other relevant scenarios where this phrase is used).

See comment below

* Replace the following:
No connection between private-2-instance[192.168.8.4] and Public Internet 161.26.0.0/16;
no fip/pgw and dst is external

with:

No connection between private-2-instance[192.168.8.4] and Public Internet 161.26.0.0/16; 
    The dst is external but there is no Floating IP or Public Gateway connecting to public internet

Done

* Replace the following:
illegal src: in combined-vpc-local-tg1 there is more than one resource (crn:467, crn:521) with the given input string vsi1-subnet2. can not determine which resource to analyze. consider using unique names or use input UID instead.

with:

ambiguous src: the configuration contains multiple resources named vsi1-subnet2, try using the VPC name to scope resources: vpc-name/instance-name

Done (partly - adding ambiguous before source is too much trouble, so the "ambiguous" is elsewhere in the message)

Do not use "combined-vpc-local-tg1" in messages, this is an internal entity.

Kept in a single place titled "no-guard error" - should never occur - but if it does then it is an internal np-guard error with error message designed for "us"

* Replace the following:
src test-vpc2/vsi1-subnet2 and dst test-vpc0/vsi0-subnet0 connected by more than one transit-gateway - local-tg1, local-tg2 - this usecase is not supported, yet.

with

The src and dst are in separate VPCs connected by multiple transit gateways (local-tg1, local-tg2). This scenario is currently not supported.

Done

Originally posted by @kyorav in #538 (comment)

ShiriMoran commented 5 months ago 
* "The following connection exists between vsi1[10.240.10.4] and vsi2[10.240.20.4]: All Connections" -- replace "All Connections" with "all connection types" or "all protocols" (in this and all other relevant scenarios where this phrase is used).

This wording is not mine - its from func (c *Set) String() string of connectionSet of package connection and its also used in other places - e.g. the basic connectivity report. If it is to be changed its should be changed there, with a sepreate issue and PR. Probobably not for V0.4 @kyorav @adisos @zivnevo