-
-
Notifications
You must be signed in to change notification settings - Fork 147
BE: Allow disabling GitHub release info #1108
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
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.
Hi stklcode! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
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.
Hi, thanks for the PR. I've left some inline comments, PTAL!
if (githubInfoEnabled) { | ||
this.githubReleaseInfo = new GithubReleaseInfo(githubApiMaxWaitTime); | ||
} else { | ||
this.githubReleaseInfo = null; |
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.
let's either mark the field as nullable or replace it with some empty object instead to prevent NPEs
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.
Let's also output a bold warning into logs on startup in case this is disabled that running outdated versions is dangerous due to CVEs and we will not provide support for outdated versions per our security policy
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.
Let's also output a bold warning into logs on startup ...
A real "warning", i.e. WARN log level? INFO ... OK, but warning seems little overdone. I'm running about 400 instances in various test clusters and remote systems, no online access, updates and security scans are done by external tooling. Simply shifting Firewall/eBPF warnings to application warnings was not what I had in mind....
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.
It might seem "overdone" until folks start coming up with air-gapped installations requesting support for 2 yo releases.
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 get the point. But does a warning really prevent these folks from requesting anything? My experience is you can write "UNMAINTEINED" in 50px bold at the top of a GH issue template and you still get new tickets with the warning not even removed 🤷♂️
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.
Nevertheless I've added a log warning. (yet another one to ignore, but at least consistent with other config scenarios).
WARN [restartedMain] i.k.u.s.ApplicationInfoService: Check for latest release is disabled. Note that old versions are not supported, please make sure that your system is up to date.
@@ -8,6 +8,7 @@ | |||
|
|||
@Slf4j | |||
public class GithubReleaseInfo { | |||
public static final String GITHUB_RELEASE_INFO_ENABLED = "github.release.info.enabled"; |
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 document this property in the docs too: https://github.com/kafbat/ui-docs
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.
Done: kafbat/ui-docs#56
Also added the 2 pre-existing properties for this feature (timeout, update-rate)
@@ -19,7 +19,7 @@ const Version: React.FC = () => { | |||
|
|||
return ( | |||
<S.Wrapper> | |||
{!isLatestRelease && ( | |||
{isLatestRelease === false && ( |
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'd like to see a visual difference between "no latest release present" and "user disabled version check" in the UI. This is crucial as it's often only the UI screenshots I have to deduct things from.
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.
Any suggestion how such information should look?
Frankly, I don't see any benefit in such information. Currently running version is displayed, if the check failed there's a warning symbol and the information if there's an update available or not can change 1 second after a screenshot was taken, because it's dynamic info.
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.
Perhaps a red (rather than yellow) exclamation mark icon (most likely it's an svg, so altering stroke-color should work) with a different text about version check disabled.
The benefit is offloading responsibility of running an oudated and possibly CVE-"enriched" version and giving the maintainers visual signal of what the user's running (red icon -> no version check = most likely outdated -> no support)
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.
A red warning, really?
I've learned to ignore the yellow icon due do failing version checks and yet there are frequent questions if there's something wrong. So annoying users, who have no control whatsoever about the deployed version in our case, with read warning sign that typically means "something is wrong here" ...
Our use case are - you guessed it - isolated systems, mostly dev/staging environments. We allow users some access to the underlying Kafka and deployment/updates are done by some automation/opt team who don't see the UI warnings anyway.
We were looking for a solution to remove the frontend warning, not making it look worse.
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.
You wouldn't imagine the number of issues I've seen where people come with their "up-to-date" installations, only to realize after wasting some time that it's not only an outdated version, but also a fork with completely unrelated functionality that we don't even have. Not sure of a middle ground here. @germanosin thoughts?
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.
On the oder end, I've seen questions from users about the warning of "UNKNOWN" version and regular tickets from ops about log warnings that nobody can do anything about.
We have filters on our network logs to prevent alerting from unauthorized access attempts and injected a logging config to suppress the "Authentication is disabled" warning for instances running behind an external auth gateway. But we cannot filter the human eye and regular "just ignore the warning" is not really a satisfactory answer.
Evaluated red and gray symbol (current yellow and none for reference). My preference is "none" or maybe gray 😉
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.
My personal preference: gray
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 extended the <WarningIcon>
component to (a) use theme's warningIcon
color instead of hardcoded color (was #F2C94C
, now #FFDD57
... close enough for me) and (b) allow override by parameter.
Override to infoIcon
color (#ABB5BA
) with adjusted message in case the check info is not available.
f1f15de
to
3c4131c
Compare
The application info object contains details about the latest version which is queried online from GitHub API. While this info can be useful, it raises warning when Kafbat UI is deployed in environments with restricted internet access. Introduce a new property "github.release.info.enabled" (default: true) that can be set to "false" to disable release info and just display the currently running version/commit.
3c4131c
to
bc55bce
Compare
Extend he WarningIcon template to allow custom coloring with fallback to theme's "warningIcon" color. If version check is disabled, show a corresponding info with a "infoIcon" (gray) colored icon instead.
c3a87e3
to
c4f2562
Compare
What changes did you make?
The application info object contains details about the latest version which is queried online from GitHub API. While this info can be useful, it raises warning when Kafbat UI is deployed in environments with restricted internet access.
Introduce a new property
github.release.info.enabled
(default:true
) that can be set tofalse
to disable release info and just display the currently running version/commit.Is there anything you'd like reviewers to focus on?
The original feature was introduced in provectus/kafka-ui#3570
Apparently, the issue was already known during development (provectus/kafka-ui#3570 (review)):
Did I miss anything or was this point just ignored? Anyway with this PR now there it is.
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
For manual testing, start the app with environment variable or property, e.g.
GITHUB_RELEASE_INFO_ENABLED=false
.Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
PR for documentation update: kafbat/ui-docs#56