feat: Add IsUsingSecrets to synthetic_monitoring.Check #1473
Conversation
This method provides a single way to determine whether a check is using secrets or not. For some types, this will be straighforward (either because they do not support secrets, or because using secrets in that case is very explicit). In other cases, this involves some guesswork, and the extra scripts in this commit are mostly about validating that guesswork. Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
There was a problem hiding this comment.
@d0ugal sorry to make this PR harder to understand.
What's important here is the name of the file. That should provide a hint as to the intention. For example this one is NOT using secrets, and it has the import bit commented out.
| usesSecretsModuleRe = regexp.MustCompile( | ||
| `(?m:` + | ||
| // Default import: import secrets from 'k6/secrets' | ||
| `^\s*import\s+secrets\s+from\s+` + secretsCanary + | ||
| `|` + | ||
| // Named import: import { } from 'k6/secrets' | ||
| // Named import: import { secrets } from 'k6/secrets' | ||
| // Named import: import { secrets, foo } from 'k6/secrets' | ||
| // Named import: import { foo, secrets } from 'k6/secrets' | ||
| // Named import: import { foo, secrets, bar } from 'k6/secrets' | ||
| // Named import: import { secrets as s } from 'k6/secrets' | ||
| `^\s*import\s*{[^}]*}\s+from\s+` + secretsCanary + | ||
| `|` + | ||
| // Namespace import: import * as secrets from 'k6/secrets | ||
| `^\s*import\s+\*\s+as\s+\S+\s+from\s+` + secretsCanary + | ||
| `|` + | ||
| // Require syntax: const secrets = require('k6/secrets') | ||
| `^\s*const\s+secrets\s*=\s*require\s*\(\s*` + secretsCanary + `\s*\)` + | ||
| `)`) | ||
| ) |
There was a problem hiding this comment.
This is the bit that is actually interesting.
I have tried to keep the regular expression simple.
It's matching lines (the ?m: bit at the start, so it's possible to write ^\s*import, meaning it's OK to have blanks before import, but import must be the first thing on the line.
| `^\s*import\s+\*\s+as\s+\S+\s+from\s+` + secretsCanary + | ||
| `|` + | ||
| // Require syntax: const secrets = require('k6/secrets') | ||
| `^\s*const\s+secrets\s*=\s*require\s*\(\s*` + secretsCanary + `\s*\)` + |
There was a problem hiding this comment.
I don't think I have a test for this, do I? How is this actually used?
| return false | ||
|
|
||
| case c.Settings.Ping != nil: | ||
| return false |
There was a problem hiding this comment.
this a good example of the intention: ping will never use secrets, so it will always return false, unconditionally.
OTOH, HTTP will support secrets, so this needs to be updated in the future.
| } | ||
| } | ||
|
|
||
| func TestCheckIsUsingSecrets(t *testing.T) { |
There was a problem hiding this comment.
This test is not trying to be exhaustive (that's why the above test exists). This test is just trying to exercise the switch.
That's fair. Are you thinking that at some point the API will have a better / more precise way of determining if a script is using secrets? What I'm thinking here is that for non-scripted checks, if we have a way to ask whether a check is using secrets, we have have a piece of generic code that decides whether to take different actions based on that. The obvious one is refreshing a token. If we can improve the way we detect that, we might end up adding a field to specific check settings stating that, and this method ends up using that to return an answer. |
Essentially yes. I think it already will handle most or all of the cases you have here. Your implementation is neater though and I should adopt that layout.
That makes sense. On my first pass I didn't realise this was in checks_extra, so we could move all the checking here and then reuse it from the API. I'm happy with either, I just want to avoid inconsistent detection. |
2 participants
This method provides a single way to determine whether a check is using secrets or not. For some types, this will be straighforward (either because they do not support secrets, or because using secrets in that case is very explicit). In other cases, this involves some guesswork, and the extra scripts in this commit are mostly about validating that guesswork.