Skip to content

CFE-351 #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Mar 21, 2022
Merged

CFE-351 #15

merged 7 commits into from
Mar 21, 2022

Conversation

sherine-k
Copy link
Contributor

Main goal:

  • All resources created with a nodeobservability CR should be cleaned up upon delete of the CR
    Also contains:
  • Reduce open rbac permissions of the operator to the namespace scope
  • Remove secret from reconcile loop : a secret containing the service account token is always automatically created and mounted on the pod using it

@sherine-k
Remove secret from reconcile loop : a secret containing the service a…
f07d853 
…ccount token is always automatically created and mounted on the pod using it

Set OwnerReferences on SCC, SA, ClusterRole, ClusterRoleBinding and DaemonSet
@sherine-k
Fix typo in namespace for main.go
2e4e6a9
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sherine-k

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2022
@sherine-k
Reduce open rbac permissions of the operator to the namespace scope
1155955 
Fix lintint issue due to removing secret reconciliation
@sherine-k
Introduce a Finalizer in order to remove ClusterRole, ClusterRoleBind…
27867fa 
…ing and SCC before deleting NodeObservability
lmzuccarelli
lmzuccarelli reviewed Mar 21, 2022
View reviewed changes
pkg/operator/controller/nodeobservability/controller.go Outdated
if err := r.deleteSecurityContextConstraints(nodeObs); err != nil {
errs = append(errs, fmt.Errorf("failed to delete SCC : %w", err))
}
if len(errs) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: merge if statement i.e if len(errs) == 0 && hasFinalizer(nodeObs) {

lmzuccarelli
lmzuccarelli reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@lmzuccarelli lmzuccarelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried deleting the nodeobeservability sample I created and got this error

Reconciler error {"reconciler group": "nodeobservability.olm.openshift.io", "reconciler kind": "NodeObservability", "name": "nodeobservability-sample", "namespace": "node-observability-operator", "error": "failed to update NodeObservability with finalizers:, failed to update finalizers: Operation cannot be fulfilled on nodeobservabilities.nodeobservability.olm.openshift.io \"nodeobservability-sample\": StorageError: invalid object, Code: 4, Key: /kubernetes.io/nodeobservability.olm.openshift.io/nodeobservabilities/node-observability-operator/nodeobservability-sample, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 0563674f-07cc-446f-9eff-b59d2b9c8616, UID in object meta: "}

Need to look at why the finalizers we not able to update

@sherine-k
Copy link
Contributor Author

/unhold

sherine-k
sherine-k commented Mar 21, 2022
View reviewed changes
lmzuccarelli
lmzuccarelli reviewed Mar 21, 2022
View reviewed changes
@sherine-k sherine-k changed the title WIP: CFE-351 CFE-351 Mar 21, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2022
@sherine-k
Copy link
Contributor Author

/unhold

@sherine-k
Copy link
Contributor Author

sherine-k commented Mar 21, 2022 •
edited by lmzuccarelli
Loading

I tried deleting the nodeobeservability sample I created and got this error

Reconciler error {"reconciler group": "nodeobservability.olm.openshift.io", "reconciler kind": "NodeObservability", "name": "nodeobservability-sample", "namespace": "node-observability-operator", "error": "failed to update NodeObservability with finalizers:, failed to update finalizers: Operation cannot be fulfilled on nodeobservabilities.nodeobservability.olm.openshift.io \"nodeobservability-sample\": StorageError: invalid object, Code: 4, Key: /kubernetes.io/nodeobservability.olm.openshift.io/nodeobservabilities/node-observability-operator/nodeobservability-sample, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 0563674f-07cc-446f-9eff-b59d2b9c8616, UID in object meta: "}

Need to look at why the finalizers we not able to update

I didn't reproduce this one

Yes ignore this, as mentioned I think it was an old remnant in my CRC env. I also tried a couple of times and could not reproduce this error

@openshift-ci
Copy link

openshift-ci bot commented Mar 21, 2022

@sherine-k: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@lmzuccarelli
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit af12a55 into openshift:main Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alebedev87 alebedev87 Awaiting requested review from alebedev87

1 more reviewer

@lmzuccarelli lmzuccarelli lmzuccarelli left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@lmzuccarelli lmzuccarelli

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sherine-k @lmzuccarelli @openshift-merge-robot