Use OpenShift's required-scc annotation on components #1931
Conversation
kubevirt-bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@RamLavi I still need to patch up CNAO itself. Could you point me to the correct place ? |
maiqueb
force-pushed
the
use-required-scc-ipam-components
branch
from
e30a3af to
d514395
Compare
| {{ if .IsOpenshift }} | ||
| annotations: | ||
| openshift.io/required-scc: "restricted-v2" | ||
| kubectl.kubernetes.io/default-container: manager |
There was a problem hiding this comment.
nit: why include this in the .IsOpenshift conditional? I'd have expected:
annotations:
kubectl.kubernetes.io/default-container: manager
{{ if .IsOpenshift }}
openshift.io/required-scc: "restricted-v2"
{{ end }}
There was a problem hiding this comment.
agree,
also on the rest please
There was a problem hiding this comment.
I guess it's possible, I'll try.
data/kubemacpool/kubemacpool.yaml
Outdated
| annotations: | ||
| openshift.io/required-scc: "restricted-v2" |
There was a problem hiding this comment.
aint it should be with
{{ if .IsOpenshift }} or {{ if .EnableSCC }} ?
same for L233
do we need to create this scc ? or it is one that is already created ?
(for example for bridge-marker we create the scc by CNAO upon needs)
There was a problem hiding this comment.
The script that templates this doesn't know any of those variables.
And kubemacpool - for whatever reason - already always has OpenShift related data set:
There was a problem hiding this comment.
if needed we can add to the script, i can show you if you want where
There was a problem hiding this comment.
why is this an issue now, when it wasn't before ?
There was a problem hiding this comment.
- key: openshift.io/run-level
operator: NotIn
values:
- "0"
- "1"
for me this seems to have less effect then creating scc
wdty?
i am totally not into being more restrictive by this PR, but imho this might not be the same case here
There was a problem hiding this comment.
I'm not creating an SCC here, I'm saying that this pod can only use this SCC.
On a platform that doesn't use SCCs - e.g. vanilla kubernetes - this is just one more annotation. There's no handler for it.
There was a problem hiding this comment.
Any OpenShift variant in a version that we should support (we support 3 versions back) will have restricted-v2 SCC already created for us ?
There was a problem hiding this comment.
btw, if KMP annotation can always be in the manifest, and no scc need to be created for this specific name
(iiuc it is a system one), then it should be better to not annotate it via CNAO, but add it directly on KMP repo please, same for the rest where applicable.
Note: this also might answer who need to annotate CNAO itself, if no SCC need to be created, and the annotation can always be in the manifest, CNAO can just have it on its own manifests,
else HCO need to create the SCC / or maybe even annotate if no other option.
There was a problem hiding this comment.
There was a problem hiding this comment.
of course you do, thanks
linux-bridge is part of CNAO
all relevant in the hack/components/bump-linux-bridge.sh and its yaml 002-linux-bridge.yaml iirc
| annotations: | ||
| openshift.io/required-scc: "passt-binding-cni" |
There was a problem hiding this comment.
do we need to create the scc?
same question for linux-bridge below (there might be more)
There was a problem hiding this comment.
if so, the PR isnt final yet iiuc
There was a problem hiding this comment.
meant that the required SCC might need to be added unless they exists already in the manifests
i see for example this one exists already, if all the other already exists / system created we are good
can you please spin it on CRC / kcli to see we are all set please?
kind: SecurityContextConstraints
metadata:
name: passt-binding-cni
There was a problem hiding this comment.
@oshoval it's working:
oc version
Client Version: 4.17.7
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: 4.17.7
Kubernetes Version: v1.30.6
cat <<EOF | oc apply -f -
apiVersion: networkaddonsoperator.network.kubevirt.io/v1
kind: NetworkAddonsConfig
metadata:
name: cluster
spec:
multus: {}
linuxBridge: {}
kubeMacPool: {}
ovs: {}
kubeSecondaryDNS: {}
kubevirtIpamController: {}
imagePullPolicy: Always
EOF
networkaddonsconfig.networkaddonsoperator.network.kubevirt.io/cluster configured
oc get pods -ncluster-network-addons -w
NAME READY STATUS RESTARTS AGE
bridge-marker-44qm4 1/1 Running 0 65s
cluster-network-addons-operator-6c96bbb9d9-7dcvr 2/2 Running 0 10m
kube-cni-linux-bridge-plugin-zh6bf 1/1 Running 0 66s
kubemacpool-cert-manager-65d6b697fb-dqjgv 1/1 Running 0 65s
kubemacpool-mac-controller-manager-5db75d6c4d-hcrvf 2/2 Running 0 65s
kubevirt-ipam-controller-manager-7f588c55df-w9jpk 1/1 Running 0 64s
ovs-cni-amd64-d4tsm 1/1 Running 0 65s
passt-binding-cni-wh29m 1/1 Running 0 64s
secondary-dns-6c89d7864d-rvr94 2/2 Running 0 64s
oc get pods -oyaml -ncluster-network-addons | grep required-scc
openshift.io/required-scc: bridge-marker
openshift.io/required-scc: linux-bridge
openshift.io/required-scc: restricted-v2
openshift.io/required-scc: restricted-v2
openshift.io/required-scc: restricted-v2
openshift.io/required-scc: passt-binding-cni
maiqueb
changed the title
Signed-off-by: Miguel Duarte Barroso <[email protected]>
Signed-off-by: Miguel Duarte Barroso <[email protected]>
maiqueb
force-pushed
the
use-required-scc-ipam-components
branch
from
d514395 to
006a7af
Compare
maiqueb
force-pushed
the
use-required-scc-ipam-components
branch
from
006a7af to
c6019b7
Compare
Signed-off-by: Miguel Duarte Barroso <[email protected]>
maiqueb
force-pushed
the
use-required-scc-ipam-components
branch
from
c6019b7 to
4ee876e
Compare
|
What this PR does / why we need it:
This PR adds the
required-sccannotation whenever the components are deploying in an OpenShift cluster.Starting from 4.19 this is required.
Special notes for your reviewer:
Release note: