identity: reload CA root cert channel on file change

[jlojosnegros]

Problem

ztunnel reads the CA root certificate once at startup and discards it. When an operator rotates the Istio CA (istio-ca-root-cert ConfigMap), the file on disk is updated by Kubernetes but ztunnel keeps using the stale cert to verify TLS with istiod. Every subsequent workload certificate renewal fails with UnknownIssuer.

Proposed Solution

Add a RootCertManager, modelled after the existing CrlManager. It holds a filesystem watcher (via notify-debouncer-full) that sets an atomic dirty flag whenever the root cert directory changes.

CaClient now retains the RootCert and a Option<Arc<RootCertManager>>. At the start of each fetch_certificate call, if the dirty flag is set, the TLS gRPC channel is rebuilt from the new cert before proceeding. On rebuild failure the dirty flag is re-armed so the next call retries.

Changes

File	Change
src/tls/control.rs	Add RootCertManager; expose control_plane_client_config as pub(crate)
src/identity/caclient.rs	Retain connection params in CaClient; add rebuild_channel(); hot-reload check in fetch_certificate()
src/identity/manager.rs	Adapt call site to new CaClient::new() signature
src/test_helpers/ca.rs	Adapt CaServer::spawn() to new signature

No Cargo.toml changes needed: notify and notify-debouncer-full are already pulled in by CrlManager.

Solves: #1768

[istio-testing]

Hi @jlojosnegros. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[ilrudie]

/ok-to-test

[nmnellis]

Will you be able to backport this fix to 1.28?

[jlojosnegros]

Will you be able to backport this fix to 1.28?

Yeah sur, no problem. Should be something like #1801

[ilrudie]

Will you be able to backport this fix to 1.28?

Yeah sur, no problem. Should be something like #1801

We'd typically only cherry-pick bug fixes. This is a grey area IMO. It could be seen as a bug of omission. At any rate, If we do want to consider this a bug we should use the cherry-pick methodology native to the project by labeling this issue for cherry-pick.

[ilrudie]

Mostly this looks good. Nice work and thanks for the contribution.

I did want to talk about the locking a little bit to make sure I understand it the same way you do an that it's the right optimization but no need to immediately jump in and start changing that bit.

[ilrudie]

This isn't requesting a change per se. Sort of reasoning "out loud" about the locking here.

After we take_dirty() the state returns to clean immediately and we enter the await to attempt rebuild_channel. During this time nothing holds the write lock and the state is clean so we may try to use an old client. It might be able to compound as contentious readers may continue to delay being able to take the write, all the while in a "clean" state. I think the advantage is that we do not queue multiple write/rebuilds, which seems like a nice property. In the happier state, the old client is still valid and we keep on running nicely but eventually wind up with a fresh client. In the worse state, the client isn't usable and we produce errors while potentially blocking the resolution by causing read contention on the lock.

Assuming we held the rwlock before marking clean, a bunch of reads could await the write lock to resolve. On success, they get the newly minted client (probably ideal?). On fail, they get the old client and what happens happens. The downside is we could have multiple writelock awaits simultaneously queue since multiple calls to fetch could see the dirty state while we await any held read locks to clear. This might be kind of helpful, since at least write locks to not lead to further read contentions, but we might need to check if the dirty was resolved by someone else before actually rebuilding.

[jlojosnegros]

Hi @ilrudie ,
Thanks for the comments!!!
Sorry for the late response

After we take_dirty() the state returns to clean immediately and we enter the await to attempt rebuild_channel. During this time nothing holds the write lock and the state is clean so we may try to use an old client. It might be able to compound as contentious readers may continue to delay being able to take the write, all the while in a "clean" state

Yeah, you are right, but the contention will be quite limited I think.
If I remember correctly, in Linux the RW lock uses "write-preferring" policy by default, so once a writer ask for the lock it only has to wait for the readers that already has the lock to unlock it, but has preference over any new reader. Given this the contention will be limited to just those readers that asked for the lock before the writer and Read lock is only hold while doing the clone which is a very fast operation (~us), so the contention time should be really small

