Throw errors from countTokens in ini tokenizer

[daemonl]

Fixes, or at least improves, #2276
The parser likely should be able to handle the hex string as-is, but the format isn't a standard part of the AWS config so likely not a real issue.

[lucix-aws]

We can't change the behavior in this way - I don't want to suddenly break consumers that have "invalid" values in their configs (according to the current implementation, at least). We should be correcting the actual failure to parse instead.

[daemonl]

@lucix-aws would you accept a PR which changes the behavior to throw global errors in all error cases (parsing issue, profile not found) based on an environment variable?

[lucix-aws]

@daemonl Sorry for the delayed response. As an FYI in case you hadn't seen, we did fix the underlying parsing issue in #2286.

I would definitely be open to supporting a "strict" parsing mode but it would have to be through an in-code setting, probably on LoadOptions:

type LoadOptions struct {
    // ...

    // Whether "strict" parsing of shared config is enabled. By default, config loading or parsing
    // failures are silently ignored. Enabling this setting will instead propagate these errors to config
    // load calls.
    SharedConfigParseStrict bool
}

We tend to prefer not to add arbitrary environment variables to drive SDK settings as it creates confusion in a cross-SDK context.

[lucix-aws]

If you're interested in having that kind of behavior independent of the fix, I'd ask that you create a feature request such that we can discuss further.

[daemonl]

The thing is that when using a tool like terraform, I won't be able to set that config. I guess I have to hope they will set it.

The default behaviour to silently ignore some but not all errors seems very surprising, especially with no way for me as a consumer of various tools to even log the error.

Implementing that flag would be even more strange as the library does throw some errors, so this would really only apply to errors when counting the number of tokens, that or the function would be changed to throw all errors, and it's caller would choose based on that config if the error should be dropped - which is also a 'breaking change'.

I really believe this is a simple bug fix, not a breaking change. I would be interested to compare the way an actually corrupt file behaves in the various SDKs and the CLI, I suspect it will be inconsistent but I would be very surprised if they silently ignore the file all together.

[daemonl]

How about something a bit different - but haven't groked enough of the code to know if it'll be easy: When the configs all fall back and finally do error out, that final error could include that it failed to parse the config, or couldn't find the profile in the config etc, so it's only changing the error message on a final failure situation.
That and/or re-instating the verbose logging env var and starting to implement it in places like this.
Really, I'm just looking for some way to know what is wrong with my setup rather than getting an error about EC2 metadata when I'm trying to deploy terraform through an SSO profile using the 'new stuff'