fix: Use the latest image of vm-console-proxy #645

akrejcir · 2023-08-08T12:51:50Z

What this PR does / why we need it:

Updated release script to change vm-console-proxy image tag.
csv-generator uses VM_CONSOLE_PROXY_IMAGE environment variable to set the image.

Release note:

csv-generator can be used to set the image of vm-console-proxy.

akrejcir · 2023-08-08T12:53:04Z

/cc @codingben @0xFelix

codingben

/lgtm

EDIT: Seems like this PR needs more work since a new vm-console-proxy v0.3.0 contains breaking changes.

akrejcir · 2023-08-08T13:51:33Z

This needs more work.

akrejcir · 2023-08-11T13:50:20Z

This is nearly complete, it's only missing one functional test.
Can you take a look?

/cc @0xFelix @lyarwood

0xFelix

Good start, added some comments!

0xFelix · 2023-08-14T13:27:42Z

config/rbac/role.yaml

Have you checked that these are the minimal required RBAC changes?

Yes, they are minimal in the sense of groups:

read permissions: get, list, watch

write permissions: create, update, patch, delete.

In my opinion it does not make sense to grant list permission and not get permission. Because list can be used to get a single element anyway. Similarly for update and patch.

@jcanocan what was the logic you applied with your previous RBAC audits?

I tend to agree with @0xFelix here that we shouldn't be granting everything even if a list basically provides get etc but lets stay consistant with @jcanocan 's previous work here and maybe write down the resulting best practice somewhere.

Ah @jcanocan is out this week, I'm okay leaving this as is for now if we write up a follow issue for next week to maybe reduce this down and document the best practice.

If we want to be more precise, It's harder to know what permissions libraries require, without running and failing, or reading the library code.

Yeah appreciate that, definitely a follow up task now given @jcanocan isn't around anyway.

Hi all! The methodology used for auditing the rules is described here: #616. Basically, I've removed all verbs from all apiGroups, and I've added requested verbs. @akrejcir is right that it's hard to know in advance which permissions are required in advance. We are limited to trial and error in this sense.

0xFelix · 2023-08-14T13:29:00Z

internal/operands/vm-console-proxy/reconcile.go

+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings;rolebindings,verbs=list;watch;create;update;delete
+// +kubebuilder:rbac:groups=apiregistration.k8s.io,resources=apiservices,verbs=get;list;watch;create;update;delete
+
+// Deprecated:
+// +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=list;watch;delete

 // RBAC for created roles
-// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances,verbs=get;list;watch
+// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch
+// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;list;watch;create;update;delete;patch
+// +kubebuilder:rbac:groups=core,resources=serviceaccounts/token,verbs=create
+// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances;virtualmachines,verbs=get;list;watch
+// +kubebuilder:rbac:groups=subresources.kubevirt.io,resources=virtualmachineinstances/vnc,verbs=get
+// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;delete;patch


Same as above.

internal/operands/vm-console-proxy/reconcile_test.go

0xFelix · 2023-08-14T13:33:58Z

internal/operands/vm-console-proxy/reconcile_test.go

