Terraform: add cert authority override support - #69623
Conversation
GitHub Actions
Inactive
|
Amplify deployment status
Preview links for changed docs pages
|
53cb2c8 to
ee280ed
Compare
GitHub Actions
Inactive
ee280ed to
09b47c9
Compare
GitHub Actions
Inactive
09b47c9 to
7a5c58a
Compare
GitHub Actions
Inactive
7a5c58a to
d3a2f3e
Compare
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3a2f3ef95
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Upsert updates a cert authority override. | ||
| func (r CertAuthorityOverrideClient) Upsert(ctx context.Context, override *subcav1.CertAuthorityOverride) error { | ||
| if _, err := r.client.SubCAClient().UpsertCertAuthorityOverride(ctx, &subcav1.UpsertCertAuthorityOverrideRequest{ | ||
| CaOverride: override, |
There was a problem hiding this comment.
When Terraform updates an existing CA override after Auth has populated status.public_key_hash_to_crl (the async CRL path used for disabled overrides in multi-Auth/HSM clusters), the plan decoded from Terraform has status omitted because it is excluded from the schema, and this Upsert sends that object as the full replacement. That can drop server-managed CRLs during a harmless metadata/spec reconciliation and corrupt the override's revocation state; preserve the prior status in PrepareUpdate or use a merge/granular update path.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this is moot, but the bot can't read e/ so it can't tell.
If no status is present in the request we'll simply use the last stored status. We do let callers supply the CRLs, and only the CRLs, so if you tctl get an old override you can re-create it verbatim.
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
LGTM.
I'm not sure about the Terraform boilerplate, as I haven't done this before, but the CertAuthorityOverride bits seem good to me.
Thanks for tackling this, Tim!
| func (r CertAuthorityOverrideClient) Get(ctx context.Context, id tfdriver.NameIdentifier) (*subcav1.CertAuthorityOverride, error) { | ||
| resp, err := r.client.SubCAClient().GetCertAuthorityOverride(ctx, &subcav1.GetCertAuthorityOverrideRequest{ | ||
| CaId: &subcav1.CertAuthorityOverrideID{ | ||
| CaType: id.Name, |
There was a problem hiding this comment.
Is it worth saying, somewhere, that cert_authority_overrides are actually identified by sub_kind + name (ie, "ca type" + "cluster name"), but that for the RPCs here the current cluster is implied as the cluster name?
There was a problem hiding this comment.
Could we add a sub_kind="windows" to the tests too, so we exercise multiple sub kinds?
Same for the manual test plan, would you mind testing with both db_client and windows overrides? You don't need to actually connect or anything, it's only to make sure that both overrides work together on Tf.
| csr, err := x509.ParseCertificateRequest(block.Bytes) | ||
| s.Require().NoError(err) | ||
|
|
||
| rootKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) |
There was a problem hiding this comment.
Do the helpers at lib/subca/testenv help here?
teleport/lib/subca/testenv/ca.go
Line 65 in adfaab0
teleport/lib/subca/testenv/ca.go
Line 70 in adfaab0
teleport/lib/subca/testenv/ca.go
Line 193 in adfaab0
|
|
||
| name := "teleport_cert_authority_override.test" | ||
|
|
||
| resource.Test(t, resource.TestCase{ |
There was a problem hiding this comment.
Could we maybe write 2 overrides here? One db_client and one windows?
2 participants
Closes #69591
Closes https://github.com/gravitational/core/issues/108
Changelog: Added
cert_authority_overridesupport to the Terraform provider,Manual Test Plan
Test Environment
Local cluster and TF Provider built off this branch.
Test Cases
terraform applycreates an enableddb_clientoverride; resource visible viatctl get cert_authority_override/db_clientandtctl auth exportserves the externally-signed chainterraform planshows no changesmetadata.descriptioncertificate_overrides[0].disabledtrue/false on an active overridesub_kindforces replacementmetadata.nameforces replacementsub_kindterraform import teleport_cert_authority_override.x db_clientsucceeds and no subsequent drifttctl auth update-overrideis detected and reconciled when the field is explicitly set in configsub_kindfails ("exists in Teleport")metadata.name!= cluster name rejected by server (incorrect cluster name)sub_kindrejected by server (invalid or unsupported sub_kind/caType)terraform destroyof a live enabled override succeeds and the resource is removed server-side