Add support for ServiceAccount auth to kubeletstats #324

pmcollins · 2020-06-15T15:01:06Z

Description: Kubeletstats receiver only supported TLS auth. This change adds support
for ServiceAccount auth as well.

Link to tracking Issue: #311

Testing: Manual testing of ServiceAccount auth was done in GKE.

Documentation: ServiceAccount config is already in the README.

linux-foundation-easycla · 2020-06-15T15:01:10Z

The committers are authorized under a signed CLA.

✅ Pablo Collins (249f4489efd20ad8400e525e35a9ee9319746db9)

codecov · 2020-06-15T15:06:34Z

Codecov Report

Merging #324 into master will increase coverage by 0.33%.
The diff coverage is 87.27%.

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   83.04%   83.38%   +0.33%     
==========================================
  Files         167      167              
  Lines        8900     8943      +43     
==========================================
+ Hits         7391     7457      +66     
+ Misses       1185     1161      -24     
- Partials      324      325       +1

Impacted Files	Coverage Δ
receiver/kubeletstatsreceiver/kubelet/cert.go	`69.23% <50.00%> (+69.23%)`	⬆️
receiver/kubeletstatsreceiver/kubelet/client.go	`92.85% <88.67%> (+41.63%)`	⬆️
receiver/carbonreceiver/transport/tcp_server.go	`65.71% <0.00%> (-1.91%)`	⬇️
receiver/kubeletstatsreceiver/factory.go	`64.10% <0.00%> (+5.12%)`	⬆️

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 a304ef1...d41a754. Read the comment docs.

Kubeletstats receiver only supported TLS auth. This change adds support for ServiceAccount auth as well. Addresses issue #311.

dmitryax

Overall looks good to me. Just minor comments

dmitryax · 2020-06-16T05:43:59Z

receiver/kubeletstatsreceiver/kubelet/client.go

