support create secret validation via {none, exists} options, to allow absent credentials (and fix #108)
#115
Conversation
benfleis
changed the title
validation to none, exists in CREATE SECRET AWS providervalidation via {none, exists} options, to allow absent credentials (and fix #108)
benfleis
force-pushed
the
add-aws-secret-validation
branch
from
1bb6867 to
c92510b
Compare
`exists` setting matches current behavior -- actual credential must be found/exist for `CREATE SECRET` to succeed. `none` matches previous (v1.3.2) behavior, where no credential is required at `CREATE SECRET` time. fixes duckdb#108
also fix/update include httpfs path
benfleis
force-pushed
the
add-aws-secret-validation
branch
from
c92510b to
0dcde66
Compare
… absent credentials (and fix duckdb#108) - `exists` setting matches current behavior -- credential must be found/exist for `CREATE SECRET` to succeed. - `none` matches previous (v1.3.2) behavior, where no credential is required at `CREATE SECRET` time.
Tmonster
left a comment
Tmonster
left a comment
There was a problem hiding this comment.
Looks good! just one question about what happens in the absence of the validation argument
copy httpfs minio scripts directly for integration testing
plus remove debugging code
There was a problem hiding this comment.
Looks good to me, but I have a few questions/comments about testing
I don't think the test/sql/aws_secret_validation tests are running in CI right now. MinioTests.yml will only run ./build/release/test/unittest "*/test/sql/env/*", which won't match test/sql/aws_secret_validation.test
Edit: hit enter too soon sorry.
Also, could we get some tests that test Env vars vs. credentials file?
I testing with assume role, and that looks fine.
test/sql/aws_secret_validation.test
Outdated
|
|
||
| #### Use non existent profile to force no available credentials | ||
|
|
||
| # bad validation param |
There was a problem hiding this comment.
can we also add a validation 'exists' with a profile? Then if we can check the contents of the secret with something like
from duckdb_secrets() select unnest(split(secret_string, ';'));
That would be super cool
There was a problem hiding this comment.
+1 - that the explicit profile exists check be added.
IMHO checking secrets contents seems unnecessarily e2e here, with little payoff. That data is passthrough, relative to this test, and the correctness of secret retention should already be tested elsewhere, right?
samansmink
left a comment
samansmink
left a comment
There was a problem hiding this comment.
Thanks for the changes @benfleis! I did another pass which led to some minor nitpicks
test/sql/aws_secret_validation.test
Outdated
| VALIDATION 'exists' | ||
| ); | ||
| ---- | ||
| <REGEX>:.*Invalid Input Error: Failed to create secret.* |
There was a problem hiding this comment.
Shouldn't this throw an InvalidConfigurationException exception too? InvalidInputException suggests that the failure depends on a wrong input whereas this actually depends on the configuration being wrong. I think ideally we'd throw an error that references the fact that it is the VALIDATION option that caused this
test/sql/aws_secret_validation.test
Outdated
| VALIDATION 'nonsense' | ||
| ); | ||
| ---- | ||
| <REGEX>:.*Invalid Configuration Error: Unknown AWS VALIDATION option.* |
There was a problem hiding this comment.
I think this is actually an InvalidInputException? This is wrong input no matter the configuration?
test/sql/aws_secret_validation.test
Outdated
| VALIDATION 'exists' | ||
| ); | ||
| ---- | ||
| <REGEX>:.*Invalid Configuration Error: Failed to load profile.* |
There was a problem hiding this comment.
again here: I think referencing the relationship to the VALIDATION option here would be nice?
There was a problem hiding this comment.
Updated the thrown errors to include "Secret Validation Failure", which (correctly) touches some other existing tests/errors as well, and included some extra 'none' validation tests alongside those.
samansmink
left a comment
samansmink
left a comment
There was a problem hiding this comment.
Looks great @benfleis!
3 participants
support create secret validation via {none, exists} options, to allow absent credentials (and fix #108)
fixes: #108
existssetting matches current behavior -- credential must be found/exist forCREATE SECRETto succeed.nonematches previous (v1.3.2) behavior, where no credential is required atCREATE SECRETtime.