cleanup PR: reflect SELinux policy in RTE DS and report Progressing condition message #1058
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shajmakh 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
good idea to expose the selinux policy in use but
- logging unconditionally will spam the logs
- logs are good only for developers, and we have a way to infer it anyway checking the daemonset, there's 1:1 mapping between security context constraint and selinux policy in use.
What about adding an annotation to the RTE daemonset, which the NROP owns anyway?
shajmakh
force-pushed
the
log-selinux-annotation-use
branch
from
5f678b6
to
03f9660
Compare
shajmakh
changed the title
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| @@ -129,6 +129,13 @@ func DaemonSetHashAnnotation(ds *appsv1.DaemonSet, cmHash string) { | |||
| template.Annotations[hash.ConfigMapAnnotation] = cmHash | |||
| } | |||
|
|
|||
| func DaemonSetAnnotation(ds *appsv1.DaemonSet, annotKey string, annotValue string) { | |||
There was a problem hiding this comment.
this can be an overkill, but let's roll with it for now
There was a problem hiding this comment.
we can add it under the spec annotations; or inject a controller event (shown in the operator CR events) which policy is being used. If it doesn't make a difference why do you think this is an overkill?
There was a problem hiding this comment.
adding a new function in pkg/objectupdate/rte seems overkilly to me, but not enough to change course just yet
| @@ -206,80 +210,81 @@ func (r *NUMAResourcesOperatorReconciler) reconcileResourceAPI(ctx context.Conte | |||
| return false, ctrl.Result{}, "", nil | |||
| } | |||
|
|
|||
| func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, string, error) { | |||
| func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, string, string, error) { | |||
There was a problem hiding this comment.
can we abuse the error instead of adding a new return value?
There was a problem hiding this comment.
we can, progressing condition never produces a real error anyway, but I don't think it is right to abuse it because it'll create a confusion on the reconciliation result.
my take is that we can define a struct to group the common values returned by the substeps used in the reconciliation procedure. OR if we want to keep it as similar to the current state we can create another type to store additional info (includes and error and a string for msg) and replace the returned error
There was a problem hiding this comment.
Returning a new struct seems the best approach, but this needs a larger refactoring. I'll attempt something in my #1032
|
note: the RTE annotation addition/removal is still buggy |
shajmakh
force-pushed
the
log-selinux-annotation-use
branch
from
03f9660
to
abcb65b
Compare
shajmakh
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
shajmakh
force-pushed
the
log-selinux-annotation-use
branch
from
abcb65b
to
0245017
Compare
When using legacy SELinux context, set that annotation to the RTE DS annotations to make it easy to identify which policy is being considered. Signed-off-by: Shereen Haj <[email protected]>
So far when controller update is in progress it's either because RTEs are not yet ready or MCPs are not yet updated. One can learn that from examining the controller logs. This PR gives another option to reflect this info by reporting the extra info under the condition message both in the controller logs and in the operator `Status`. Signed-off-by: Shereen Haj <[email protected]>
Even if the MCP was not updated the controller will still be reporting it under the operator status. Due to previous adjustments, we are now seeing all MCPs under the status. This is wrong however and should change in next commit. Signed-off-by: Shereen Haj <[email protected]>
So far the operator status was having the mcps set even if they are not updated. Fix this to reflect the MCPs only if they are updated while wait for the rest to get reconciled. Signed-off-by: Shereen Haj <[email protected]>
Add controller tests to verify that when the annotation is set in the NROP CR it is also reflected under the RTE annotations and when it's deleted it also vanishes from the DS annotations. Signed-off-by: Shereen Haj <[email protected]>
Labels and annotations and rarely set but the problem with the old code was that new sets are added on top of what exists in the current sets either update the value or add a new key:value pair. However, the case where the key:value pair is not found in the new set doesn't make a difference in the new map state and if the key is found in the current map it will still exist in the updated one, while the required is to omit it. Apart from the API validations, the controller doesn't validate the labels set nor the annotations. Signed-off-by: Shereen Haj <[email protected]>
shajmakh
force-pushed
the
log-selinux-annotation-use
branch
from
0245017
to
3dc13bc
Compare
shajmakh
changed the title
openshift-ci
bot
removed
do-not-merge/work-in-progress
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Cleanup PR to enhance trouble shooting. Please see the commits for additional info.