Skip to content
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.

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

create role binding for custom and default ns #443

Merged
merged 7 commits into from
Oct 26, 2023

Conversation

birsanv
Copy link
Collaborator

@birsanv birsanv commented Oct 25, 2023

https://issues.redhat.com/browse/ACM-8348

https://issues.redhat.com/browse/ACM-8263

Issue:

  1. in 2.8.2 For MSA accounts created in a ns different than the default addon, we create a custom ManifestWork to create a RoleBinding for the ServiceAccount in that ns

The issue is that the RoleBinding has the same name as the one created by the initial ManifestWork and since this is a global resource, both ManifestWork owns it. Each ManifestWork keep updating the RoleBinding

oc get clusterrolebinding managedserviceaccount-import -o yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
creationTimestamp: "2023-10-25T15:42:54Z"
name: managedserviceaccount-import
ownerReferences:

apiVersion: work.open-cluster-management.io/v1
kind: AppliedManifestWork
name: 13583c9e64c6bd8e94c94f7aa0e5a3b873c8033fc07c1f2ceb7c6b5da3056750-addon-managed-serviceaccount-import
uid: b5fcf244-e461-404a-88c7-4c4aeba5db57
apiVersion: work.open-cluster-management.io/v1
kind: AppliedManifestWork
name: 13583c9e64c6bd8e94c94f7aa0e5a3b873c8033fc07c1f2ceb7c6b5da3056750-addon-managed-serviceaccount-import-custom
uid: c0476c37-c3b0-4184-bedd-e9ffb10e131c
resourceVersion: "49530"
uid: 597859e7-ce78-43dc-8b65-b038492751bc
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: klusterlet-bootstrap-kubeconfig
subjects:
kind: ServiceAccount
name: auto-import-account
namespace: open-cluster-management-addon-observability
  1. second issue for 2.9 : even when a MSA is to use a custom ns, the framework ignores this value and puts in into the default addon ns. The backup operator code is not aware of this change and it creates the custom ManifestWork, which creates the RoleBinding for the custom ns, instead of the default one

Fixes:

  • Delete the custom Manifest work if exists
  • if the MSA sets a custom ns, create a custom-2 suffixed ManifestWork ; use the same name for the RoleBinding in the workload so it doesn't collide with the one created for the addon ns
  • Even if the MSA is set to a custom ns, to account for the framework fixes in 2.9, we also create a ManifestWork for the default addon ns

Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
Signed-off-by: Valentina Birsan <vbirsan@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

93.9% 93.9% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@tesshuflower tesshuflower left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: birsanv, tesshuflower

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 [birsanv,tesshuflower]

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 merged commit bc75c0c into stolostron:main Oct 26, 2023
8 checks passed
@birsanv birsanv deleted the vb_1025_23 branch October 26, 2023 17:24
@birsanv
Copy link
Collaborator Author

birsanv commented Oct 26, 2023

/cherry-pick release-2.8

@openshift-cherrypick-robot

@birsanv: #443 failed to apply on top of branch "release-2.8":

Applying: create role binding for custom and default ns
Applying: fix code smells
Applying: add tests
Applying: remove -custom MWork create a new one -custom-2
Using index info to reconstruct a base tree...
M	controllers/pre_backup.go
M	controllers/pre_backup_test.go
Falling back to patching base and 3-way merge...
Auto-merging controllers/pre_backup_test.go
Auto-merging controllers/pre_backup.go
CONFLICT (content): Merge conflict in controllers/pre_backup.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 remove -custom MWork create a new one -custom-2
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-2.8

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.

@birsanv
Copy link
Collaborator Author

birsanv commented Oct 26, 2023

/cherry-pick release-2.8

@openshift-cherrypick-robot

@birsanv: new pull request created: #447

In response to this:

/cherry-pick release-2.8

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.

@tesshuflower
Copy link
Collaborator

/cherry-pick release-2.7

@openshift-cherrypick-robot

@tesshuflower: new pull request created: #449

In response to this:

/cherry-pick release-2.7

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants