kubemacpool: Remove namespace labels #1566

qinqon · 2023-06-14T08:00:28Z

What this PR does / why we need it:
The kubemacpool project was adding restricted label to test security context. This change remove labels altogether and configure them at CI to test it.

Special notes for your reviewer:
Since the CNAO namespace is shared between components is done there instead of kubemacpool that has it's own namespace.

Release note:

Remove kubemacpool namespace labels.

oshoval

Thanks

oshoval · 2023-06-14T08:16:07Z

data/kubemacpool/kubemacpool.yaml

@@ -1,9 +1,6 @@
 apiVersion: v1
 kind: Namespace
 metadata:
-  labels:
-    control-plane: mac-controller-manager


why control-plane: mac-controller-manager is removed as well ?
it was not part of the latest breaking change

+1, it should stay. I think we already delete it for non-namespaced objects (here).

Btw some of the logic there was in order to maintain backward compatability
maybe the versions we support /deployed on customer site are new enough so we can remove this patch?
(more info here #1145)

also worth to see https://github.com/kubevirt/cluster-network-addons-operator/pull/1145/files#r796372158
not sure if was handled

of course not part of this PR scope

I would not depend on the controller and keep the manifest as we want them to be, so we don't need time for them to be fixed.

The controller is removing them only for backward compatibility, for versions where CNAO label stuff that it didn't need to have and then those versions were upgraded to one that fixed the labeling.

You can see here a discussion that tells that we better remove it from the source itself - KMP, IIRC
https://github.com/kubevirt/cluster-network-addons-operator/pull/1145/files#r796372158

oshoval · 2023-06-14T08:24:53Z

/test pull-cluster-network-addons-operator-unit-test

oshoval · 2023-06-14T08:26:10Z

Worth to enclose the failure of HCO please on PR desc

RamLavi · 2023-06-14T08:34:33Z

automation/check-patch.e2e-kubemacpool-functests.sh

+metadata:
+  name: cluster-network-addons
+  labels:
+    control-plane: mac-controller-manager


I think adding/removing the control-plane label should not be part of this PR,

I will remove it from here, but the adding at bump-kubemacpool is ok

RamLavi · 2023-06-14T08:36:09Z

hack/components/bump-kubemacpool.sh

 - op: replace
  path: /data/RANGE_START
  value: "{{ .RangeStart }}"
 - op: replace
  path: /data/RANGE_END
  value: "{{ .RangeEnd }}"
 EOF
+    cat <<EOF > config/cnao/cnao_remove-labels_patch.yaml


can you specifically remove the pod-security.kubernetes.io/enforce label?

I am going to remove all the labels since controller is removing the other one too.

oshoval · 2023-06-14T09:48:55Z

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1566/pull-e2e-cnao-multus-dynamic-networks-functests/1668906222926958592
we have this here btw (don't think it is related to this PR IIRC, but maybe worth to double check if interesting)

W0614 09:24:17.766599   36399 warnings.go:70] would violate PodSecurity "restricted:latest": allowPrivilegeEscalation != false (container "samplepod" must set securityContext.allowPrivilegeEscalation=false), unrestricted capabilities (container "samplepod" must set securityContext.capabilities.drop=["ALL"]), runAsNonRoot != true (pod or container "samplepod" must set securityContext.runAsNonRoot=true), seccompProfile (pod or container "samplepod" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")

oshoval · 2023-06-14T09:49:15Z

/test pull-e2e-cluster-network-addons-operator-kubemacpool-functests

oshoval · 2023-06-14T11:14:58Z

The connection to the server localhost:8080 was refused - did you specify the right host or port?
seems due to this PR ?
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1566/pull-e2e-cluster-network-addons-operator-kubemacpool-functests/1668918522979291136

or just a flake ? (didnt look deep)
happened twice, first time i see it

RamLavi

/lgtm

I think that removing all the labels in the yaml patch may bite us in the future, but we can remove the control-plane label from the NS on the KMP repo in parallel, then consider refining the yaml patch to remove exactly the labels we want.

oshoval · 2023-06-15T08:16:27Z

/test pull-e2e-cluster-network-addons-operator-kubemacpool-functests

oshoval · 2023-06-15T08:23:14Z

it still fails

qinqon · 2023-06-15T08:29:26Z

/override pull-e2e-cnao-multus-dynamic-networks-functests
/test pull-e2e-cluster-network-addons-operator-kubemacpool-functests

kubevirt-bot · 2023-06-15T08:29:29Z

@qinqon: Overrode contexts on behalf of qinqon: pull-e2e-cnao-multus-dynamic-networks-functests

In response to this:

/override pull-e2e-cnao-multus-dynamic-networks-functests
/test pull-e2e-cluster-network-addons-operator-kubemacpool-functests

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.

qinqon · 2023-06-15T08:33:01Z

hiccup

pull-e2e-cluster-network-addons-operator-kubemacpool-functest

/lgtm

I think that removing all the labels in the yaml patch may bite us in the future, but we can remove the control-plane label from the NS on the KMP repo in parallel, then consider refining the yaml patch to remove exactly the labels we want.

Is very dangerous to label the shared namespace, it affects all the network components, we should remove the label, if we see kubemacpool depends on something, we will fix it.

The kubemacpool project was adding restricted label to test security context. This change remove labels altogether and configure them at CI to test it. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

sonarcloud · 2023-06-15T08:48:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

oshoval · 2023-06-15T08:54:35Z

Sumup here for future possible AI, thanks

#1567

qinqon · 2023-06-15T10:11:32Z

/override pull-e2e-cnao-multus-dynamic-networks-functests
/approve

kubevirt-bot · 2023-06-15T10:11:38Z

@qinqon: Overrode contexts on behalf of qinqon: pull-e2e-cnao-multus-dynamic-networks-functests

In response to this:

/override pull-e2e-cnao-multus-dynamic-networks-functests
/approve

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 · 2023-06-15T10:12:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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~~ [qinqon]

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

RamLavi

/lgtm

oshoval · 2023-06-15T12:40:40Z

automation/components-functests.setup.sh

+apiVersion: v1
+kind: Namespace
+metadata:
+  name: cluster-network-addons


nit: we don't need to parameterize the ns name ?
anyhow if needed can be done on follow PR

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 Jun 14, 2023

kubevirt-bot requested review from phoracek and RamLavi June 14, 2023 08:00

kubevirt-bot added the size/S label Jun 14, 2023

qinqon force-pushed the remove-kubemacpool-namespace-labels branch from 28641b6 to 7a47ce7 Compare June 14, 2023 08:01

oshoval reviewed Jun 14, 2023

View reviewed changes

qinqon requested a review from oshoval June 14, 2023 08:27

RamLavi reviewed Jun 14, 2023

View reviewed changes

qinqon force-pushed the remove-kubemacpool-namespace-labels branch from 7a47ce7 to f62b973 Compare June 14, 2023 09:00

qinqon requested a review from RamLavi June 14, 2023 09:00

RamLavi approved these changes Jun 15, 2023

View reviewed changes

kubevirt-bot assigned RamLavi Jun 15, 2023

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

kubemacpool: Remove namespace labels

a32efaa

The kubemacpool project was adding restricted label to test security context. This change remove labels altogether and configure them at CI to test it. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

qinqon force-pushed the remove-kubemacpool-namespace-labels branch from f62b973 to a32efaa Compare June 15, 2023 08:47

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

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

RamLavi approved these changes Jun 15, 2023

View reviewed changes

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

kubevirt-bot merged commit 26b0318 into kubevirt:main Jun 15, 2023
13 checks passed

oshoval mentioned this pull request Jun 15, 2023

Improve CNAO/KMP CONTROL_PLANE_KEY flow #1567

Open

oshoval reviewed Jun 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubemacpool: Remove namespace labels #1566

kubemacpool: Remove namespace labels #1566

qinqon commented Jun 14, 2023 •

edited

oshoval left a comment

oshoval Jun 14, 2023

RamLavi Jun 14, 2023

oshoval Jun 14, 2023 •

edited

qinqon Jun 14, 2023

oshoval Jun 14, 2023

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023

RamLavi Jun 14, 2023

qinqon Jun 14, 2023

RamLavi Jun 14, 2023

qinqon Jun 14, 2023

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023 •

edited

RamLavi left a comment

oshoval commented Jun 15, 2023

oshoval commented Jun 15, 2023

qinqon commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

qinqon commented Jun 15, 2023

sonarcloud bot commented Jun 15, 2023

oshoval commented Jun 15, 2023

qinqon commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

RamLavi left a comment

oshoval Jun 15, 2023

kubemacpool: Remove namespace labels #1566

kubemacpool: Remove namespace labels #1566

Conversation

qinqon commented Jun 14, 2023 • edited

oshoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023

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

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023

oshoval commented Jun 14, 2023 • edited

RamLavi left a comment

Choose a reason for hiding this comment

oshoval commented Jun 15, 2023

oshoval commented Jun 15, 2023

qinqon commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

qinqon commented Jun 15, 2023

sonarcloud bot commented Jun 15, 2023

oshoval commented Jun 15, 2023

qinqon commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

kubevirt-bot commented Jun 15, 2023

RamLavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinqon commented Jun 14, 2023 •

edited

oshoval Jun 14, 2023 •

edited

oshoval commented Jun 14, 2023 •

edited