Skip to content

Conversation

vitorsavian
Copy link
Member

@vitorsavian vitorsavian commented Jul 3, 2025

Proposed Changes

  • Use a wrapper for easy access to the listener config

Why

  • K3s need to regenerate leaf certs with hot reload without the need for restart
  • the main idea is to also return the config that was used for creating the listener, since the best way to regenerate/reload the dynamic listener cert on demand is to use the listener that was created in NewListenerWithChain initial setup.

@vitorsavian vitorsavian changed the title Wrap Listener to also return dynamiclistener config [WIP] Wrap Listener to also return dynamiclistener config Jul 3, 2025
@vitorsavian vitorsavian changed the title [WIP] Wrap Listener to also return dynamiclistener config Wrap Listener to also return dynamiclistener config Jul 3, 2025
@vitorsavian vitorsavian marked this pull request as ready for review July 3, 2025 19:11
@vitorsavian vitorsavian requested review from brandond and a team July 3, 2025 19:28
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Couple nits.

How do you envision this being called from the K3s side? You're just planning on calling listener.RegenerateCerts directly from K3s?

If you add the ability to do that directly, you might as well remove the RegenerateCerts callback that can be set in the config, but is only called once from NewListenerWithChain

RegenerateCerts func() bool

if config.RegenerateCerts != nil && config.RegenerateCerts() {
if err := dynamicListener.regenerateCerts(); err != nil {
return nil, nil, err
}

Signed-off-by: Vitor Savian <[email protected]>
@vitorsavian vitorsavian requested a review from brandond July 7, 2025 18:27
@brandond
Copy link
Member

brandond commented Jul 7, 2025

Any plans to address the duplication of function with the existing RegenerateCerts callback? I think we should just get rid of it since K3s is the only user that I am aware of, and this would replace that.

@vitorsavian
Copy link
Member Author

I thought we would have another user that used dynamic listener, but since we are the only users, I will remove that.

@brandond
Copy link
Member

brandond commented Jul 7, 2025

We are actually the only users of NewListener/NewListenerWithChain - other rancher projects all just use server.ListenAndServe

@brandond
Copy link
Member

brandond commented Sep 8, 2025

@vitorsavian do we still need this?

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.

2 participants