Skip to content

Don't create NodePasswordValidationFailed event if agent is disabled#9312

Merged
brandond merged 1 commit intok3s-io:masterfrom
matskiv:remove-pw-validation-event-when-no-agent
Feb 9, 2024
Merged

Don't create NodePasswordValidationFailed event if agent is disabled#9312
brandond merged 1 commit intok3s-io:masterfrom
matskiv:remove-pw-validation-event-when-no-agent

Conversation

@matskiv
Copy link
Copy Markdown
Contributor

@matskiv matskiv commented Jan 29, 2024

Proposed Changes

When the agent is disabled, e.g. when k3s image is used in a vcluster, a "NodePasswordValidationFailed" Event is created on every restart. This results in users questioning the state of their cluster, even though there is no impact on functionality.
The proposed change would create a debug log instead of a user-facing event.

Types of Changes

enhancment/bug fix?

Verification

Testing

Linked Issues

User-Facing Change

The `NodePasswordValidationFailed` Events will no longer be emitted, if the agent is disabled.

Further Comments

I am open to discussion and suggestion on better implementation.

@matskiv matskiv requested a review from a team as a code owner January 29, 2024 20:35
Copy link
Copy Markdown
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.

Thanks for the contribution!

I don't think this is the correct place to do this. I think we only want to disable node password validation for the local node, so we should just return early from verifyLocalPassword before the deferred validation is enqueued, if the agent is disabled:

k3s/pkg/server/router.go

Lines 516 to 520 in 6d77b7a

if _, ok := deferredNodes[node.Name]; !ok {
deferredNodes[node.Name] = true
go ensureSecret(ctx, config, node)
logrus.Infof("Password verified locally for node %s", node.Name)
}

We don't want to disable ALL node password validation just because the server doesn't have a local agent. There may still be agents, and we do want to validate their passwords.

@matskiv matskiv force-pushed the remove-pw-validation-event-when-no-agent branch 2 times, most recently from e9eaaee to 4a3eeb0 Compare January 30, 2024 18:13
@matskiv
Copy link
Copy Markdown
Contributor Author

matskiv commented Jan 30, 2024

@brandond Thank you for the quick review! Good points! I hope this iteration addresses the comments in a acceptable way.

Signed-off-by: Oleg Matskiv <oleg.matskiv@gmail.com>
@matskiv matskiv force-pushed the remove-pw-validation-event-when-no-agent branch from 4a3eeb0 to 5e7b8a4 Compare February 5, 2024 21:22
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6d77b7a) 40.56% compared to head (5e7b8a4) 41.05%.
Report is 26 commits behind head on master.

Files Patch % Lines
pkg/server/router.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9312      +/-   ##
==========================================
+ Coverage   40.56%   41.05%   +0.48%     
==========================================
  Files         154      154              
  Lines       16555    16626      +71     
==========================================
+ Hits         6716     6825     +109     
+ Misses       8692     8646      -46     
- Partials     1147     1155       +8     
Flag Coverage Δ
inttests 37.56% <25.00%> (-0.17%) ⬇️
unittests 15.42% <ø> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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