+func newServiceAccountClient(endpoint string, caCertPath string, tokenPath string, logger *zap.Logger) (*clientImpl, error) {
+	rootCAs, err := systemCertPoolPlusPath(caCertPath)
+	if err != nil {
+		return nil, err


Should we provide more details by wrapping this error?

Good point. Done.

dmitryax · 2020-06-16T05:44:47Z

receiver/kubeletstatsreceiver/kubelet/client.go

+	}
+	tok, err := ioutil.ReadFile(tokenPath)
+	if err != nil {
+		return nil, err


Same here. It might be not clear to the end user why do we try to read this file

Agree. Fixed.

dmitryax · 2020-06-16T06:07:05Z

receiver/kubeletstatsreceiver/kubelet/client.go

 		InsecureSkipVerify: insecureSkipVerify,
 	}
-	return &tlsClient{
+	if endpoint == "" {
+		endpoint = defaultEndpoint(logger)


I would put a log entry here like (maybe warning?): kubelet endpoint is not provided, using hostname "..." by default.

I think usually it's not going to work so it would be good to let user know about it, in case if they just forgot to set the endpoint config.

This functionality is borrowed from the Smart Agent and I agree it's not obvious. If you set hostNetwork to true in the pod spec, the pod has access to the node's loopback device. I have added a comment to explain, have added a log warning, and updated the readme.

Cool. Thank you!

dmitryax · 2020-06-16T06:11:22Z

receiver/kubeletstatsreceiver/kubelet/client.go

+	hostname, err := os.Hostname()
+	if err != nil {
+		logger.Error("unable to get hostname", zap.Error(err))
+		endpoint = "localhost"


Not sure if there is a case when this would work in k8s, but let's keep it if you think it can work

Should work with hostNetwork, but I've removed falling back to localhost since getting the hostname shouldn't error out (and smart agent doesn't do this either).

Yes, I was talking about localhost in this comment

Improved logging and explained hostNetwork usage.

dmitryax · 2020-06-16T16:05:31Z

receiver/kubeletstatsreceiver/README.md

+is running to be used as the endpoint. If the hostNetwork flag is set, and the
+collector is running in a pod, this hostname will resolve to the node's network
+namespace.
+


Can we add a serviceAccount usage example here with downward API?

If I understand correctly it should be:

kubeletstats: authType: serviceAccount endpoint: https://${K8S_NODE_NAME}:10250

Another note:

Make sure that it's set using the downward API in the collector pod spec as follows:

env: - name: K8S_NODE_NAME valueFrom: fieldRef: fieldPath: spec.nodeName

Yeah, that's a good idea. Done.

Update README with service account example

dmitryax

Looks good to me. I'm not sure if we need to hit 95% coverage here. Probably only NewClient, where case k8sconfig.AuthTypeServiceAccount: looks like still not covered

pmcollins · 2020-06-16T18:17:48Z

Looks good to me. I'm not sure if we need to hit 95% coverage here. Probably only NewClient, where case k8sconfig.AuthTypeServiceAccount: looks like still not covered

Thanks for the review Dmitrii. I haven't added coverage for those two lines because they require secrets to be mounted. I'll have to make some tweaks to make this unit-testable.

dmitryax · 2020-06-16T20:45:48Z

@pmcollins could you check the linter issue please?

dmitryax

LGTM. @tigrannajaryan @bogdandrutu the diff has pretty good coverage. I don't think we gain a lot by reaching 95% here. Could you take a look if we can merge it?

Kubeletstats receiver only supported TLS auth. This change adds support for ServiceAccount auth as well. **Link to tracking Issue:** #311 **Testing:** Manual testing of ServiceAccount auth was done in GKE. **Documentation:** ServiceAccount config is already in the README.

* Added a configuration options for csv lazy quotes (embedded) quotes Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com> * Updated csv_parser docs with lazy_quotes option Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

Fixes #323

pmcollins requested a review from a team June 15, 2020 15:01

tigrannajaryan assigned pmcollins and dmitryax Jun 15, 2020

pmcollins added 7 commits June 15, 2020 16:19

Add support for ServiceAccount auth to kubeletstats

dfb6b30

Kubeletstats receiver only supported TLS auth. This change adds support for ServiceAccount auth as well. Addresses issue #311.

Improve test coverage

ac0b2b7

Improve test coverage

79e4e08

Improve test coverage

91dd92c

Trigger CI with minor change

0fce584

Improve test coverage

39fac0d

Improve test coverage

2be9f6a

dmitryax reviewed Jun 16, 2020

View reviewed changes

Respond to PR feedback

f079f93

Improved logging and explained hostNetwork usage.

dmitryax reviewed Jun 16, 2020

View reviewed changes

Respond to PR feedback

812e8da

Update README with service account example

dmitryax approved these changes Jun 16, 2020

View reviewed changes

Improve test coverage

1a8c93c

Improve test coverage

d41a754

flands added this to the 0.5.0 milestone Jun 17, 2020

dmitryax approved these changes Jun 17, 2020

View reviewed changes

tigrannajaryan merged commit 4ca134c into open-telemetry:master Jun 17, 2020

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022

Unslice to simplify (#324)

eb3b31e

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Add posargs (#324)

0f88a05

Fixes #323

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Update span interface usage (#324)

0fe9096

crobert-1 mentioned this pull request Sep 1, 2023

[receiver/kubeletstats] tls verify config not being respected #26319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ServiceAccount auth to kubeletstats #324

Add support for ServiceAccount auth to kubeletstats #324

pmcollins commented Jun 15, 2020

linux-foundation-easycla bot commented Jun 15, 2020 •

edited

Loading

codecov bot commented Jun 15, 2020 •

edited

Loading

dmitryax left a comment

dmitryax Jun 16, 2020

pmcollins Jun 16, 2020

dmitryax Jun 16, 2020

pmcollins Jun 16, 2020

dmitryax Jun 16, 2020

pmcollins Jun 16, 2020

dmitryax Jun 16, 2020

dmitryax Jun 16, 2020

pmcollins Jun 16, 2020

dmitryax Jun 16, 2020

dmitryax Jun 16, 2020 •

edited

Loading

pmcollins Jun 16, 2020

dmitryax left a comment

pmcollins commented Jun 16, 2020

dmitryax commented Jun 16, 2020

dmitryax left a comment

Add support for ServiceAccount auth to kubeletstats #324

Add support for ServiceAccount auth to kubeletstats #324

Conversation

pmcollins commented Jun 15, 2020

linux-foundation-easycla bot commented Jun 15, 2020 • edited Loading

codecov bot commented Jun 15, 2020 • edited Loading

Codecov Report

dmitryax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitryax Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitryax left a comment

Choose a reason for hiding this comment

pmcollins commented Jun 16, 2020

dmitryax commented Jun 16, 2020

dmitryax left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jun 15, 2020 •

edited

Loading

codecov bot commented Jun 15, 2020 •

edited

Loading

dmitryax Jun 16, 2020 •

edited

Loading