feat(ske): support audit log configuration - #1578
feat(ske): support audit log configuration#1578tobias-pfaffelmoser-ske wants to merge 8 commits into
Conversation
| } | ||
|
|
||
| func mapAudit(cl *ske.Cluster, m *Model) error { | ||
| // A missing audit block only occurs in regions where the feature is |
There was a problem hiding this comment.
I don't fully get this, can't the API just include false in the response in that case?
{
...
"audit": {
"enabled": false
}
}Else you might have users which will run into state drifts: When Terraform users set audit.enabled to false in their Terraform configuration, Terraform will also send false to the API. From my current understanding the API then just won't return the audit object at all in the response. This will lead to Terraform setting audit.enabled to null here which will then result in a state drift error.
There was a problem hiding this comment.
Good point.
Just checked with the team: when the feature is available in the respective region, the API always echoes the audit object back, including { "enabled": false }. So setting audit.enabled = false returns enabled: false and maps cleanly back into state - so no drift expected in this scenario.
When the feature is not available in the respective region, the API omits the audit object entirely. That is the situation where cl.Audit == nil, and in that case, audit shouldn't be configured on the cluster in the first place. Mapping it to null might make sense: if a user tries to configure audit in a region without the feature, the resulting diff could be the desired signal that the configuration has no effect at all. WDYT?
There was a problem hiding this comment.
I'm just curious, why would the feature be not available in a region at all? Don't we roll out the same service version to all regions?
There was a problem hiding this comment.
the resulting diff could be the desired signal that the configuration has no effect at all. WDYT?
And no, that's nothing we should aim for. State drifts are pain for our users and we shouldn't expose such problems to our users.... What brings me back to the question above 😉
There was a problem hiding this comment.
I'm just curious, why would the feature be not available in a region at all? Don't we roll out the same service version to all regions?
e.g. in eu02 ske is rolled out but telemetry router/link is still WIP. This will be also the case for next regions
There was a problem hiding this comment.
As checked together with @MichaelEischer today:
- SKE API change for audit log feature rolled out to all public regions -> API returns non-null audit object in any scenario
- If audit feature generally not available/activated (at e.g. eu02), API also returns audit object with
{ "enabled": false }. However, sending{ "enabled": true }to API results in returned error in respective region.
52f7b99 to
2160dd9
Compare
rubenhoenle
left a comment
rubenhoenle
left a comment
There was a problem hiding this comment.
As written in the PR template checklist, the Acceptance tests / E2E tests need to be adjusted to include your new field: ske_acc_test.go
Min test
In the min test only required fields must be set in the test configuration.
Since your field is optional you only have to add checks that the new field has the correct default value.
terraform-provider-stackit/stackit/internal/services/ske/ske_acc_test.go
Lines 120 to 265 in e45f2f5
Max test
For the Max test you have to add your field to the Terraform config:
Afterwards add a new variable for your field:
Also update the field to test not only the create operation but also the update operation:
terraform-provider-stackit/stackit/internal/services/ske/ske_acc_test.go
Lines 110 to 118 in e45f2f5
Then add the checks for your new field to the text steps:
terraform-provider-stackit/stackit/internal/services/ske/ske_acc_test.go
Lines 267 to 509 in e45f2f5
2160dd9 to
54be3c6
Compare
| resource.TestCheckResourceAttr("stackit_ske_cluster.cluster", "maintenance.start", testutil.ConvertConfigVariable(testConfigVarsMax["maintenance_start"])), | ||
| resource.TestCheckResourceAttr("stackit_ske_cluster.cluster", "maintenance.end", testutil.ConvertConfigVariable(testConfigVarsMax["maintenance_end"])), | ||
| resource.TestCheckResourceAttrSet("stackit_ske_cluster.cluster", "region"), | ||
| resource.TestCheckResourceAttr("data.stackit_ske_cluster.cluster", "network.control_plane.access_scope", testutil.ConvertConfigVariable(testConfigVarsMin["network_control_plane_access_scope"])), |
There was a problem hiding this comment.
Please also check the output of the datasource here (min test)
| resource.TestCheckResourceAttr("stackit_ske_cluster.cluster", "maintenance.end", testutil.ConvertConfigVariable(configVarsMinUpdated()["maintenance_end"])), | ||
| resource.TestCheckResourceAttr("stackit_ske_cluster.cluster", "region", testutil.ConvertConfigVariable(configVarsMinUpdated()["region"])), | ||
| resource.TestCheckResourceAttrSet("stackit_ske_cluster.cluster", "kubernetes_version_used"), | ||
| resource.TestCheckResourceAttr("stackit_ske_cluster.cluster", "network.control_plane.access_scope", testutil.ConvertConfigVariable(configVarsMinUpdated()["network_control_plane_access_scope"])), |
There was a problem hiding this comment.
Please also check it here after the update (min test)
rubenhoenle
left a comment
rubenhoenle
left a comment
There was a problem hiding this comment.
Looks good besides that ✔️
8862ef0 to
28cee91
Compare
5 participants
Description
Adds support for configuring SKE cluster audit log forwarding via a new optional
auditblock on thestackit_ske_clusterresource (and as a read-only attributeon the data source):
The audit.enabled flag is passed through to the SKE API (ske.Audit) on create/update and mapped back into state on read. This feature is in private preview and can only be enabled for accounts/projects that have been enabled for audit log forwarding to a Telemetry Router.
This also bumps the stackit-sdk-go/services/ske dependency to v1.19.0, which introduces the audit field.
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)