Skip to content

Add the ability to inject authenticators. #2311

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
merged 2 commits into from
Mar 24, 2025
Merged

Add the ability to inject authenticators. #2311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c3c488
Add the ability to inject authenticators.
brendandburns Mar 10, 2025
a26e167
Switch to config local authenticators instead of
brendandburns Mar 21, 2025
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
6 changes: 6 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ export class KubeConfig implements SecurityAuthentication {
new OpenIDConnectAuth(),
];

// Optionally add additional external authenticators, you must do this
// before you load a kubeconfig file that references them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it is worth adding a test that intentionally does not do this (if for no other reason than knowing if it accidentally changes)?

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 don't think this can ever change, because the kubeconfig file references the authenticator as part of the YAML file, so if we don't know about the authenticator because it hasn't been registered when we load the kubeconfig, there's no way that we can link the authenticator requested in the file to the kubeconfig.

So while we could add a test, I'm not sure it would ever break.

public static addAuthenticator(authenticator: Authenticator): void {
this.authenticators.push(authenticator);
Copy link
Contributor

@davidgamero davidgamero Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i may be misunderstanding typescript classes here, but is the intended use here to add them to all existing KubeConfig objects since the authenticators property is static?

also, could we avoid the loading order issue by adding an optional parameter for an array of authenticators to the constructor instead? that way the authenticators must be added prior to loading a config

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'm ok with this, but I also think it makes it a little more complicated. I wanted the authenticators to be able to self-register just by being import-ed which requires the static registration. @cjihrig wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to note is that the authenticators are already static, so I think making this method static aligns well with the current setup.

I don't personally see a ton of upside to self-registering via import - my understanding is that it would mean calling KubeConfig.addAuthenticator() inside the import instead of in the code managing the KubeConfig instance.

Given the choice between static and per-instance custom authenticators, I think per-instance is likely to have less surprising behavior. For example, in the test added in this PR, wouldn't CustomAuthenticator be included in every subsequent test?

}

/**
* The list of all known clusters
*/
Expand Down
41 changes: 41 additions & 0 deletions src/config_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { mock } from 'node:test';

import mockfs from 'mock-fs';

import { Authenticator } from './auth.js';
import { Headers } from 'node-fetch';
import { HttpMethod } from './index.js';
import { assertRequestAgentsEqual, assertRequestOptionsEqual } from './test/match-buffer.js';
Expand Down Expand Up @@ -1703,5 +1704,45 @@ describe('KubeConfig', () => {
}
validateFileLoad(kc);
});

it('should inject a custom Authenticator', async () => {
class CustomAuthenticator implements Authenticator {
public isAuthProvider(user: User): boolean {
return user.authProvider === 'custom';
}

public async applyAuthentication(user: User, opts: RequestOptions): Promise<void> {
if (user.authProvider === 'custom') {
// Simulate token retrieval
const token = 'test-token';
opts.headers = opts.headers || {};
opts.headers.Authorization = `Bearer ${token}`;
} else {
throw new Error('No custom configuration found');
}
}
}

const customAuthenticator = new CustomAuthenticator();
KubeConfig.addAuthenticator(customAuthenticator);
const kc = new KubeConfig();

const cluster: Cluster = {
name: 'test-cluster',
server: 'https://localhost:6443',
skipTLSVerify: false,
};
const user: User = {
name: 'test-user',
authProvider: 'custom',
};

kc.loadFromClusterAndUser(cluster, user);

const opts: RequestOptions = {};
await kc.applyToHTTPSOptions(opts);

strictEqual(opts.headers!.Authorization, 'Bearer test-token');
});
});
});