feat: Allow passing unknown flags to plugins

[erezrokah]

Summary

Should help with https://github.com/cloudquery/cloudquery-issues/issues/2455#issuecomment-2363343665 (internal issue).

At the moment, each time we add a new flag to a plugin and pass it from the CLI it will break all existing plugins since they fail to parse known flags.

This should allow the CLI send new flags to new plugins without breaking existing ones

---

Use the following steps to ensure your PR is ready to be reviewed

• Read the contribution guidelines 🧑‍🎓
• Run go fmt to format your code 🖊
• Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
• Update or add tests 🧪
• Ensure the status checks below are successful ✅

[erezrokah]

The way I see it, we can add new properties to the spec without breaking existing plugins, so this should be the same

[erezrokah]

Not sure why Post Unit Tests is failing seems unrelated

[disq]

Can we make it log a warning with the unhandled/unknown flag?

[erezrokah]

Can we make it log a warning with the unhandled/unknown flag?

I can see if Cobra exposes the unknown flags

[hermanschaaf]

Yeah this seems a bit controversial, as the plugin may silently fail to act as you instructed it to act, which might be worse than failing hard

[disq]

I think we should also add an --abort-on-unknown-flag type thing.

[marianogappa]

Actually @hermanschaaf has a really good point here cc @erezrokah

I encountered this many times across the years. You misspell a flag and don't notice and suddenly your tool misbehaves and you find out the week after 🤔

I think this needs some thinking. Because forwards/backwards compatibility is important too.

[erezrokah]

Yeah this seems a bit controversial, as the plugin may silently fail to act as you instructed it to act, which might be worse than failing hard

Yeah I think this is mostly an internal problem for us, less for our users.

[disq]

Maybe we can turn this around, by adding an --ignore-unknown-flags flag? Then each time the cli specifies a "known-new" flag we include --ignore-unknown-flags temporarily too, for a few versions until a set time has passed. (Implementation would need have two parse operations if the ignore flag wasn't specified on the first parse, to make sure there really was no unknown flags, to be able to fail if there were)

[erezrokah]

I can see if Cobra exposes the unknown flags

We need spf13/pflag#199, spf13/cobra#739 (comment)

[erezrokah]

Maybe we can turn this around, by adding an --ignore-unknown-flags flag? Then each time the cli specifies a "known-new" flag we include --ignore-unknown-flags temporarily too, for a few versions until a set time has passed. (Implementation would need have two parse operations if the ignore flag wasn't specified on the first parse, to make sure there really was no unknown flags, to be able to fail if there were)

I like that idea but it needs to be an environment variable (it needs to be evaluated before parsing happens), so maybe when the CLI starts plugins it can pass IGNORE_UNKNOWN_FLAGS=true

[hermanschaaf]

Yeah I think this is mostly an internal problem for us, less for our users.

@erezrokah why? Maybe I'm missing something. I know users don't typically run plugins directly, but I still think it's everyone's problem.

Let's say we added a new feature to the CLI to allowlist only connections to certain domains (to use a random example). If this is to be enforced by the plugin, it would need to be passed as a flag. With the change here, if users enabled this feature in the CLI, they would expect it to work, but actually it would not, if they are using an older version of a plugin.

[erezrokah]

@erezrokah why? Maybe I'm missing something. I know users don't typically run plugins directly, but I still think it's everyone's problem.

Let's say we added a new feature to the CLI to allowlist only connections to certain domains (to use a random example). If this is to be enforced by the plugin, it would need to be passed as a flag. With the change here, if users enabled this feature in the CLI, they would expect it to work, but actually it would not, if they are using an older version of a plugin.

Ah, good point. @marianogappa and I discussed this problem via DMs as well. At the moment the CLI does not know which features plugins support. For example not all plugins support deterministic_cq_id (e.g. DB source plugins). Plugins that don't use the scheduler don't support otel_endpoint.

So maybe what we really need is a way for plugins to tell the CLI the features they support

[marianogappa]

With the change here, if users enabled this feature in the CLI, they would expect it to work, but actually it would not, if they are using an older version of a plugin.

Even with a warning I think it's problematic. Older version of a plugin is one case, but misspelling is another, am I the only one who misspells a lot?

[disq]

environment variable (it needs to be evaluated before parsing happens)

My idea was to parse everything twice, once with cobra.Unknown enabled, to detect the unknown flag, and then a second time... but I like this env var idea.

[disq]

way for plugins to tell the CLI the features they support

Move most things to spec, then we can have a message through GRPC? Simplify serve to have only --address (or just a few more). Do you think initializing otel after the spec is parsed is acceptable?

Also this needs to be accomodated in other language SDKs as well, so moving more things into spec is beneficial from SDK maintenance PoV.

[erezrokah]

Move most things to spec, then we can have a message through GRPC? Simplify serve to have only --address (or just a few more). Do you think initializing otel after the spec is parsed is acceptable?

Well if you init OTEL after spec is parsed, you're bound to miss some logs. But regardless plugins don't have access to top level spec, so we'd need to pass everything to the init gRPC call I think

[disq]

Move most things to spec, then we can have a message through GRPC? Simplify serve to have only --address (or just a few more). Do you think initializing otel after the spec is parsed is acceptable?

Well if you init OTEL after spec is parsed, you're bound to miss some logs. But regardless plugins don't have access to top level spec, so we'd need to pass everything to the init gRPC call I think

Yes the options would be in the outer spec which gets passed into Init (or capability detection call first, then selectively to Init)

[erezrokah]

OK going to close this since this warrants an offline discussion