Skip to content

Dispose certificates in Kubernetes.Dispose() #1191

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

Merged
k8s-ci-robot merged 2 commits into from
Feb 1, 2023
Merged

Dispose certificates in Kubernetes.Dispose() #1191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cd01f1
Dispose certs created by Kuberentes
shenglol Feb 1, 2023
e869a20
Update tests
shenglol Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/KubernetesClient/Kubernetes.ConfigInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,27 +90,27 @@ private void InitializeFromConfig(KubernetesClientConfiguration config)
// set credentails for the kubernetes client
SetCredentials(config);

var clientCert = CertUtils.GetClientCert(config);
if (clientCert != null)
ClientCert = CertUtils.GetClientCert(config);
if (ClientCert != null)
{
#if NET5_0_OR_GREATER
HttpClientHandler.SslOptions.ClientCertificates.Add(clientCert);
HttpClientHandler.SslOptions.ClientCertificates.Add(ClientCert);

// TODO this is workaround for net7.0, remove it when the issue is fixed
// seems the client certificate is cached and cannot be updated
HttpClientHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) =>
{
return clientCert;
return ClientCert;
};
#else
HttpClientHandler.ClientCertificates.Add(clientCert);
HttpClientHandler.ClientCertificates.Add(ClientCert);
#endif
}
}

private X509Certificate2Collection CaCerts { get; }

private X509Certificate2 ClientCert { get; }
private X509Certificate2 ClientCert { get; set; }

private bool SkipTlsVerify { get; }

Expand Down
5 changes: 0 additions & 5 deletions src/KubernetesClient/Kubernetes.WebSocket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,6 @@ protected async Task<WebSocket> StreamConnectAsync(Uri uri, string webSocketSubP
}

// Set Credentials
if (this.ClientCert != null)
Copy link
Contributor

@brendandburns brendandburns Feb 1, 2023

Choose a reason for hiding this comment

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

Why are you deleting this?

Copy link
Contributor Author

@shenglol shenglol Feb 1, 2023

Choose a reason for hiding this comment

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

This is dead code. this.ClientCert is never assigned and is always null before the change. I did a git blame and it seems it's a leftover from a previous refactoring. The logic is covered by the code blow.

{
webSocketBuilder.AddClientCertificate(this.ClientCert);
}

