Secret Source Implementation

[mstoykov]

What?

Adds an extensible secret sources. Basically #4139 but without http secret source as that turned out to have too many configuration problems. But it has a mock secret source which basically gets them form the cli arguments - probably should be renamed.

Usage:

import secrets from "k6/secrets";

export default async () => {
	const my_secret = await secrets.get("cool"); // get secret from a source with the provided identifier
	console.log(my_secret);
	await secrets.get("else"); // get secret from a source with the provided identifier
	console.log(my_secret);
}

go run . run --secret-source=mock=cool=some,else=source script.js

or with a secret file:

cool=some
else=source

go run . run --secret-source=file=file.secret script.js

The extension points are under ./secretsource and mostly around the Get method which gets a secret and returns it.

The rest of k6 makes certain to not request it again and redact it from logs.

TODO:

• Should we throw an exception or return undefined on errors, or at least on not found secrets. I currently think having an exception lets sources have specific errors potentially helping with debugging. But maybe logging the error is good enough 🤷
• Multiple secret sources is almost done but not quite
• Unit Tests and likely some integration ones will be nice - although I do plan to add some more.
• Have HTTP secret source? Maybe for later version this seems to have way too many moving pieces to be a good idea for first versions
• Configuration currently only happens from CLI flags which might not be ideal and is definitely problematic as , is special, so you need ot use some other character for separation like : in the mock configuration.
• Having Name be parsed by the internal parts before we actually make the secret source
• Having a way to define a Default secret source when named ones are also possible.
• Some Benchmarks will be nice
• Docs of any kind
• Moving to k6/secrets seems like a better idea - both easier to track if it used, but also will stop adding stuff to "k6" a module that mostly has stuff that ... are not great.
• usage statistics on used secret source
• Use Description and log which secret sources are used a kin to outputs

Why?

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

[mstoykov]

This is contrast with how it is done for outputs or js modules.

I kind of prefer this to having a function with all of them in one place which gets constant conflicts.

But I can redo it to be more aligned with other parts of k6.

[oleiade]

I'm not against it. I actually find it somewhat elegant. The only complaint I have is that I find import side-effects very implicit, and thus somewhat harder to maintain; as you need to remember that the import is gonna do some stuff for you. But this is more of a preference thing, and not a blocker for me.

[mstoykov]

I would prefer to give this a go in a separate PR, maybe in the next version.

[mstoykov]

I can pull this in a separate PR if needed.

[mstoykov]

Reminder for me

[inancgumus]

I'll continue to review this PR, but for now, I find these are a bit of a mouthful and repetitive:

• secretsource.SecretsManager
• secretsource.SecretSource

What do you think about:

• secret.LogRedacter (more descriptive)
• secret.Source (easy to type, read, and remember)

Also, does the secretsource.SecretSource interface need all these methods (especially Name() and Description())? What do you think about:

package secret

type Provider struct {
	Name        string
	Description string
	// more extensible without breaking the interface
}

type Source interface {
	Provider() Provider
	Get(key string) (value string, err error)
}

Sidenote: I'm also not sure if we need Name() in the original interface because we use it to look up if the secret source provider is registered. We could also check through its concrete typename (through the interface) to see if it's registered.

[mstoykov]

I am not against renaming some of the stuff, but:

1. I think LogRedactor isn't more descriptive. The idea is more that it manages the secrets and is later used to get them back. It also facilitating redaction of the logs is not it's main function.
2. I kind of do not like really small names such as secret. So I prefer to keep that as secretsource.

So secretsource.Manager and secretsource.Source seems good enough.

On the struct part ... I am not against it, but not certain it is really better.

Also of note is that :

1. Name is actually the name of the source to be used in case of multiple secret sources. This likely will be easier to get from the manager or something else preparsing the configurations. This seems to not be what people think it is as I have another datapoint where people think it is the name of the secret source as type.
2. Description isn't currently used, and I guess I should add adding the secrets to the logs around outputs and such to the TODO.

[inancgumus]

I think LogRedactor isn't more descriptive. The idea is more that it manages the secrets and is later used to get them back. It also facilitating redaction of the logs is not it's main function.

Manager sounds more like Registry then? Manager is also fine, but it's generic. It also sounds like it has multiple responsibilities: registering secret sources and attaching a logger. Splitting these might make responsibilities clearer.

