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

Jump to bottom

Add additional log statements and update client-go #92

Merged
merged 2 commits into from
Nov 20, 2020
Merged

Add additional log statements and update client-go #92

merged 2 commits into from
Nov 20, 2020

Conversation

shravan-achar
Copy link
Contributor

Issue #, if available:

Description of changes:

  • Add logging statements where containers/pod already have required properties
  • Update client-go to kubernetes-1.18. Update to go1.13

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@shravan-achar shravan-achar requested a review from a team as a code owner November 19, 2020 03:24
@shravan-achar shravan-achar changed the title Add addition log statements and update client-go Add additional log statements and update client-go Nov 19, 2020
jaypipes
jaypipes suggested changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Great stuff @shravan-achar :) Just one request inline to remove the unrelated Gauge -> Histogram metric change.

Thanks!
-jay

go.mod Show resolved Hide resolved
pkg/cert/request.go Show resolved Hide resolved
jqmichael
jqmichael reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@jqmichael jqmichael left a comment

Choose a reason for hiding this comment

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

/lgtm.

Minor comments.

Can we test this change against a 1.15 cluster to ensure backwards-compatibility?

pkg/handler/handler.go Outdated Show resolved Hide resolved
pkg/handler/handler.go Outdated Show resolved Hide resolved
pkg/cache/cache.go Outdated Show resolved Hide resolved
pkg/handler/handler.go Outdated Show resolved Hide resolved
@mcristina422 mcristina422 mentioned this pull request Nov 19, 2020
Closed
@shravan-achar
Copy link
Contributor Author

Can we test this change against a 1.15 cluster to ensure backwards-compatibility?

@jqmichael - Can confirm the container image works against k8s 1.15 server

jaypipes
jaypipes previously approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

pkg/cert/request.go Show resolved Hide resolved
nckturner
nckturner reviewed Nov 19, 2020
View reviewed changes
pkg/handler/handler.go Outdated Show resolved Hide resolved
nckturner
nckturner reviewed Nov 19, 2020
View reviewed changes
pkg/cache/cache.go
@@ -60,6 +60,7 @@ func (c *serviceAccountCache) Get(name, namespace string) (role, aud string, use
klog.V(5).Infof("Fetching sa %s/%s from cache", namespace, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want a very high (i.e. 9) level log that prints out the service account cache on every read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been following this logging convention doc https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md. Anything >=5 is trace level logging. If I enable logLevel 9, I can see HTTP level logs (Request headers). So, I kept it at 5.

nckturner
nckturner reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@nckturner nckturner left a comment

Choose a reason for hiding this comment

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

Nice! Had some minor suggestions.

@shravan-achar
Add logging statements where containers/pod already have required
24c6809 
properties

Use read lock instead of RW lock when listing cache contents

Update README.md
@shravan-achar
Copy link
Contributor Author

@jaypipes @nckturner - Pushed a small update to bump up logging levels for some statements. Re-requesting review :)

@jqmichael jqmichael merged commit 2c44edc into aws:master Nov 20, 2020
@mcristina422 mcristina422 mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcristina422 mcristina422 mcristina422 left review comments

@nckturner nckturner nckturner approved these changes

@jqmichael jqmichael jqmichael approved these changes

@jaypipes jaypipes Awaiting requested review from jaypipes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shravan-achar @mcristina422 @jaypipes @nckturner @jqmichael