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

Sanitize Telemetry data #4758

Merged

Conversation

valaparthvi
Copy link
Contributor

What type of PR is this?
/kind bug

What does this PR do / why we need it:
This PR sanitizes some more data that might have some potential PII data.

Which issue(s) this PR fixes:

Fixes part of #4462

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:
Running unit tests should be fine or perhaps test with data that potentially contains personal data.

@valaparthvi valaparthvi requested a review from kadel May 31, 2021 12:12
@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label May 31, 2021
@openshift-ci openshift-ci bot requested review from dharmit and mohammedzee1000 May 31, 2021 12:12
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 31, 2021
@valaparthvi valaparthvi force-pushed the sanitization branch 2 times, most recently from e85f5e8 to 4a4963c Compare June 1, 2021 06:28

// sanitizeURL sanitizes URLs from the error string
func sanitizeURL(errString string) string {
urlPattern, _ := regexp.Compile(`(http)\S*`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider a regex that matches a URL that might not have prefix "http" as well?
https://stackoverflow.com/questions/42618872/regex-for-website-or-url-validation/42619368

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 7, 2021
@valaparthvi
Copy link
Contributor Author

test /psi-unit-test-windows

[ssh:Windows 10]                             
INFO[2021-06-07T10:06:23Z]                                              
INFO[2021-06-07T10:06:23Z] {"component":"entrypoint","file":"prow/entrypoint/run.go:165","func":"k8s.io/test-infra/prow/entrypoint.Options.ExecuteProcess","level":"error","msg":"Process did not finish before 2h0m0s timeout","severity":"error","time":"2021-06-07T10:06:03Z"} 
INFO[2021-06-07T10:06:23Z] {"component":"entrypoint","file":"prow/entrypoint/run.go:255","func":"k8s.io/test-infra/prow/entrypoint.gracefullyTerminate","level":"error","msg":"Process did not exit before 15s grace period","severity":"error","time":"2021-06-07T10:06:18Z"} 
INFO[2021-06-07T10:06:23Z] {"component":"entrypoint","error":"process timed out","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-07T10:06:18Z"} 
INFO[2021-06-07T10:06:23Z] error: failed to execute wrapped command: exit status 127 

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jun 7, 2021

/test psi-kubernetes-integration-e2e
/test psi-unit-test-windows

 ERRO[2021-06-07T08:06:11Z] [ssh:Fedora-32-minikube] !!!run failed, skipping!!! 
ERRO[2021-06-07T08:06:11Z] 2021/06/07 08:06:06 : failed due to err Failed the test, see logs above ^ 
ERRO[2021-06-07T08:06:11Z] {"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-07T08:06:06Z"} 
ERRO[2021-06-07T08:06:11Z] error: failed to execute wrapped command: exit status 1 

@valaparthvi
Copy link
Contributor Author

/test psi-kubernetes-integration-e2e

@valaparthvi
Copy link
Contributor Author

/test psi-unit-test-windows

@valaparthvi
Copy link
Contributor Author

/retest

psi-kubernetes-integration-e2e - the make job apparently causing the below error has been disabled. Retesting should make the failure go away.

++ make goget-ginkgo 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] verifying github.com/mikefarah/yaml/[email protected]/go.mod: checksum mismatch 
INFO[2021-06-07T14:18:09Z]                                              
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] 	downloaded: h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] 	go.sum:     h1:ahVqZF4n1W4NqwvVnZzC4es67xsW9uR/RRf2RRxieJU= 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube]                     
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] SECURITY ERROR      
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] This download does NOT match an earlier download recorded in go.sum. 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] The bits may have been replaced on the origin server, or an attacker may 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] have intercepted the download attempt. 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube]                     
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] For more information, see 'go help module-auth'. 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] verifying github.com/mikefarah/yaml/[email protected]/go.mod: checksum mismatch 
INFO[2021-06-07T14:18:09Z]                                              
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] 	downloaded: h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= 
INFO[2021-06-07T14:18:09Z] [ssh:Fedora-32-minikube] 	go.sum:     h1:ahVqZF4n1W4NqwvVnZzC4es67xsW9uR/RRf2RRxieJU= 

psi-unit-test-windows

 INFO[2021-06-07T16:17:59Z] ++ tar -xzf ci-firewall-linux-amd64.tar.gz   
INFO[2021-06-07T16:17:59Z] ++ ./ci-firewall request --mainbranch main --sendqueue amqp.ci.queue.win.unit.send --sendtopic amqp.ci.topic.win.unit.send --sendexchange amqp.ci.exchange.win.unit.send --setupscript scripts/setup_script_unit.sh --jenkinsproject odo-windows-unit-pr-build --runscript scripts/run_script_unit.sh --timeout 4h00m 
INFO[2021-06-07T16:17:59Z] {"component":"entrypoint","file":"prow/entrypoint/run.go:165","func":"k8s.io/test-infra/prow/entrypoint.Options.ExecuteProcess","level":"error","msg":"Process did not finish before 2h0m0s timeout","severity":"error","time":"2021-06-07T16:17:40Z"} 
INFO[2021-06-07T16:17:59Z] {"component":"entrypoint","file":"prow/entrypoint/run.go:255","func":"k8s.io/test-infra/prow/entrypoint.gracefullyTerminate","level":"error","msg":"Process did not exit before 15s grace period","severity":"error","time":"2021-06-07T16:17:55Z"} 
INFO[2021-06-07T16:17:59Z] {"component":"entrypoint","error":"process timed out","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-07T16:17:55Z"} 

func sanitizeUserInfo(errString string) string {
user1, err1 := user.Current()
if err1 != nil {
return errors.Wrapf(err1, err1.Error()).Error()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest if we cannot get the user, we just return the error as is?
this function is about santising the userInfo and if we cannot get the user in my opinion we shouldn't add that to the error because lets say there is an error getting the current user you would send an error like "X component not found: cannot resolve user".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right of course. It doesn't really make sense to wrap the error like that either, it isn't really doing anything!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome

@girishramnani
Copy link
Contributor

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 8, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 8, 2021
@valaparthvi
Copy link
Contributor Author

/override psi-kubernetes-integration-e2e
Reason - #4776

@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2021

@valaparthvi: /override requires a failed status context to operate on.
The following unknown contexts were given:

  • psi-kubernetes-integration-e2e

Only the following contexts were expected:

  • ci/prow/psi-kubernetes-integration-e2e
  • ci/prow/psi-unit-test-mac
  • ci/prow/psi-unit-test-windows
  • ci/prow/unit
  • ci/prow/v4.7-integration-e2e
  • tide

In response to this:

/override psi-kubernetes-integration-e2e
Reason - #4776

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@valaparthvi
Copy link
Contributor Author

/override ci/prow/psi-kubernetes-integration-e2e
Reason - #4776

@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2021

@valaparthvi: Overrode contexts on behalf of valaparthvi: ci/prow/psi-kubernetes-integration-e2e

In response to this:

/override ci/prow/psi-kubernetes-integration-e2e
Reason - #4776

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit ca5ebd6 into redhat-developer:main Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants