Skip to content

Commit

Permalink
Rename diff args new (#268)
Browse files Browse the repository at this point in the history 
rename DiffAnalyzer args from dir1/dir2 to ref1/ref2 + add API option WithArgNames

Signed-off-by: adisos <[email protected]>
Co-authored-by: Ziv Nevo <[email protected]>
  • Loading branch information
@adisos @zivnevo
adisos and zivnevo committed Nov 9, 2023
1 parent e1f6b13 commit 2392bbc
Show file tree
Hide file tree
Showing 61 changed files with 5,390 additions and 5,246 deletions.
18 changes: 9 additions & 9 deletions cmd/netpolicy/cmd/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,13 +384,13 @@ func TestCommands(t *testing.T) {
},
expectedOutput: "Connectivity diff:\n" +
"diff-type: added, source: 0.0.0.0-255.255.255.255, destination: default/unicorn[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/redis-cart[Deployment], destination: default/unicorn[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/unicorn[Deployment], destination: 0.0.0.0-255.255.255.255, dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/unicorn[Deployment], destination: default/redis-cart[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added",
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added",
exact: true,
isErr: false,
},
Expand All @@ -409,13 +409,13 @@ func TestCommands(t *testing.T) {
},
expectedOutput: "Connectivity diff:\n" +
"diff-type: added, source: 0.0.0.0-255.255.255.255, destination: default/unicorn[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/redis-cart[Deployment], destination: default/unicorn[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/unicorn[Deployment], destination: 0.0.0.0-255.255.255.255, dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added\n" +
"diff-type: added, source: default/unicorn[Deployment], destination: default/redis-cart[Deployment], dir1:" +
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added",
" No Connections, dir2: All Connections, workloads-diff-info: workload default/unicorn[Deployment] added",
exact: true,
isErr: false,
hasFile: true,
Expand Down Expand Up @@ -516,7 +516,7 @@ func TestCommands(t *testing.T) {
filepath.Join(getTestsDir(), "onlineboutique_with_pods_severe_error")},
expectedOutput: "Connectivity diff:\n" +
"diff-type: changed, source: default/frontend-99684f7f8[ReplicaSet], " +
"destination: default/adservice-77d5cd745d[ReplicaSet], dir1: TCP 9555, dir2: TCP 8080",
"destination: default/adservice-77d5cd745d[ReplicaSet], dir1: TCP 9555, dir2: TCP 8080",
exact: true,
isErr: false,
},
Expand Down
11 changes: 8 additions & 3 deletions cmd/netpolicy/cmd/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ var (
outFormat string
)

const (
dir1Arg = "dir1"
dir2Arg = "dir2"
)