if (this.HttpClientHandler != null)
{
#if NET5_0_OR_GREATER
Expand Down
17 changes: 16 additions & 1 deletion src/KubernetesClient/Kubernetes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,27 @@ public void Dispose()
/// <param name="disposing">True to release both managed and unmanaged resources; false to releases only unmanaged resources.</param>
protected virtual void Dispose(bool disposing)
{
if (!_disposed)
if (disposing && !_disposed)
{
_disposed = true;

// Dispose the client
HttpClient?.Dispose();

// Dispose the certificates
if (CaCerts is not null)
{
foreach (var caCert in CaCerts)
{
caCert.Dispose();
}

CaCerts.Clear();
}


ClientCert?.Dispose();

HttpClient = null;
FirstMessageHandler = null;
HttpClientHandler = null;
Expand Down
16 changes: 8 additions & 8 deletions tests/E2E.Tests/MinikubeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public void SimpleTest()
var namespaceParameter = "default";
var podName = "k8scsharp-e2e-pod";

var client = CreateClient();
using var client = CreateClient();

void Cleanup()
{
Expand Down Expand Up @@ -79,7 +79,7 @@ public void PatchTest()
var namespaceParameter = "default";
var podName = "k8scsharp-e2e-patch-pod";

var client = CreateClient();
using var client = CreateClient();

void Cleanup()
{
Expand Down Expand Up @@ -183,7 +183,7 @@ void Cleanup()
[MinikubeFact]
public async Task WatcherIntegrationTest()
{
var kubernetes = CreateClient();
using var kubernetes = CreateClient();

var job = await kubernetes.BatchV1.CreateNamespacedJobAsync(
new V1Job()
Expand Down Expand Up @@ -251,7 +251,7 @@ public async Task WatcherIntegrationTest()
[MinikubeFact]
public void LeaderIntegrationTest()
{
var client = CreateClient();
using var client = CreateClient();
var namespaceParameter = "default";

void Cleanup()
Expand Down Expand Up @@ -350,7 +350,7 @@ public async Task LogStreamTestAsync()
var namespaceParameter = "default";
var podName = "k8scsharp-e2e-logstream-pod";

var client = CreateClient();
using var client = CreateClient();

void Cleanup()
{
Expand Down Expand Up @@ -446,7 +446,7 @@ async Task<V1Pod> Pod()
[MinikubeFact]
public async Task DatetimeFieldTest()
{
var kubernetes = CreateClient();
using var kubernetes = CreateClient();

await kubernetes.CoreV1.CreateNamespacedEventAsync(
new Corev1Event(
Expand Down Expand Up @@ -478,7 +478,7 @@ public async void GenericTest()
var namespaceParameter = "default";
var podName = "k8scsharp-e2e-generic-pod";

var client = CreateClient();
using var client = CreateClient();
var genericPods = new GenericClient(client, "", "v1", "pods");

void Cleanup()
Expand Down Expand Up @@ -590,7 +590,7 @@ public async Task CopyToPodTestAsync()
var namespaceParameter = "default";
var podName = "k8scsharp-e2e-cp-pod";

var client = CreateClient();
using var client = CreateClient();

async Task<int> CopyFileToPodAsync(string name, string @namespace, string container, Stream inputFileStream, string destinationFilePath, CancellationToken cancellationToken = default(CancellationToken))
{
Expand Down
4 changes: 3 additions & 1 deletion tests/Kubectl.Tests/KubectlTests.Version.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using k8s.E2E;
using k8s.kubectl.beta;
using System.Text.Json;
using Xunit;

Expand All @@ -9,7 +10,8 @@ public partial class KubectlTests
[MinikubeFact]
public void Version()
{
var client = CreateClient();
using var kubernetes = MinikubeTests.CreateClient();
var client = new Kubectl(kubernetes);
var version = client.Version();
var serverobj = version.ServerVersion;

Expand Down
7 changes: 0 additions & 7 deletions tests/Kubectl.Tests/KubectlTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
using k8s.E2E;
using k8s.kubectl.beta;
using System.Diagnostics;

namespace k8s.kubectl.Tests;

public partial class KubectlTests
{
private Kubectl CreateClient()
{
return new Kubectl(MinikubeTests.CreateClient());
}

private string RunKubectl(string args)
{
var p = new Process
Expand Down
12 changes: 6 additions & 6 deletions tests/KubernetesClient.Tests/CertUtilsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public void LoadFromFiles()
useRelativePaths: false);

// Just validate that this doesn't throw and private key is non-null
var cert = CertUtils.GeneratePfx(cfg);
using var cert = CertUtils.GeneratePfx(cfg);
Assert.NotNull(cert.GetRSAPrivateKey());
}

Expand All @@ -44,7 +44,7 @@ public void LoadFromFilesRelativePath()
"federal-context");

// Just validate that this doesn't throw and private key is non-null
var cert = CertUtils.GeneratePfx(cfg);
using var cert = CertUtils.GeneratePfx(cfg);
Assert.NotNull(cert.GetRSAPrivateKey());
}

Expand All @@ -58,7 +58,7 @@ public void LoadFromInlineData()
useRelativePaths: false);

// Just validate that this doesn't throw and private key is non-null
var cert = CertUtils.GeneratePfx(cfg);
using var cert = CertUtils.GeneratePfx(cfg);
Assert.NotNull(cert.GetRSAPrivateKey());
}

Expand All @@ -73,7 +73,7 @@ public void LoadFromInlineDataRelativePath()
"victorian-context");

// Just validate that this doesn't throw and private key is non-null
var cert = CertUtils.GeneratePfx(cfg);
using var cert = CertUtils.GeneratePfx(cfg);
Assert.NotNull(cert.GetRSAPrivateKey());
}

Expand All @@ -85,8 +85,8 @@ public void LoadPemWithMultiCert()
{
var certCollection = CertUtils.LoadPemFileCert("assets/ca-bundle.crt");

var intermediateCert = new X509Certificate2("assets/ca-bundle-intermediate.crt");
var rootCert = new X509Certificate2("assets/ca-bundle-root.crt");
using var intermediateCert = new X509Certificate2("assets/ca-bundle-intermediate.crt");
using var rootCert = new X509Certificate2("assets/ca-bundle-root.crt");

Assert.Equal(2, certCollection.Count);

Expand Down