search

k8snetworkplumbingwg / multus-cni

A CNI meta-plugin for multi-homed pods in Kubernetes
Apache License 2.0
2.36k stars 585 forks source link

Parameterize config watcher loop speed #1232

Closed connorkuehl closed 2 months ago

connorkuehl commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

dougbtv commented 7 months ago

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 commented 7 months ago

Coverage Status

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

connorkuehl commented 7 months ago

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 :slightly_smiling_face:

connorkuehl commented 6 months ago

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

edit: rebased

connorkuehl commented 6 months ago

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

mingshuoqiu commented 4 months ago

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

mingshuoqiu commented 3 months ago

@dougbtv @s1061123 gentle ping. Do I need anyone else to review to speed up the merge progress? Thanks