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

Fix: temporarily ignore sanity checks when creating traceflow via UI #1097

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Fix: temporarily ignore sanity checks when creating traceflow via UI #1097

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 35 additions & 34 deletions plugins/octant/cmd/antrea-octant-plugin/traceflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,53 +112,55 @@ func (p *antreaOctantPlugin) actionHandler(request *service.ActionRequest) error

switch actionName {
case addTfAction:
// TODO Octant v0.13.1 does not support alerts, sending alerts is supported with Octant no earlier than v0.16.0.
// TODO After upgrading Octant, send alerts when one of the sanity checks for users' input fail to pass.
srcNamespace, err := request.Payload.String(srcNamespaceCol)
if err != nil {
log.Printf("Failed to get srcNamespace as string: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get srcNamespace as string: %s", err)
}
if match := regExpMatch(namespaceStrPattern, srcNamespace); !match {
log.Printf("Failed to match namespace %s with K8s Namespace pattern %s", srcNamespace, namespaceStrPattern)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to match namespace %s with K8s Namespace pattern %s", srcNamespace, namespaceStrPattern)
}

srcPod, err := request.Payload.String(srcPodCol)
if err != nil {
log.Printf("Failed to get srcPod as string: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get srcPod as string: %s", err)
}
if match := regExpMatch(podStrPattern, srcPod); !match {
log.Printf("Failed to match pod name %s with K8s Pod pattern %s", srcPod, podStrPattern)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to match pod name %s with K8s Pod pattern %s", srcPod, podStrPattern)
}

// Judge the destination type and get destination according to the type.
dstType, err := request.Payload.StringSlice(dstTypeCol)
if err != nil || len(dstType) == 0 {
log.Printf("Failed to get dstType as string slice: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get dstType as string slice: %s", err)
}
log.Printf("dstType %+v", dstType)
dst, err := request.Payload.String(dstCol)
if err != nil {
log.Printf("Failed to get dst as string: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get dst as string: %s", err)
}
dstNamespace, err := request.Payload.OptionalString(dstNamespaceCol)
if err != nil {
log.Printf("Failed to get dstNamespace as string: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get dstNamespace as string: %s", err)
}
var destination opsv1alpha1.Destination
switch dstType[0] {
case opsv1alpha1.DstTypePod:
if match := regExpMatch(podStrPattern, dst); !match {
log.Printf("Failed to match pod name %s with K8s Pod pattern %s", dst, podStrPattern)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to match pod name %s with K8s Pod pattern %s", dst, podStrPattern)
}
if match := regExpMatch(namespaceStrPattern, dstNamespace); !match {
log.Printf("Failed to match namespace %s with K8s Namespace pattern %s", dstNamespace, namespaceStrPattern)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to match namespace %s with K8s Namespace pattern %s", dstNamespace, namespaceStrPattern)
}
destination = opsv1alpha1.Destination{
Namespace: dstNamespace,
Expand All @@ -167,20 +169,20 @@ func (p *antreaOctantPlugin) actionHandler(request *service.ActionRequest) error
case opsv1alpha1.DstTypeIPv4:
s := net.ParseIP(dst)
if s == nil {
log.Printf("Failed to get destination IP as a valid IPv4 IP: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get destination IP as a valid IPv4 IP: %s", err)
}
if s.To4() == nil {
log.Printf("Failed to get destination IP as a valid IPv4 IP: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get destination IP as a valid IPv4 IP: %s", err)
}
destination = opsv1alpha1.Destination{
IP: dst,
}
case opsv1alpha1.DstTypeService:
if match := regExpMatch(namespaceStrPattern, dstNamespace); !match {
log.Printf("Failed to match namespace %s with K8s Namespace pattern %s", dstNamespace, namespaceStrPattern)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to match namespace %s with K8s Namespace pattern %s", dstNamespace, namespaceStrPattern)
}
destination = opsv1alpha1.Destination{
Namespace: dstNamespace,
Expand All @@ -190,19 +192,19 @@ func (p *antreaOctantPlugin) actionHandler(request *service.ActionRequest) error

srcPort, err := request.Payload.Uint16(srcPortCol)
if err != nil {
log.Printf("Failed to get srcPort as int: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get srcPort as int: %s", err)
}
dstPort, err := request.Payload.Uint16(dstPortCol)
if err != nil {
log.Printf("Failed to get dstPort as int: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get dstPort as int: %s", err)
}

protocol, err := request.Payload.StringSlice(protocolCol)
if err != nil || len(protocol) == 0 {
log.Printf("Failed to get protocol as string slice: %s", err)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
"failed to get protocol as string slice: %s", err)
}

// Judge whether the name of trace flow is duplicated.
Expand All @@ -211,9 +213,8 @@ func (p *antreaOctantPlugin) actionHandler(request *service.ActionRequest) error
ctx := context.Background()
tfOld, _ := p.client.OpsV1alpha1().Traceflows().Get(ctx, tfName, v1.GetOptions{})
if tfOld.Name == tfName {
log.Printf("Duplicate traceflow \"%s\": same source pod and destination pod in less than one second"+
": %+v. ", tfName, tfOld)
return nil
log.Printf("Invalid user input, CRD creation or Traceflow request may fail: "+
Copy link
Member

Choose a reason for hiding this comment

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

When iterating this feature, I suggest adding a suffix of a random string to save the step, or maybe we could just set "generateName" instead of "name" to ask apiserver to create a unique name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I fix it in this patch or fix it later?

"duplicate traceflow \"%s\": same source pod and destination pod in less than one second: %+v. ", tfName, tfOld)
}

tf := &opsv1alpha1.Traceflow{
Expand Down Expand Up @@ -281,7 +282,7 @@ func (p *antreaOctantPlugin) actionHandler(request *service.ActionRequest) error
case showGraphAction:
name, err := request.Payload.String(traceNameCol)
if err != nil {
log.Printf("Failed to get name at string : %w", err)
log.Printf("Failed to get name at string: %s", err)
return nil
Copy link
Member

Choose a reason for hiding this comment

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

If it fails to get the trace name, should it return? otherwise the graph would be wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't matter whether we return or not here. If the name is empty, then the plugin gets a traceflow CRD with all fields being empty value, and then generate an empty graph.

Copy link
Member

Choose a reason for hiding this comment

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

Even the result is same, the readability and maintainability is different, returning here ensures the following code has a valid traceflow name, otherwise people would need to worry what's the behavior if getting traceflow with an empty name, and what would the graph look like with an empty string.
I think this patch shouldn't make the change to worse the readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return nil added.

}
// Invoke GenGraph to show
Expand Down