I kind of do not like really small names such as secret. So I prefer to keep that as secretsource.

I see that we already have a new package called k6/secrets (a module). Since we already have k6/secrets, I believe using secret here might be clearer, shorter, and more idiomatic. But secretsource is also fine since you prefer to keep it that way.

So secretsource.Manager and secretsource.Source seems good enough.

I see that you changed SecretSource to Source; that seems better 👍 If you're open, I still think Provider might communicate intent clearer. Source is fine, but there's still slight stuttering (secretsource.Source). Not critical, just mentioning.

On the struct part ... I am not against it, but not certain it is really better.
Description isn't currently used, and I guess I should add adding the secrets to the logs around outputs and such to the TODO.

I totally get your point about descriptions for logs/CLI. But interfaces usually work best when minimal—only essential behavior. One small idea could be to store descriptions separately at registration (rather than directly in the interface). But if that feels overly complex, sticking to your approach is okay, too. Maybe like:

type ... struct {
    Source      Source
    Description string
}

Anyway, these are all just suggestions. I'm fine with whatever you decide 🙂

[mstoykov]

I would like @oleiade opinion on this as well, but also please review the remaining part of the PR.

From my perspective:

1. The manager is not a registry - the registry is in different place. It is structure that manages secret sources and returns secrets from them. We use the same name for (metric) outputs
2. k6/secrets is named that as that is what the user uses it for - it is to get secrets. They do not explicitly work with secret sources. Maybe we will add other functionality around secrets there one day.
3. secretsource is named that as this is the only thing the package does - it defines secretsources and their interface and how to work with them. The people working with the two packages are for different use cases, even if they are connected. Having a package secrets that than has Source interface inside sounds great, but I would argue that the package name will not be telling what is inside it - there are not secrets there, just an interface for Sources.

[inancgumus]

Thanks for your patience 🙇 I'm a late reviewer because of the Hackathons.

I believe we're looking at this from different perspectives because I see packages as providers of functionality rather than containers of functionality. So, I consider the secret (or secrets) package to be a provider of secret.Source functionality or its protocol.

I find "manager" very generic and can (and probably will) have many responsibilities beyond its purposes. That's what usually happens to manager types. It's like a "utility" name. Even now, it has two responsibilities: a registry of secret sources and a hook adder. This hooking behavior may not be its responsibility.

Anyway, like I said, I don't want to block this PR just because of semantic discussions. I will approve it since I don't see any issues with the code 🤝 ❤️

[oleiade]

Great work 👏🏻 I really like the shape this takes, and I had a good time experimenting with it.

I'm happy with the current state of things, and although I have just a couple of comments, I approve the PR, as I don't consider any of those blocking whatsoever.

• I really like the secret source module approach 👍🏻
• I've found that the introduction of the Usage wiring was a bit unnecessary. You actually left comment around that. No strong opinion, but it could make sense to me if it was in a separate PR indeed.
• The first test run I've made with PR, I forgot to pass the secret source argument, and I noticed that k6 produced an error message which I found could be more explicit:
  
  In this case, as a user I would have expected a message along the lines of "The test script tries to access secrets, but no source where defined, make sure to use the command-line argument --etc..."
• on the checklist item above "Should we throw an exception or return undefined on errors, or at least on not found secrets. I currently think having an exception lets sources have specific errors potentially helping with debugging. But maybe logging the error is good enough 🤷": I tend to think exceptions would make sense here 🙇🏻

[oleiade]

I'm not against it. I actually find it somewhat elegant. The only complaint I have is that I find import side-effects very implicit, and thus somewhat harder to maintain; as you need to remember that the import is gonna do some stuff for you. But this is more of a preference thing, and not a blocker for me.

[mstoykov]

@oleiade I've changed the original message and added an additional one in case there is nothing configured.

I do not really want to be too specific on stuff given k6 direction in a more generic solution. So I do not want to return errors that have stuff such as script in there, given that secrets can be used by not just from js and so on.

Arguably we can have better errors if we add even more types for those and handle them in the runner or something like this.

[oleiade]

@mstoykov Cheers! I find the new error message is much more explicit, and I agree with your point on keeping it general/generic 🙇🏻

[inancgumus]

There are some issues I pointed out in this comment. Other than that it's LGTM. Good work! 👍