+			route := &routev1.Route{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      routeName,
+					Namespace: namespace,


Same as above.

Do these test cases add any value compared to the same ones above?

Yes, this case tests the Cleanup() method, and the above one tests Reconcile() method.

internal/vm-console-proxy-bundle/bundle.go

tests/vm_console_proxy_test.go

codingben · 2023-08-15T12:17:24Z

.github/workflows/release-vm-console-proxy.yaml

@@ -17,6 +17,7 @@ jobs:
          OUTPUT_FILE=./data/vm-console-proxy-bundle/vm-console-proxy.yaml
          mkdir -p ./data/vm-console-proxy-bundle
          curl -L https://github.com/kubevirt/vm-console-proxy/releases/download/${RELEASE_VERSION}/vm-console-proxy.yaml > ${OUTPUT_FILE}
+          sed -i "s/defaultVmConsoleProxyImageTag = .*$/defaultVmConsoleProxyImageTag = \"${RELEASE_VERSION}\"/" ./internal/operands/vm-console-proxy/defaults.go 


IMHO, I don't think we should do that bump here.

This workflow is used to update vm-console-proxy bundle only

We try to change code during that workflow, what if defaults.go will be removed in the future?

Why not just use latest tag? if anyways this intended to be updated on every release to latest one, just use latest then: quay.io/kubevirt/vm-console-proxy:latest.

This workflow is used to update vm-console-proxy bundle only

This workflow is used to update SSP operator code, so that it uses the latest release of vm-cosole-proxy. It can update the image tag and bundle.

We try to change code during that workflow, what if defaults.go will be removed in the future?

I will add a comment to defaults.go explaining how the file is used.

Why not just use latest tag? if anyways this intended to be updated on every release to latest one, just use latest then: quay.io/kubevirt/vm-console-proxy:latest.

There is no latest tag. We would need to add automation to create and update it.
https://quay.io/repository/kubevirt/vm-console-proxy?tab=tags&tag=latest

Using the latest tag would probably not save us work, because we would need to fix the version when creating a release branch.

lyarwood · 2023-08-15T21:04:58Z

/lgtm

lyarwood · 2023-08-15T21:05:06Z

/retest-required

akrejcir · 2023-08-16T07:05:21Z

/retest

akrejcir · 2023-08-16T13:02:24Z

/retest

akrejcir · 2023-08-16T14:30:00Z

@ksimon1 , are the CI failures related to the CDI issue?

akrejcir · 2023-08-21T14:46:25Z

/retest

akrejcir · 2023-08-21T17:35:48Z

/retest

jcanocan · 2023-08-22T08:36:15Z

/retest

akrejcir · 2023-08-23T09:57:05Z

We cannot merge this yet.
Incorrect implementation of APIService extension in vm-cosole-proxy is blocking the deletion of any namespace in the cluster.

Holding until a fixed version of vm-console-proxy is released.

/hold

akrejcir · 2023-08-29T15:48:41Z

I've fixed the bug in vm-cosole-proxy: kubevirt/vm-console-proxy#27
And released a new version.

/unhold

- Updated data/vm-console-proxy-bundle/vm-console-proxy.yaml - Updated release script to change vm-console-proxy image tag. - csv-generator uses VM_CONSOLE_PROXY_IMAGE env variable to set the image. - Operator deploys new resources needed by vm-console-proxy. - Operator removes Route resource, that is no longer needed. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

sonarcloud · 2023-08-30T08:27:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

lyarwood · 2023-08-30T08:43:02Z

/lgtm

jcanocan · 2023-08-30T09:03:36Z

/lgtm

akrejcir · 2023-08-30T10:58:46Z

/cherry-pick release-v0.18

kubevirt-bot · 2023-08-30T10:59:21Z

@akrejcir: #645 failed to apply on top of branch "release-v0.18":

Applying: feat: Use the latest image of vm-console-proxy
.git/rebase-apply/patch:32: trailing whitespace.
          sed -i "s/defaultVmConsoleProxyImageTag = .*$/defaultVmConsoleProxyImageTag = \"${RELEASE_VERSION}\"/" ./internal/operands/vm-console-proxy/defaults.go 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	config/rbac/role.yaml
M	data/olm-catalog/ssp-operator.clusterserviceversion.yaml
M	internal/common/environment.go
M	internal/operands/vm-console-proxy/reconcile.go
Falling back to patching base and 3-way merge...
Auto-merging internal/operands/vm-console-proxy/reconcile.go
CONFLICT (content): Merge conflict in internal/operands/vm-console-proxy/reconcile.go
Auto-merging internal/common/environment.go
Auto-merging data/olm-catalog/ssp-operator.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in data/olm-catalog/ssp-operator.clusterserviceversion.yaml
Auto-merging config/rbac/role.yaml
CONFLICT (content): Merge conflict in config/rbac/role.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 feat: Use the latest image of vm-console-proxy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.18

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.

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 Aug 8, 2023

kubevirt-bot requested review from codingben and jcanocan August 8, 2023 12:51

kubevirt-bot added the size/M label Aug 8, 2023

akrejcir force-pushed the fix-proxy-image branch from 0b6885f to 803a13f Compare August 8, 2023 12:52

kubevirt-bot requested a review from 0xFelix August 8, 2023 12:53

codingben approved these changes Aug 8, 2023

View reviewed changes

kubevirt-bot assigned codingben Aug 8, 2023

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

akrejcir marked this pull request as draft August 8, 2023 13:51

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023

akrejcir mentioned this pull request Aug 8, 2023

Update vm-console-proxy manifests to v0.3.0 #644

Closed

codingben mentioned this pull request Aug 9, 2023

refactor(CNV-31248): remove namespace annotation #634

Merged

akrejcir force-pushed the fix-proxy-image branch from 803a13f to 5aa8fc0 Compare August 10, 2023 16:10

kubevirt-bot added size/XL and removed lgtm Indicates that a PR is ready to be merged. size/M labels Aug 10, 2023

akrejcir force-pushed the fix-proxy-image branch 2 times, most recently from 77c61ec to 64a42b5 Compare August 11, 2023 13:48

kubevirt-bot requested a review from lyarwood August 11, 2023 13:50

0xFelix reviewed Aug 14, 2023

View reviewed changes

codingben reviewed Aug 15, 2023

View reviewed changes

tests/vm_console_proxy_test.go Outdated Show resolved Hide resolved

codingben reviewed Aug 15, 2023

View reviewed changes

akrejcir force-pushed the fix-proxy-image branch 2 times, most recently from 4dacb93 to f533bb5 Compare August 15, 2023 13:14

akrejcir marked this pull request as ready for review August 15, 2023 13:14

kubevirt-bot assigned lyarwood Aug 15, 2023

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

akrejcir force-pushed the fix-proxy-image branch from ccdef89 to e884e8c Compare August 16, 2023 10:42

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

akrejcir force-pushed the fix-proxy-image branch from e884e8c to e417a81 Compare August 21, 2023 07:55

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2023

akrejcir force-pushed the fix-proxy-image branch from e417a81 to 67f795e Compare August 29, 2023 15:47

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

akrejcir force-pushed the fix-proxy-image branch from 67f795e to c261521 Compare August 30, 2023 08:26

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

kubevirt-bot assigned jcanocan Aug 30, 2023

kubevirt-bot merged commit 2896081 into kubevirt:main Aug 30, 2023
11 checks passed

akrejcir deleted the fix-proxy-image branch August 30, 2023 10:58

akrejcir mentioned this pull request Aug 30, 2023

[release-v0.18] fix: Use the latest image of vm-console-proxy #670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use the latest image of vm-console-proxy #645

fix: Use the latest image of vm-console-proxy #645

akrejcir commented Aug 8, 2023 •

edited

akrejcir commented Aug 8, 2023

codingben left a comment •

edited

akrejcir commented Aug 8, 2023

akrejcir commented Aug 11, 2023

0xFelix left a comment

0xFelix Aug 14, 2023

akrejcir Aug 15, 2023

lyarwood Aug 15, 2023

lyarwood Aug 15, 2023

akrejcir Aug 15, 2023

lyarwood Aug 15, 2023

jcanocan Aug 21, 2023

0xFelix Aug 14, 2023

akrejcir Aug 15, 2023

0xFelix Aug 14, 2023

akrejcir Aug 15, 2023

akrejcir Aug 15, 2023

codingben Aug 15, 2023

akrejcir Aug 15, 2023

lyarwood commented Aug 15, 2023

lyarwood commented Aug 15, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 21, 2023

akrejcir commented Aug 21, 2023

jcanocan commented Aug 22, 2023

akrejcir commented Aug 23, 2023

akrejcir commented Aug 29, 2023

sonarcloud bot commented Aug 30, 2023

lyarwood commented Aug 30, 2023

jcanocan commented Aug 30, 2023

akrejcir commented Aug 30, 2023

kubevirt-bot commented Aug 30, 2023

fix: Use the latest image of vm-console-proxy #645

fix: Use the latest image of vm-console-proxy #645

Conversation

akrejcir commented Aug 8, 2023 • edited

akrejcir commented Aug 8, 2023

codingben left a comment • edited

Choose a reason for hiding this comment

akrejcir commented Aug 8, 2023

akrejcir commented Aug 11, 2023

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood commented Aug 15, 2023

lyarwood commented Aug 15, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 16, 2023

akrejcir commented Aug 21, 2023

akrejcir commented Aug 21, 2023

jcanocan commented Aug 22, 2023

akrejcir commented Aug 23, 2023

akrejcir commented Aug 29, 2023

sonarcloud bot commented Aug 30, 2023

lyarwood commented Aug 30, 2023

jcanocan commented Aug 30, 2023

akrejcir commented Aug 30, 2023

kubevirt-bot commented Aug 30, 2023

akrejcir commented Aug 8, 2023 •

edited

codingben left a comment •

edited