Skip to content

Remove logging of certs#6866

Merged
rshriram merged 1 commit intoistio:release-1.0from
ijsnellf:do-not-log-certs
Jul 6, 2018
Merged

Remove logging of certs#6866
rshriram merged 1 commit intoistio:release-1.0from
ijsnellf:do-not-log-certs

Conversation

@ijsnellf
Copy link
Copy Markdown
Contributor

@ijsnellf ijsnellf commented Jul 5, 2018 •
edited
Loading

We were generating logs like this on Pilot:

2018-07-05T19:00:22.840433Z    info    Certificate received [[%!V(*x509.Certificate=&{[48 130 3 15 48 130 1 247 160 3 2 1 2 2 17 0 175 231 74 227 12 112 111 22 224 128 76 98 68 125 74 230 48 13 6 9 42 134 72 134 247 13 1 1 11 5 0 48 19 49 17 48 15 6 3 85 4 10 19 8 74 117 106 117 32 ...

@ijsnellf ijsnellf requested review from costinm and rshriram July 5, 2018 19:19
@costinm costinm changed the base branch from master to release-1.0 July 5, 2018 19:56
@costinm
Copy link
Copy Markdown
Contributor

costinm commented Jul 5, 2018

Please rebase and resolve conflicts - needs to be in release-1.0 ( changed the base - but there are conflicts)

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Jul 5, 2018

/lgtm
/approved
(once rebase is done)

@rshriram rshriram changed the base branch from release-1.0 to master July 5, 2018 20:09
@ijsnellf ijsnellf force-pushed the do-not-log-certs branch from 5f05e21 to 5956eba Compare July 5, 2018 20:28
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 5, 2018

New changes are detected. LGTM label has been removed.

@istio-testing istio-testing removed the lgtm label Jul 5, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, ijsnellf

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

The pull request process is described here

Details 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

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 5, 2018 •
edited
Loading

Codecov Report

Merging #6866 into release-1.0 will increase coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           release-1.0   #6866   +/-   ##
===========================================
+ Coverage           71%     71%   +1%     
===========================================
  Files              370     370           
  Lines            32031   31952   -79     
===========================================
- Hits             22534   22531    -3     
+ Misses            8561    8494   -67     
+ Partials           936     927    -9
Impacted Files Coverage Δ
mixer/adapter/servicecontrol/handler.go 39% <0%> (-8%) ⬇️
mixer/adapter/rbac/util.go 64% <0%> (-5%) ⬇️
mixer/adapter/stackdriver/stackdriver.go 50% <0%> (-4%) ⬇️
mixer/adapter/signalfx/cumulative.go 67% <0%> (-4%) ⬇️
mixer/adapter/kubernetesenv/cache.go 92% <0%> (-1%) ⬇️
mixer/adapter/statsd/statsd.go 96% <0%> (-1%) ⬇️
mixer/adapter/bypass/util.go 7% <0%> (ø) ⬇️
mixer/adapter/circonus/circonus.go 72% <0%> (ø) ⬇️
mixer/adapter/memquota/rollingWindow.go 100% <0%> (ø) ⬆️
mixer/adapter/stdio/stdio.go 100% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32a2a81...5956eba. Read the comment docs.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 5, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 5, 2018

@ijsnellf: PR needs rebase.

Details

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.

@googlebot
Copy link
Copy Markdown
Collaborator

googlebot commented Jul 5, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Jul 5, 2018
@ijsnellf ijsnellf changed the base branch from master to release-1.0 July 5, 2018 20:28
@ijsnellf ijsnellf added cla: human-approved and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged labels Jul 5, 2018
@ijsnellf
Copy link
Copy Markdown
Contributor Author

ijsnellf commented Jul 5, 2018

/test istio-pilot-e2e-envoyv2-v1alpha3

@rshriram rshriram merged commit 0616a43 into istio:release-1.0 Jul 6, 2018
@ijsnellf ijsnellf deleted the do-not-log-certs branch July 6, 2018 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@costinm costinm Awaiting requested review from costinm

1 more reviewer

@rshriram rshriram rshriram approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@costinm costinm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ijsnellf @costinm @istio-testing @googlebot @rshriram