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

[receiver/kubeletstats] fix client to refresh service account tokens #26316

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

jinja2
Copy link
Contributor

@jinja2 jinja2 commented Aug 30, 2023

Description:
Supports refreshing service account tokens in the client used to communicate with kubelet

Link to tracking Issue:
Issue: 26120

Testing:
Updated unit tests
Local e2e testing in k8s cluster

Screenshot of local testing with 2 collector pods started at same time running the kubeletstats receiver and service account token expiration set to 10mins. The one of left is running old build and the one on right is the new build.

Screenshot 2023-08-30 at 11 08 46 AM

@jinja2 jinja2 requested a review from dmitryax as a code owner August 30, 2023 18:19
@jinja2 jinja2 requested a review from a team August 30, 2023 18:19
@@ -165,9 +166,27 @@ func (p *saClientProvider) BuildClient() (Client, error) {
}
tr := defaultTransport()
tr.TLSClientConfig = &tls.Config{
RootCAs: rootCAs,
RootCAs: rootCAs,
InsecureSkipVerify: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have not been passing the user provided InsecureSkipVerify config when authType is service account. I am keeping the same behavior here and will create a different issue to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#26319 Any ideas/context if this was done intentionally?

require.NoError(t, err)
require.Equal(t, "s3cr3t", string(cl.(*clientImpl).tok))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewBearerAuthWithRefreshRoundTripper handles the authorization header for us, so removing this test case and replacing with the server validation check

OSjOBwyTvlCIOF5AnafFuCk81EnbMLQeIvsfudXTAi0aw6HMmS2UgmGUcwGsFDRt
Xx1n1nLclK73f2a9mrzrh1s3lMom1FLaZ8fRecB9cVZJAHdw7RDtkG0qFf+v9Hk0
y8JKVfSmRIveHz62a/61T36VSKIO0qksdR4wEPCjADsdpagVMrwjyu8qShLFXRkn
MIIFuDCCA6CgAwIBAgIUX6gHrM+cXSsdSQTFUTl7ISB8xoYwDQYJKoZIhvcNAQEL
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated cert since the ones checked in have expired

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@jinja2 jinja2 force-pushed the fix-kubelet-client branch from e404683 to 35679c6 Compare August 30, 2023 21:00
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Just on nit. Otherwise LGTM

It'd be nice to remove defaultTLSClient as the next step (separate PR) if it's being used only by the tlsClientProvider now

internal/kubelet/client.go Outdated Show resolved Hide resolved
@jinja2 jinja2 force-pushed the fix-kubelet-client branch from 1b4fb5d to c72e3b3 Compare September 5, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants