feat: Add pod annotations specific for openshift environment

[erezo9]

Signed-off-by: Erez Tamam erezo9@gmail.com

What this PR does / why we need it:
Because openshift environment does not need scc, i have mad a key created for openshift (might be usefull for other things in the future) that will allow pod annotations for that vendor
@sozercan as we talked in the weekly

Fixes #1329

[chewong]

nit: use spaces instead of tabs

[erezo9]

@chewong ill change the replacments and run make again to see the change

[erezo9]

@chewong done and pushed

[chewong]

LGTM. I will hand it off it @sozercan for a final review.

[codecov-commenter]

Codecov Report

Merging #2116 (7b9d276) into master (92b3b7d) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2116      +/-   ##
==========================================
- Coverage   54.38%   54.31%   -0.08%     
==========================================
  Files         111      111              
  Lines        9478     9478              
==========================================
- Hits         5155     5148       -7     
- Misses       3933     3938       +5     
- Partials      390      392       +2     

Flag	Coverage Δ
unittests	54.31% <ø> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/readiness/list.go	79.41% <0.00%> (-11.77%)	⬇️
...onstrainttemplate/constrainttemplate_controller.go	54.19% <0.00%> (-0.72%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92b3b7d...7b9d276. Read the comment docs.

[ritazh]

/hold

I tried to reproduce #2102 but could not.

[erezo9]

/hold

I tried to reproduce #2102 but could not.

how did you try to reproduce?
I installed via helm like this
helm install --namespace gatekeeper gatekeeper -f values.yaml gatekeeper/gatekeeper

and my values are:

podAnnotations:
  {myenv: test}

and this is the annotations that I got on the controller manager deployment

      annotations:
        container.seccomp.security.alpha.kubernetes.io/manager: runtime/default
        myenv: test

[ritazh]

PTAL: #2102 (comment)

[chewong]

I was able to repro it. I suspected that helm was trying to merge the pod annotations from the default values.yaml with the custom values.yaml you provided. To fix that, we need to set the default value of our pod annotations to be {} and overwrite it to {container.seccomp.security.alpha.kubernetes.io/manager: runtime/default} if it's empty in order to preserve the same behavior.

diff --git a/cmd/build/helmify/replacements.go b/cmd/build/helmify/replacements.go
index 4610edab..f44ac272 100644
--- a/cmd/build/helmify/replacements.go
+++ b/cmd/build/helmify/replacements.go
@@ -49,7 +49,11 @@ var replacements = map[string]string{
 
        "HELMSUBST_DEPLOYMENT_REPLICAS": `{{ .Values.replicas }}`,
 
-       `HELMSUBST_ANNOTATIONS: ""`: `{{- toYaml .Values.podAnnotations | trim | nindent 8 }}`,
+       `HELMSUBST_ANNOTATIONS: ""`: `{{- if .Values.podAnnotations }}
+        {{- toYaml .Values.podAnnotations | trim | nindent 8 }}
+        {{- else }}
+        container.seccomp.security.alpha.kubernetes.io/manager: runtime/default
+        {{- end }}`,
 
        "HELMSUBST_SECRET_ANNOTATIONS": `{{- toYaml .Values.secretAnnotations | trim | nindent 4 }}`,
 
diff --git a/cmd/build/helmify/static/values.yaml b/cmd/build/helmify/static/values.yaml
index 9f0eca2c..ee7514c0 100644
--- a/cmd/build/helmify/static/values.yaml
+++ b/cmd/build/helmify/static/values.yaml
@@ -80,8 +80,7 @@ image:
   release: v3.9.0-beta.2
   pullPolicy: IfNotPresent
   pullSecrets: []
-podAnnotations:
-  {container.seccomp.security.alpha.kubernetes.io/manager: runtime/default}
+podAnnotations: {}
 podLabels: {}
 podCountLimit: 100
 secretAnnotations: {}
diff --git a/manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment.yaml b/manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment
.yaml
index 06f82ba2..2bbe5ffb 100644
--- a/manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment.yaml
+++ b/manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment.yaml
@@ -25,7 +25,11 @@ spec:
   template:
     metadata:
       annotations:
+        {{- if .Values.podAnnotations }}
         {{- toYaml .Values.podAnnotations | trim | nindent 8 }}
+        {{- else }}
+        container.seccomp.security.alpha.kubernetes.io/manager: runtime/default
+        {{- end }}
       labels:
 {{- include "gatekeeper.podLabels" . }}
         app: '{{ template "gatekeeper.name" . }}'
diff --git a/manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml b/manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml
index e1a47339..1b94c448 100644
--- a/manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml
+++ b/manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml
@@ -25,7 +25,11 @@ spec:
   template:
     metadata:
       annotations:
+        {{- if .Values.podAnnotations }}
         {{- toYaml .Values.podAnnotations | trim | nindent 8 }}
+        {{- else }}
+        container.seccomp.security.alpha.kubernetes.io/manager: runtime/default
+        {{- end }}
       labels:
 {{- include "gatekeeper.podLabels" . }}
         app: '{{ template "gatekeeper.name" . }}'
diff --git a/manifest_staging/charts/gatekeeper/values.yaml b/manifest_staging/charts/gatekeeper/values.yaml
index 9f0eca2c..ee7514c0 100644
--- a/manifest_staging/charts/gatekeeper/values.yaml
+++ b/manifest_staging/charts/gatekeeper/values.yaml
@@ -80,8 +80,7 @@ image:
   release: v3.9.0-beta.2
   pullPolicy: IfNotPresent
   pullSecrets: []
-podAnnotations:
-  {container.seccomp.security.alpha.kubernetes.io/manager: runtime/default}
+podAnnotations: {}
 podLabels: {}
 podCountLimit: 100
 secretAnnotations: {}

I have tested the changes above and it worked. Feel free to do more testing and update the PR.

[erezo9]

@chewong
Yes, that is a the solution but, that annotation is required for k8s, while its not required for openshift given the scc that needs to be implemented
https://open-policy-agent.github.io/gatekeeper/website/docs/vendor-specific/

With this restricted profile, it won't be possible to set the container.seccomp.security.alpha.kubernetes.io/manager: runtime/default annotation. On the other hand, given the limited amount of privileges provided by the anyuid scc, the annotation can be removed.

In this PR im trying to keep the k8s annotation which is a must, and the openshift annotations that are not needed

Also, lets say i have an annotation i want to add, just prometheus.io/scrape: true
with the template provided the seccomp annotation will not be added

[sozercan]

@erezo9 does the seccompProfile field work in openshift? https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-seccomp-profile-for-a-container

If so, we can add seccompProfile and set the annotation to {}

[erezo9]

@erezo9 does the seccompProfile field work in openshift? https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-seccomp-profile-for-a-container

If so, we can add seccompProfile and set the annotation to {}

Well if you are removing the annotation, shouldn’t you be also removing the security context?
I can check on Sunday, but if I were to guess, it will have the same behaviour

[erezo9]

@chewong @sozercan
So I thought of a change and pushed it, I added a check to see if the k8s has capablities of openshift, it wont add the annotation
if it doesnt have the openshift capability, it will have k8s annotation
On helm template you can run
helm template gatekeeper/ --api-versions security.openshift.io/v1
It will show the change as if it is running on openshift cluster

[chewong]

Well if you are removing the annotation, shouldn’t you be also removing the security context?

We are planning to remove this deprecated annotation anyway in favor of the new seccompProfile field in the security context field (see #1329). That's why we were wondering if the seccompProfile field would be safely ignored by openshift so we don't need the extra logic to determine whether we are running in openshift or not.

[erezo9]

Well if you are removing the annotation, shouldn’t you be also removing the security context?

We are planning to remove this deprecated annotation anyway in favor of the new seccompProfile field in the security context field (see #1329). That's why we were wondering if the seccompProfile field would be safely ignored by openshift so we don't need the extra logic to determine whether we are running in openshift or not.

Ill check tommorow, but I think the isOpenshift flag will be relavent for that as well and for other things possible in the future

[erezo9]

@chewong
I have checked with the seccomps and it returns the same behavior as the annotation
if you want, i can remove the annotation, add the seccomp instead and add the check there - that will solve the deprecation of the annotation and the solution of thesecomp

[chewong]

That works. I think we can omit the openshift-specific template that we currently have and use a new value .Values.enableSeccompProfile to enable or disable the feature. WDYT?

[erezo9]

That works. I think we can omit the openshift-specific template that we currently have and use a new value .Values.enableSeccompProfile to enable or disable the feature. WDYT?

that could work, so ill change the code and add to this change?

[chewong]

Sounds good.

[erezo9]

Sounds good.

can you send me a message on slack?

[ritazh]

/hold cancel
Looks much better!
Thanks for the PR @erezo9!