Skip to content

Conversation

@tertsdiepraam
Copy link
Contributor

@tertsdiepraam tertsdiepraam commented Aug 28, 2025

No description provided.

Copy link
Contributor

@bal-e bal-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I'm not sure this code is in the right place. I believe it will be easier to fit in the key manager once some of my state stuff is done, but we can merge this in the meanwhile and I'll adjust it as part of my integration PR.

@ximon18
Copy link
Member

ximon18 commented Aug 31, 2025

@bal-e thought this should be done by the KeyManager. She might be right, but feels like a bit of a hassle right now? I'm not sure.

One problem with invoking dnst keyset create and dnst keyset init in ZoneLoader is that to use a KMIP server we will need to add a call to dnst keyset kmip add-server between those other two dnst keyset commands, and that will require passing KMIP server details to the ZoneLoader which otherwise has no need of that information at all, in fact ZoneLoader has no need to know that dnst keyset even exists. So yes it makes more sense to do this in the Key Manager.

@Philip-NLnetLabs
Copy link
Member

keyset create and keyset init should be done in the module that also applies the policy to keyset. Because the policy has to be set between init and create.

@tertsdiepraam
Copy link
Contributor Author

Alright, seems like everyone is in favor of the key manager. I'll try to update the PR.

@tertsdiepraam tertsdiepraam force-pushed the zone-register-keyset branch 4 times, most recently from f747e64 to 57af651 Compare September 1, 2025 15:26
@tertsdiepraam tertsdiepraam force-pushed the zone-register-keyset branch 5 times, most recently from 383f508 to 2ce466e Compare September 3, 2025 10:18
@tertsdiepraam tertsdiepraam merged commit 68c0728 into main Sep 3, 2025
27 checks passed
@tertsdiepraam tertsdiepraam deleted the zone-register-keyset branch September 8, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants