feat: Add key_algorithm configuration to application template#113
feat: Add key_algorithm configuration to application template#113
Conversation
Partially fixes #33. The key algoirthm and relevant parameters were hard coded. This change allows you to specify options you would like in a succinct manner. It also defaults the values if unspecified in order to minimise configuration. key_reuse option also now defaults to false for more minimal configuration. Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| prts := strings.Split(v.ValueString(), "_") | ||
|
|
||
| // First check no unknown key types or malformed input. | ||
| if prts[0] != "RSA" && prts[0] != "EC" || len(prts) != 2 { |
There was a problem hiding this comment.
Probably for later; but I think we could add a validator to the schema attribute.
There was a problem hiding this comment.
(which is better, because then we validate at plan time, rather than at apply time)
| continue | ||
| } | ||
| if !slices.Contains(allowedKeyLengths, int32(length)) { | ||
| fmt.Printf("Unsupported key length: %d\n", length) |
There was a problem hiding this comment.
Generally, I've tried to avoid doing too much validation in terraform space.
If the platform gains support for a new key length, I don't think we should have to do another release of the provider just to add it.
That's why I've been phrasing things in the docs as "valid options include" rather than "this is the list of valid options"
There was a problem hiding this comment.
Looking at the validator option here: https://developer.hashicorp.com/terraform/plugin/framework/validation#attribute-validation
I think that may well reduce the need to validate in code and cut it down. I'd be happy to do that as it would mean less maintenance.
However I don't feel like key options will change quickly or massively. I felt it was better to be explicit with the available options now, rather than cater to something that's unlikely to change. I'm sure there would be a big noise around adding say quantum key support?
There was a problem hiding this comment.
I kinda reckon that the provider should validate the format, there's no point sending stuff we know is garbage, but the api should validate the value. I think there's a difference between being clear about what the available options are now, vs. only allowing those options without a further code change/provider release.
I had been more thinking about the user roles here, where I might expect that other roles might be added for more granular access - but even here, although I'd not expect the types (RSA, EC) to change, I might expect different key lengths or curves to be added.
There was a problem hiding this comment.
Maybe take another look now, I've changed to use a validator, here've and example catch:
│ Error: Invalid Attribute Value Match
│
│ with tlspc_certificate_template.provider_test,
│ on main.tf line 98, in resource "tlspc_certificate_template" "provider_test":
│ 98: key_algorithms = ["RSA_3072", "RSA_4096", "EC_ED25519", "EC_4096"]
│
│ Attribute key_algorithms[3] value must be one of: ["RSA_1024" "RSA_2048" "RSA_3072" "RSA_4096" "EC_P256" "EC_P384" "EC_P521" "EC_ED25519"], got:
│ "EC_4096"
╵
Given it's a very small list that seemed easier to comprehend than a complicated regex.. which effectively does the same: Narrow input to the 8 available options.
I tried regex, but quickly found if I tried to be clever, I left a hole in the middle. eg.
# tlspc_certificate_template.provider_test will be updated in-place
~ resource "tlspc_certificate_template" "provider_test" {
id = "8b7e0170-239b-11f1-aa82-bb1f9e36b693"
~ key_algorithms = [
# (2 unchanged elements hidden)
"EC_ED25519",
+ "EC_4096",
]
name = "provider-test-template"
# (3 unchanged attributes hidden)
}
There was a problem hiding this comment.
What error do we get back from the API if we do send an invalid key alg?
Signed-off-by: Peter Fiddes <peter.fiddes@jetstack.io>
bdfdedc to
0487ec1
Compare
| Default: listdefault.StaticValue(defaultKeyAlgorithms), | ||
| Validators: []validator.List{ | ||
| listvalidator.ValueStringsAre( | ||
| stringvalidator.OneOf(allowedAlgorithms...), |
There was a problem hiding this comment.
Unless you need to use allowedAlgorithms in multiple places, I would just hardcode the list in here.
Because, as a global array variable, the list is otherwise mutable.
There was a problem hiding this comment.
I wasn't sure if it was worth having them on the data source as well.
I don't think it's needed for that so I will move it over.
Partially fixes #33.
The key algorithm and relevant parameters were hard coded. This change allows you to specify options you would like in a succinct manner. It also defaults the values if unspecified in order to minimise configuration. key_reuse option also now defaults to false for more minimal configuration.
Example testing resource:
Result:
State: