-
Notifications
You must be signed in to change notification settings - Fork 6
Shows which security tools are detected in host support bundle spec #2890
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
base: main
Are you sure you want to change the base?
Conversation
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer:
Airgap Installer (may take a few minutes before the airgap bundle is built):
Happy debugging! |
The following antivirus or network security tool(s) was detected: | ||
{{ "{{" }} .Detected {{ "}}" }} | ||
These types of tools have been known to interfere with Kubernetes operation in various ways. If an installation problem persists, you may need to disable these tools temporarily as part of the troubleshooting process to identify if any system administrator exceptions may be required to maintain necessary internal Kubernetes operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what our goal is here. This is a failure, so it's going to block the user from proceeding (unless they pass the --bypass-host-preflights
flag). But then the message says that if an installation problem persists, you might want to disable these. Which sounds like we assume the install will proceed but might fail. in a sense, that feels more like a warning so that the install would proceed but the user would be notified so they could reflect on that later if the install fails. we have thus far stayed away from warnings in ec host preflights because i feel they're ambiguous for a user. this could be a place for one, but i'm also open to keeping it a failure, though i think other wording should change if we stick with fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, we are trying to soften the message from the prior "just turn off the tools" messaging which has been flagged as sounding like advising an unacceptable bad practice. But the prior messaging we replaced is pretty vague too. It also sounds like "if the problem persists..." but if you're saying it always fails in the presence of these tools being detected, that prior messaging wasn't actually helpful at all.
Old messaging
Antivirus or network security tools detected. These tools are known to interfere with Kubernetes operation in various ways. If problems persist, disable these tools, or consult with your organization's system administrator to ensure that exceptions are made for Kubernetes operation."
We want to accomplish the following:
- Warn that we detect these tools, and that they have been known to potentially interfere with kubernetes operations in a way that may prevent successful install or subsequent upgrade. That gives people a path to start troubleshooting any install failure
- Output what tool we actually detected, so (1) vendor and support know this, and (2) we start getting data on it
Given above it feels like it should be a warn right? These tools 'can' but aren't guaranteed to impact expected operations, so it doesn't feel like they should block just by their presence alone.
I wish I had the context on why this was made a failure. Maybe because we only had failures at the time. @banjoh do you have any context here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this change is in the host support bundle spec, not the host preflight spec
Embedded cluster has not had security tools check in host preflights, only in host support bundles. I believe the check was made a failure to make it prominent in Vendor Portal when a support bundle is uploaded for vendors to pay attention to it. @diamonwiggins is that accurate? You might recall since you contributed the change?
As a side note, this check and many others in EC were copied from our collection of troubleshoot host support bundle specs. Older commit history can be found there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking of this as preflights, so that's my fault. tbh it could be a good warning preflight because this is going to be much more reactive than proactive. you'll have to wait for the install to fail, tell your vendor, figure out you should generate a support bundle, etc. so maybe we should consider a warn host preflight.
still seems like a warn to be in the context of support bundles, since it's not necessarily the cause of the problem, though it's likely to be. but i'm actually fine with this wording for support bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although in my defense, the PR is titled "host preflight," and so is the shortcut story. so maybe this was supposed to be a host preflight but instead the change was made to the support bundle spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True on the title. That's my fault as it was called a host preflight in a convo that generated this ticket and i should have noticed the code was actually support bundle. lol. confusion all around. Ok so to recap:
- @ajp-io it sounds like you're fine from a product standpoint of this remaining the wording in the support bundle messaging, and that it's ok that it's a fail right now, since it's not actually blocking the installation at this point.
- I agree a matching host preflight would be nice to be more proactive as well, especially if we move forward with the idea of running them more in advance of the day of install. Do you have preferred messaging in this use case? We can spin out another story for that improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also updated the story title and pr title to reduce future confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like a warning for a support bundle too, given that we're not saying this 100% was an issue. but if it won't be noticed in VP without being a fail, then that's fine.
Co-authored-by: Salah Al Saleh <[email protected]>
What this PR does / why we need it:
This PR changes host preflight checks to show which security tools are detected, instead of just saying some were found, and changes wording to not make it seem like the security tools have to be permanently disabled
Which issue(s) this PR fixes:
https://app.shortcut.com/replicated/story/129209/host-preflights-improve-messaging-around-security-tools-detected
Does this PR require a test?
NONE
Does this PR require a release note?
Does this PR require documentation?
NONE