Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameterize config watcher loop speed #1232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

connorkuehl
Copy link

@connorkuehl connorkuehl commented Feb 15, 2024

This loop spins really quickly otherwise. This will allow cluster operators to choose how quickly the thin plugin reconciles the kubeconfig in the event of a stale service account token.

@s1061123
Copy link
Member

Thank you for the PR, @connorkuehl

Currently multus thin_entrypoint assumes that Kubeconfig is not updated by deployment because this modification might be reverted again when cert is rotated. Hence thin_entrypoint supports only 'generated Kubeconfig without mody out-of-band' and thin_entrypoint does not support 'Kubeconfig with modify out-of-band'.

@connorkuehl
Copy link
Author

@s1061123 Ha, yeah, I was trying to test the Kubeconfig regeneration when the service account token changes but I couldn't figure out how to manually rotate the service account token, so I figured I could test by mutating the Kubeconfig and making sure that it did regenerate properly.

If I drop that patch, would you still consider the patch that parameterizes how quickly the loop executes?

@connorkuehl connorkuehl changed the title Reduce stale kubeconfig hashes in the config reconciler loop Parameterize config watcher loop speed Feb 26, 2024
@connorkuehl
Copy link
Author

@s1061123 I dropped the kubeconfig hashing patch. Would you be willing to consider the remaining patch for controlling how quickly the loop spins?

@dougbtv
Copy link
Member

dougbtv commented Feb 29, 2024

can we keep the default second wait? if we'd like to we can update the sample daemonset to include the minute long wait

also let's update the docs/how-to-use.md and/or docs/configuration.md to reflect the option as well

@coveralls
Copy link

coveralls commented Feb 29, 2024

Coverage Status

coverage: 62.975% (+0.2%) from 62.778%
when pulling d2d1bb2 on connorkuehl:stale-kubeconfig-hash
into 0fd3fa7 on k8snetworkplumbingwg:master.

@connorkuehl
Copy link
Author

can we keep the default second wait? if we'd like to we can update the sample daemonset to include the minute long wait

also let's update the docs/how-to-use.md and/or docs/configuration.md to reflect the option as well

Sure thing! I've pushed a new revision setting the default value to 1 second and also added to the how-to-use.md. Thanks for the review 🙂

Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

LGTM! Appreciate the contribution.

@connorkuehl
Copy link
Author

connorkuehl commented Mar 22, 2024

Thanks, @dougbtv! It looks like the CI failure is unrelated to the PR. Would you mind retriggering it?

edit: rebased

This loop spins really quickly otherwise. This will allow cluster
operators to choose how quickly the thin plugin reconciles the
kubeconfig in the event of a stale service account token.

Signed-off-by: Connor Kuehl <[email protected]>
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@connorkuehl
Copy link
Author

@dougbtv @s1061123 just checking to see if either of you had any other feedback on the patch

@mingshuoqiu
Copy link

@dougbtv @s1061123 gentle ping. Do you still have any concern to merge the PR?

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.

None yet

5 participants