func runDiffCommand() error {
var connsDiff diff.ConnectivityDiff
var err error
Expand All @@ -55,7 +60,7 @@ func runDiffCommand() error {
}

func getDiffOptions(l *logger.DefaultLogger) []diff.DiffAnalyzerOption {
res := []diff.DiffAnalyzerOption{diff.WithLogger(l), diff.WithOutputFormat(outFormat)}
res := []diff.DiffAnalyzerOption{diff.WithLogger(l), diff.WithOutputFormat(outFormat), diff.WithArgNames(dir1Arg, dir2Arg)}
if stopOnFirstError {
res = append(res, diff.WithStopOnError())
}
Expand Down Expand Up @@ -93,8 +98,8 @@ func newCommandDiff() *cobra.Command {
}

// define any flags and configuration settings.
c.Flags().StringVarP(&dir1, "dir1", "", "", "Original Resources path to be compared")
c.Flags().StringVarP(&dir2, "dir2", "", "", "New Resources path to compare with original resources path")
c.Flags().StringVarP(&dir1, dir1Arg, "", "", "Original Resources path to be compared")
c.Flags().StringVarP(&dir2, dir2Arg, "", "", "New Resources path to compare with original resources path")
supportedDiffFormats := strings.Join(diff.ValidDiffFormats, ",")
c.Flags().StringVarP(&outFormat, "output", "o", common.DefaultFormat, getOutputFormatDescription(supportedDiffFormats))
// out file
Expand Down
26 changes: 13 additions & 13 deletions pkg/netpol/diff/connectivity_diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ import (

// ConnectivityDiff captures the set of differences in terms of connectivity between two input k8s resource sets
type ConnectivityDiff interface {
// RemovedConnections is a list of differences where the specified conn only exists in dir1
// RemovedConnections is a list of differences where the specified conn only exists in ref1
RemovedConnections() []SrcDstDiff

// AddedConnections is a list of differences where the specified conn only exists in dir2
// AddedConnections is a list of differences where the specified conn only exists in ref2
AddedConnections() []SrcDstDiff

// ChangedConnections is a list of differences where the specified conn exists in dir1 and dir2 but not identical
// ChangedConnections is a list of differences where the specified conn exists in ref1 and ref2 but not identical
// connection properties
ChangedConnections() []SrcDstDiff

// UnchangedConnections is a list of connections that exists in dir1 and dir2, and are identical
// UnchangedConnections is a list of connections that exists in ref1 and ref2, and are identical
UnchangedConnections() []SrcDstDiff

// IsEmpty returns true if there is no diff in connectivity, i.e. removed, added and changed connections are empty
Expand All @@ -32,17 +32,17 @@ type SrcDstDiff interface {
Src() Peer
// Dst returns the destination peer
Dst() Peer
// Dir1Connectivity returns the AllowedConnectivity from src to dst in dir1
Dir1Connectivity() AllowedConnectivity
// Dir2Connectivity returns the AllowedConnectivity from src to dst in dir2
Dir2Connectivity() AllowedConnectivity
// IsSrcNewOrRemoved returns true if the src peer exists only in dir2 (if DiffType is Added) or if
// the src peer exists only in dir1 (if DiffType is Removed)
// Ref1Connectivity returns the AllowedConnectivity from src to dst in ref1
Ref1Connectivity() AllowedConnectivity
// Ref2Connectivity returns the AllowedConnectivity from src to dst in ref2
Ref2Connectivity() AllowedConnectivity
// IsSrcNewOrRemoved returns true if the src peer exists only in ref2 (if DiffType is Added) or if
// the src peer exists only in ref1 (if DiffType is Removed)
IsSrcNewOrRemoved() bool
// IsDstNewOrRemoved returns true if the dst peer exists only in dir2 (if DiffType is Added) or if
// the dst peer exists only in dir1 (if DiffType is Removed)
// IsDstNewOrRemoved returns true if the dst peer exists only in ref2 (if DiffType is Added) or if
// the dst peer exists only in ref1 (if DiffType is Removed)
IsDstNewOrRemoved() bool
// DiffType returns the diff type of dir2 w.r.t dir1, which can be ChangedType/RemovedType/AddedType/NonChangedType
// DiffType returns the diff type of ref2 w.r.t ref1, which can be ChangedType/RemovedType/AddedType/UnchangedType
DiffType() DiffTypeStr
}

Expand Down
69 changes: 47 additions & 22 deletions pkg/netpol/diff/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package diff

import (
"errors"
"fmt"
"os"

v1 "k8s.io/api/core/v1"
Expand All @@ -29,20 +30,22 @@ type DiffAnalyzer struct {
stopOnError bool
errors []DiffError
outputFormat string
ref1Name string
ref2Name string
}

// ConnDiffFromResourceInfos returns the connectivity diffs from two lists of resource.Info objects,
// representing two versions of manifest sets to compare
func (da *DiffAnalyzer) ConnDiffFromResourceInfos(infos1, infos2 []*resource.Info) (ConnectivityDiff, error) {
// connectivity analysis for first dir
// TODO: should add input arg dirPath to this API func? so that log msgs can specify the dir, rather then just "dir1"/"dir2"
conns1, workloads1, shouldStop, cDiff, errVal := da.getConnlistAnalysis(infos1, true, false, "")
// TODO: should add input arg dirPath to this API func? so that log msgs can specify the dir, rather then just "ref1"/"ref2"
conns1, workloads1, shouldStop, cDiff, errVal := da.getConnlistAnalysis(infos1, true, "")
if shouldStop {
return cDiff, errVal
}

// connectivity analysis for second dir
conns2, workloads2, shouldStop, cDiff, errVal := da.getConnlistAnalysis(infos2, false, true, "")
conns2, workloads2, shouldStop, cDiff, errVal := da.getConnlistAnalysis(infos2, false, "")
if shouldStop {
return cDiff, errVal
}
Expand All @@ -65,7 +68,7 @@ func (da *DiffAnalyzer) ConnDiffFromDirPaths(dirPath1, dirPath2 string) (Connect
if len(errs1) == 0 {
dirPath = dirPath2
}
da.logger.Errorf(err, "Error getting resourceInfos from dir paths dir1/dir2 ")
da.logger.Errorf(err, "Error getting resourceInfos from dir paths %s/%s ", da.ref1Name, da.ref2Name)
da.errors = append(da.errors, parser.FailedReadingFile(dirPath, err))
return nil, err // return as fatal error if both infos-lists are empty, or if stopOnError is on,
// or if at least one input dir does not exist
Expand All @@ -74,12 +77,12 @@ func (da *DiffAnalyzer) ConnDiffFromDirPaths(dirPath1, dirPath2 string) (Connect
// split err if it's an aggregated error to a list of separate errors
errReadingFile := "error reading file"
for _, err := range errs1 {
da.logger.Errorf(err, atDir1Prefix+errReadingFile) // print to log the error from builder
da.logger.Errorf(err, da.errPrefixSpecifyRefName(true)+errReadingFile) // print to log the error from builder
da.errors = append(da.errors, parser.FailedReadingFile(dirPath1, err)) // add the error from builder to accumulated errors
}
for _, err := range errs2 {
da.logger.Errorf(err, atDir2Prefix+errReadingFile) // print to log the error from builder
da.errors = append(da.errors, parser.FailedReadingFile(dirPath2, err)) // add the error from builder to accumulated errors
da.logger.Errorf(err, da.errPrefixSpecifyRefName(false)+errReadingFile) // print to log the error from builder
da.errors = append(da.errors, parser.FailedReadingFile(dirPath2, err)) // add the error from builder to accumulated errors
}
}
return da.ConnDiffFromResourceInfos(infos1, infos2)
Expand Down Expand Up @@ -161,6 +164,15 @@ func WithStopOnError() DiffAnalyzerOption {
}
}

// WithArgNames is a functional option that sets the names to be used for the two sets of analyzed resources
// (default is ref1,ref2) in the output reports and log messages.
func WithArgNames(ref1Name, ref2Name string) DiffAnalyzerOption {
return func(d *DiffAnalyzer) {
d.ref1Name = ref1Name
d.ref2Name = ref2Name
}
}

// NewDiffAnalyzer creates a new instance of DiffAnalyzer, and applies the provided functional options.
func NewDiffAnalyzer(options ...DiffAnalyzerOption) *DiffAnalyzer {
// object with default behavior options
Expand All @@ -169,6 +181,8 @@ func NewDiffAnalyzer(options ...DiffAnalyzerOption) *DiffAnalyzer {
stopOnError: false,
errors: []DiffError{},
outputFormat: common.DefaultFormat,
ref1Name: "ref1",
ref2Name: "ref2",
}
for _, o := range options {
o(da)
Expand Down Expand Up @@ -203,7 +217,7 @@ func (da *DiffAnalyzer) hasFatalError() error {
}

// return a []ConnlistAnalyzerOption with mute errs/warns, so that logging of err/wanr is not duplicated, and
// added to log only by getConnlistAnalysis function, which adds the context of dir1/dir2
// added to log only by getConnlistAnalysis function, which adds the context of ref1/ref2
func (da *DiffAnalyzer) determineConnlistAnalyzerOptions() []connlist.ConnlistAnalyzerOption {
if da.stopOnError {
return []connlist.ConnlistAnalyzerOption{connlist.WithMuteErrsAndWarns(), connlist.WithLogger(da.logger), connlist.WithStopOnError()}
Expand All @@ -220,8 +234,7 @@ func (da *DiffAnalyzer) determineConnlistAnalyzerOptions() []connlist.ConnlistAn
// the main function, if the analysis should stop.
func (da *DiffAnalyzer) getConnlistAnalysis(
infos []*resource.Info,
dir1 bool,
dir2 bool,
isRef1 bool,
dirPath string) (
[]connlist.Peer2PeerConnection,
[]connlist.Peer,
Expand All @@ -233,9 +246,10 @@ func (da *DiffAnalyzer) getConnlistAnalysis(
conns, workloads, err := connlistaAnalyzer.ConnlistFromResourceInfos(infos)

// append all fatal/severe errors and warnings returned by connlistaAnalyzer
errPrefix := da.errPrefixSpecifyRefName(isRef1)
for _, e := range connlistaAnalyzer.Errors() {
// wrap err/warn with new err type that includes context of dir1/dir2
daErr := newConnectivityAnalysisError(e.Error(), dir1, dir2, dirPath, e.IsSevere(), e.IsFatal())
// wrap err/warn with new err type that includes context of ref1/ref2
daErr := newConnectivityAnalysisError(e.Error(), errPrefix, dirPath, e.IsSevere(), e.IsFatal())
da.errors = append(da.errors, daErr)
logErrOrWarning(daErr, da.logger)
}
Expand All @@ -244,7 +258,7 @@ func (da *DiffAnalyzer) getConnlistAnalysis(
// check it exists, if not, append a new fatal err to the da.errors array
if da.hasFatalError() == nil {
// append the fatal error (indicates an issue in connlist analyzer, that did not append this err as expected)
da.errors = append(da.errors, newConnectivityAnalysisError(err, dir1, dir2, dirPath, false, true))
da.errors = append(da.errors, newConnectivityAnalysisError(err, errPrefix, dirPath, false, true))
}
}

Expand All @@ -264,6 +278,17 @@ func (da *DiffAnalyzer) getConnlistAnalysis(
return conns, workloads, shouldStop, cDiff, errVal
}

func (da *DiffAnalyzer) errPrefixSpecifyRefName(isRef1 bool) string {
if isRef1 {
return getErrPrefix(da.ref1Name)
}
return getErrPrefix(da.ref2Name)
}

func getErrPrefix(location string) string {
return fmt.Sprintf("at %s: ", location)
}

func logErrOrWarning(d DiffError, l logger.Logger) {
if d.IsSevere() || d.IsFatal() {
l.Errorf(d.Error(), "")
Expand Down Expand Up @@ -351,7 +376,7 @@ func (c *connsPair) Dst() Peer {
return c.firstConn.Dst()
}

func (c *connsPair) Dir1Connectivity() AllowedConnectivity {
func (c *connsPair) Ref1Connectivity() AllowedConnectivity {
if c.diffType == AddedType {
return &allowedConnectivity{
allProtocolsAndPorts: false,
Expand All @@ -364,7 +389,7 @@ func (c *connsPair) Dir1Connectivity() AllowedConnectivity {
}
}

func (c *connsPair) Dir2Connectivity() AllowedConnectivity {
func (c *connsPair) Ref2Connectivity() AllowedConnectivity {
if c.diffType == RemovedType {
return &allowedConnectivity{
allProtocolsAndPorts: false,
Expand Down Expand Up @@ -689,7 +714,7 @@ func (da *DiffAnalyzer) ConnectivityDiffToString(connectivityDiff ConnectivityDi
return "", nil
}
da.logger.Infof("Found connections diffs")
diffFormatter, err := getFormatter(da.outputFormat)
diffFormatter, err := getFormatter(da.outputFormat, da.ref1Name, da.ref2Name)
if err != nil {
da.errors = append(da.errors, newResultFormattingError(err))
return "", err
Expand All @@ -703,21 +728,21 @@ func (da *DiffAnalyzer) ConnectivityDiffToString(connectivityDiff ConnectivityDi
}

// returns the relevant formatter for the analyzer's outputFormat
func getFormatter(format string) (diffFormatter, error) {
func getFormatter(format, ref1Name, ref2Name string) (diffFormatter, error) {
if err := ValidateDiffOutputFormat(format); err != nil {
return nil, err
}
switch format {
case common.TextFormat:
return &diffFormatText{}, nil
return &diffFormatText{ref1: ref1Name, ref2: ref2Name}, nil
case common.CSVFormat:
return &diffFormatCSV{}, nil
return &diffFormatCSV{ref1: ref1Name, ref2: ref2Name}, nil
case common.MDFormat:
return &diffFormatMD{}, nil
return &diffFormatMD{ref1: ref1Name, ref2: ref2Name}, nil
case common.DOTFormat:
return &diffFormatDOT{}, nil
return &diffFormatDOT{ref1: ref1Name, ref2: ref2Name}, nil
default:
return &diffFormatText{}, nil
return &diffFormatText{ref1: ref1Name, ref2: ref2Name}, nil
}
}

Expand Down
24 changes: 8 additions & 16 deletions pkg/netpol/diff/diff_errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ type handlingIPpeersError struct {
}

type connectivityAnalysisError struct {
origErr error
dir1 bool
dir2 bool
dirPath string
origErr error
errPrefix string
dirPath string
}

///////////////////////////
Expand Down Expand Up @@ -64,20 +63,13 @@ func (e *handlingIPpeersError) Error() string {
return e.origErr.Error()
}

const (
atDir1Prefix = "at dir1: "
atDir2Prefix = "at dir2: "
)

func (e *connectivityAnalysisError) Error() string {
var prefix string
switch {
case e.dirPath != "":
prefix = "at " + e.dirPath + ": "
case e.dir1:
prefix = atDir1Prefix
case e.dir2:
prefix = atDir2Prefix
prefix = getErrPrefix(e.dirPath)
case e.errPrefix != "": // prefix of ref1/ref2 names
prefix = e.errPrefix
}
return prefix + e.origErr.Error()
}
Expand All @@ -91,7 +83,7 @@ func newHandlingIPpeersError(err error) *diffGeneratingError {
return &diffGeneratingError{err: &handlingIPpeersError{err}, fatal: true, severe: false}
}

func newConnectivityAnalysisError(err error, dir1, dir2 bool, dirPath string, isSevere, isFatal bool) *diffGeneratingError {
func newConnectivityAnalysisError(err error, errPrefix, dirPath string, isSevere, isFatal bool) *diffGeneratingError {
return &diffGeneratingError{err: &connectivityAnalysisError{
origErr: err, dir1: dir1, dir2: dir2, dirPath: dirPath}, fatal: isFatal, severe: isSevere}
origErr: err, errPrefix: errPrefix, dirPath: dirPath}, fatal: isFatal, severe: isSevere}
}
Loading

0 comments on commit 2392bbc

Please sign in to comment.