I think the advantage is that we do not queue multiple write/rebuilds, which seems like a nice property. In the happier state, the old client is still valid and we keep on running nicely but eventually wind up with a fresh client. In the worse state, the client isn't usable and we produce errors while potentially blocking the resolution by causing read contention on the lock.

You are right again. Old client will be used while rebuild is done and that will fail if the old cert is not valid just after rotation, but I was assuming a healthy time window between rotation and certification expiry.

Assuming we held the rwlock before marking clean, a bunch of reads could await the write lock to resolve. On success, they get the newly minted client (probably ideal?). On fail, they get the old client and what happens happens. The downside is we could have multiple writelock awaits simultaneously queue since multiple calls to fetch could see the dirty state while we await any held read locks to clear. This might be kind of helpful, since at least write locks to not lead to further read contentions, but we might need to check if the dirty was resolved by someone else before actually rebuilding.

Yeah, the problem with that is we cannot hold a RWlock through an await operation, so we should change std::async::RWLock to tokio::async::RWLock which is not trivial, and as you have already said, we will have to use a double-checked locking.

Working from this idea I was thinking ...
What if we use a "rebuild_lock" (tokio::sync::Mutex) to serialize channel builds, so we avoid queuing multiple builds, it will be something like:

if take_dirty() is true
    get rebuild_lock                                    // only one rebuild in queue
    if take_dirty() is true                             // double-check 
        new_client = rebuild_channel().await
        *self.client.write().unwrap() = new_client      // quick write lock
    endif
    unlock rebuild_lock
endif

This will:

• "serialize" rebuilds, so we only have one at a time
• avoid readers to be block while rebuilding channel, as write lock is only hold for the client swap
• no need to change std::sync::RWLock

[ilrudie]

Thanks for the discussion. I agree we should have time to rotate out the client in most reasonable cases so this is probably a suitable implementation with a good balance of complexity.

It might be nice to add how long we waited for the write lock to the debug message. This way if we suspect contention, we already have some information that can be made available to troubleshoot. I don't think we need to immediately introduce more complexity though.

[jlojosnegros]

Good idea.
I have added the elapsed time in write contention to the debug log.

[jlojosnegros]

Will you be able to backport this fix to 1.28?

Yeah sur, no problem. Should be something like #1801

We'd typically only cherry-pick bug fixes. This is a grey area IMO. It could be seen as a bug of omission. At any rate, If we do want to consider this a bug we should use the cherry-pick methodology native to the project by labeling this issue for cherry-pick.

I have marked the PR as a draft because I was not sure about the process to "backport" this in the project. I assumed we would need to follow some formal process to do that, I just wanna see if the backport was easy to do.

Thanks for the rest of the comments. I will review them and will come back asap.

[ilrudie]

Will you be able to backport this fix to 1.28?

Yeah sur, no problem. Should be something like #1801

We'd typically only cherry-pick bug fixes. This is a grey area IMO. It could be seen as a bug of omission. At any rate, If we do want to consider this a bug we should use the cherry-pick methodology native to the project by labeling this issue for cherry-pick.

I have marked the PR as a draft because I was not sure about the process to "backport" this in the project. I assumed we would need to follow some formal process to do that, I just wanna see if the backport was easy to do.

Thanks for the rest of the comments. I will review them and will come back asap.

I started asking release managers in slack. I'm not sure if they will accept it or not but I labeled the PR for cherry-pick to 1.28 and 1.29 and we can sort that out in the release branches.

[ilrudie]

Couple minor comments. Thanks for the productive discussions.

@fjglira, I think this could merge very soon. It would be good to gather the needed opinions about cherry-picks as soon as we can.

[ilrudie]

Thanks for the discussion. I agree we should have time to rotate out the client in most reasonable cases so this is probably a suitable implementation with a good balance of complexity.

It might be nice to add how long we waited for the write lock to the debug message. This way if we suspect contention, we already have some information that can be made available to troubleshoot. I don't think we need to immediately introduce more complexity though.

[fjglira]

According to the discussion on slack we can manually cherry-pick this changes as a bug fix but let’s add a flag to allow user to turn off the watcher. On master we can keep the current behavior

[ilrudie]

lgtm, thanks for working on this!