Use `RUNBOOK_URL_TEMPLATE` env for the runbooks URL template #524

assafad · 2023-03-14T18:39:16Z

What this PR does / why we need it:
When RUNBOOK_URL_TEMPLATE env variable is set, its value will be used as the runbooks URL template. Otherwise, keep using the default runbook URL template https://kubevirt.io/monitoring/runbooks/%s.
This allows using different runbooks versions, in accordance with the installation.

Which issue(s) this PR fixes:
Jira-ticket - https://issues.redhat.com/browse/CNV-14382

Special notes for your reviewer:

Release note:

None

openshift-ci · 2023-03-14T18:39:27Z

Hi @assafad. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

assafad · 2023-03-14T18:39:30Z

/hold

0xFelix · 2023-03-15T17:48:38Z

Thanks! Can you add tests for the new feature please?

akrejcir · 2023-03-16T08:47:01Z

Does the env variable have to be a format string, or will it always be a prefix?

In my opinion, accepting a format string as an env variable doesn't seem safe. It could probably cause a panic, if the formatting parameters are incorrect.

assafad · 2023-03-19T13:08:37Z

Blocked by kubevirt/monitoring#165

assafad · 2023-03-19T13:13:16Z

Thanks! Can you add tests for the new feature please?

@0xFelix Hi, I've added some tests on internal/operands/metrics/reconcile_test.go. Beware that we also have this test, that checks if the runbooks are accessible. Please let me know if the new tests make sense.

assafad · 2023-03-19T13:22:31Z

Does the env variable have to be a format string, or will it always be a prefix?

In my opinion, accepting a format string as an env variable doesn't seem safe. It could probably cause a panic, if the formatting parameters are incorrect.

@akrejcir Not sure I fully understand your concern. We don't assume that RUNBOOK_URL_TEMPLATE is a string (os.LookUpEnv() returns a string), and don't assume it's a prefix. Different installations may use different runbooks versions, thus we only assume (and verify) the template contains exactly 1 %s, as runbook URLs contain the alerts names.

0xFelix · 2023-03-21T09:43:22Z

@akrejcir wanted to point out that you can inject arbitrary format specifiers when reading RUNBOOK_URL_TEMPLATE from the envs and using it as a format string (e.g. https://kubevirt.io/monitoring/runbooks/%d, what will happen?). Can't you make it a prefix instead, so it is safe to read from the envs?

assafad · 2023-03-22T10:46:13Z

@akrejcir wanted to point out that you can inject arbitrary format specifiers when reading RUNBOOK_URL_TEMPLATE from the envs and using it as a format string (e.g. https://kubevirt.io/monitoring/runbooks/%d, what will happen?). Can't you make it a prefix instead, so it is safe to read from the envs?

@0xFelix, @akrejcir For now, U/S URLs template is indeed a prefix, but different installations/versions have different documentation requirements. Thus, assuming the alerts names are suffix is not generic enough and might require future updates. I validated here that RUNBOOK_URL_TEMPLATE value contains exactly 1 %s, and in addition we have this test that verifies the runbooks are accessible. Let me know if expanding the %s validation can reduce your concern.

akrejcir · 2023-03-22T13:09:52Z

Ok, we can keep this for now. If the pattern is incorrect, the resulting string will contain an error message https://pkg.go.dev/fmt#hdr-Format_errors . So it will not panic.

akrejcir · 2023-03-22T13:11:28Z

/lgtm

0xFelix · 2023-03-24T08:30:38Z

@akrejcir @assafad So it will not panic but instead will fail silently? IIUC the formatted string will contain the error message, but we don't check that?

akrejcir · 2023-03-24T12:48:11Z

@akrejcir @assafad So it will not panic but instead will fail silently? IIUC the formatted string will contain the error message, but we don't check that?

Right, it's not a good solution.

@assafad, can you change the code, so that the env variable RUNBOOK_URL_TEMPLATE will contain our own simple template format with a placeholder, that will be replaced using strings.Replace().
For example:

https://kubevirt.io/monitoring/runbooks/{}

Where {} will be replaced.

akrejcir · 2023-03-24T12:48:18Z

/lgtm cancel

assafad · 2023-04-18T08:09:00Z

/retest
/unhold

0xFelix · 2023-04-18T08:20:33Z

internal/operands/metrics/resources.go

+	}
+
+	if strings.Count(runbookURLTemplate, "%s") != 1 || strings.Count(runbookURLTemplate, "%") != 1 {
+		panic(errors.New("runbook URL template must have exactly 1 %s substring"))


Can you return an error instead of panicking? The error is caused by user input and that could be handled.

Updated, please let me know if the change makes sense

assafad · 2023-04-18T11:16:43Z

/ok-to-test

assafad · 2023-04-19T09:16:02Z

/retest

internal/operands/metrics/reconcile_test.go

internal/operands/metrics/resources.go

0xFelix

Just one more nit, otherwise looks good to me!

0xFelix · 2023-04-21T13:54:05Z

internal/operands/metrics/reconcile_test.go

+		},
+		Entry("should use the default template when no ENV variable is set", defaultRunbookURLTemplate),
+		Entry("should use the ENV variable when a valid value is set", "valid/runbookURL/template/%s"),
+		Entry("should throw an error when an invalid ENV variable value is set", "invalid/runbookURL/template/"),


Add negative test case for another format specifier, e.g. %d?

Signed-off-by: assafad <aadmi@redhat.com>

sonarcloud · 2023-04-24T12:14:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.2% Duplication

assafad · 2023-04-25T08:53:07Z

/retest

0xFelix · 2023-04-25T13:06:17Z

Thank you! It looks good to me!

@akrejcir WDYT?

/approve

kubevirt-bot · 2023-04-25T13:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [0xFelix]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akrejcir · 2023-04-25T13:13:59Z

Looks good, thanks.

/lgtm

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 14, 2023

kubevirt-bot requested review from akrejcir and lyarwood March 14, 2023 18:39

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 14, 2023

kubevirt-bot added size/M do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 14, 2023

kubevirt-bot added size/L and removed size/M labels Mar 19, 2023

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 22, 2023

kubevirt-bot assigned akrejcir Mar 22, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2023

0xFelix reviewed Apr 18, 2023

View reviewed changes

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2023

0xFelix reviewed Apr 20, 2023

View reviewed changes

internal/operands/metrics/reconcile_test.go Outdated Show resolved Hide resolved

internal/operands/metrics/resources.go Outdated Show resolved Hide resolved

internal/operands/metrics/resources.go Outdated Show resolved Hide resolved

0xFelix reviewed Apr 21, 2023

View reviewed changes

Use RUNBOOK_URL_TEMPLATE env for the runbooks URL template

875a709

Signed-off-by: assafad <aadmi@redhat.com>

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2023

kubevirt-bot merged commit 77a99cd into kubevirt:master Apr 25, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `RUNBOOK_URL_TEMPLATE` env for the runbooks URL template #524

Use `RUNBOOK_URL_TEMPLATE` env for the runbooks URL template #524

assafad commented Mar 14, 2023 •

edited

openshift-ci bot commented Mar 14, 2023

assafad commented Mar 14, 2023

0xFelix commented Mar 15, 2023

akrejcir commented Mar 16, 2023

assafad commented Mar 19, 2023

assafad commented Mar 19, 2023

assafad commented Mar 19, 2023 •

edited

0xFelix commented Mar 21, 2023 •

edited

assafad commented Mar 22, 2023 •

edited

akrejcir commented Mar 22, 2023

akrejcir commented Mar 22, 2023

0xFelix commented Mar 24, 2023

akrejcir commented Mar 24, 2023

akrejcir commented Mar 24, 2023

assafad commented Apr 18, 2023

0xFelix Apr 18, 2023

assafad Apr 18, 2023

assafad commented Apr 18, 2023

assafad commented Apr 19, 2023

0xFelix left a comment

0xFelix Apr 21, 2023

assafad Apr 24, 2023

sonarcloud bot commented Apr 24, 2023

assafad commented Apr 25, 2023

0xFelix commented Apr 25, 2023

kubevirt-bot commented Apr 25, 2023

akrejcir commented Apr 25, 2023

Use RUNBOOK_URL_TEMPLATE env for the runbooks URL template #524

Use RUNBOOK_URL_TEMPLATE env for the runbooks URL template #524

Conversation

assafad commented Mar 14, 2023 • edited

openshift-ci bot commented Mar 14, 2023

assafad commented Mar 14, 2023

0xFelix commented Mar 15, 2023

akrejcir commented Mar 16, 2023

assafad commented Mar 19, 2023

assafad commented Mar 19, 2023

assafad commented Mar 19, 2023 • edited

0xFelix commented Mar 21, 2023 • edited

assafad commented Mar 22, 2023 • edited

akrejcir commented Mar 22, 2023

akrejcir commented Mar 22, 2023

0xFelix commented Mar 24, 2023

akrejcir commented Mar 24, 2023

akrejcir commented Mar 24, 2023

assafad commented Apr 18, 2023

0xFelix Apr 18, 2023

Choose a reason for hiding this comment

assafad Apr 18, 2023

Choose a reason for hiding this comment

assafad commented Apr 18, 2023

assafad commented Apr 19, 2023

0xFelix left a comment

Choose a reason for hiding this comment

0xFelix Apr 21, 2023

Choose a reason for hiding this comment

assafad Apr 24, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Apr 24, 2023

assafad commented Apr 25, 2023

0xFelix commented Apr 25, 2023

kubevirt-bot commented Apr 25, 2023

akrejcir commented Apr 25, 2023

Use `RUNBOOK_URL_TEMPLATE` env for the runbooks URL template #524

Use `RUNBOOK_URL_TEMPLATE` env for the runbooks URL template #524

assafad commented Mar 14, 2023 •

edited

assafad commented Mar 19, 2023 •

edited

0xFelix commented Mar 21, 2023 •

edited

assafad commented Mar 22, 2023 •

edited