Skip to content

Conversation

clarku
Copy link

@clarku clarku commented Aug 1, 2025

Previously the default-account-cluster-network-operator ClusterRoleBinding wouldn't be deleted because it had invalid annotations of

include.release.openshift.io/(profile): "false"

in #2084 .

This manifest was removed in #2725, but this PR reverts that change by adding this ClusterRoleBinding back to the Cluster Version Operator (CVO) manifests with fixed annotations so that it will be deleted properly. The plan is to reapply the changes in the other PR once this change has been in a full version of OpenShift.

@clarku clarku changed the title Configure CVO to delete default-account role binding OCPBUGS-60032: ClusterRoleBinding for default ServiceAccount still exists Aug 1, 2025
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 1, 2025
@openshift-ci-robot
Copy link
Contributor

@clarku: This pull request references Jira Issue OCPBUGS-60032, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.20.0) matches configured target version for branch (4.20.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Previously the default-account-cluster-network-operator ClusterRoleBinding wouldn't be deleted because it had invalid annotations of

include.release.openshift.io/(profile): "false"

in #2084 .

This manifest was removed in #2725, but this PR reverts that change by adding this ClusterRoleBinding back to the CVO manifests with fixed annotations so that it will be deleted properly. The plan is to reapply the changes in that other PR once this change has been in a full version of OpenShift.

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 openshift-eng/jira-lifecycle-plugin repository.

@clarku clarku changed the title OCPBUGS-60032: ClusterRoleBinding for default ServiceAccount still exists OCPBUGS-60032: Default-account ClusterRoleBinding still exists in cluster Aug 1, 2025
@clarku clarku changed the title OCPBUGS-60032: Default-account ClusterRoleBinding still exists in cluster OCPBUGS-60032: Configure CVO to delete default-account role binding Aug 1, 2025
@openshift-ci-robot
Copy link
Contributor

@clarku: This pull request references Jira Issue OCPBUGS-60032, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.20.0) matches configured target version for branch (4.20.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Previously the default-account-cluster-network-operator ClusterRoleBinding wouldn't be deleted because it had invalid annotations of

include.release.openshift.io/(profile): "false"

in #2084 .

This manifest was removed in #2725, but this PR reverts that change by adding this ClusterRoleBinding back to the Cluster Version Operator (CVO) manifests with fixed annotations so that it will be deleted properly. The plan is to reapply the changes in that other PR once this change has been in a full version of OpenShift.

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 openshift-eng/jira-lifecycle-plugin repository.

@clarku clarku force-pushed the OCPBUGS-60032 branch 2 times, most recently from 0172a32 to 6330205 Compare August 1, 2025 02:27
@clarku
Copy link
Author

clarku commented Aug 1, 2025

/retest

6 similar comments
@clarku
Copy link
Author

clarku commented Aug 1, 2025

/retest

@clarku
Copy link
Author

clarku commented Aug 1, 2025

/retest

@clarku
Copy link
Author

clarku commented Aug 2, 2025

/retest

@clarku
Copy link
Author

clarku commented Aug 3, 2025

/retest

@clarku
Copy link
Author

clarku commented Aug 6, 2025

/retest

@clarku
Copy link
Author

clarku commented Aug 12, 2025

/retest

@ricky-rav
Copy link
Contributor

/assign @jcaamano

@jcaamano
Copy link
Contributor

@clarku do you have access to any reference documentation that states include.release.openshift.io/(profile): "false" is an invalid annotation and how that in any case prevents the binding to be deleted?

@jcaamano
Copy link
Contributor

@lance5890 did you have any chance to crosscheck that the binding was indeed removed before pushing #2725?

Is the problem that we are hitting here platform specific?

@clarku
Copy link
Author

clarku commented Aug 28, 2025

@jcaamano,

Thanks for taking a look at this PR. After your comments, I took another look at this.

Firstly, for reference, here is the enhancement for cluster profiles.

Regarding your specific question, I think I was slightly incorrect earlier when I referred to include.release.openshift.io/(profile): "false" as an "invalid" annotation. It is probably more accurate to say that it is redundant as leaving the annotation out would have the same effect as using "false". This can be seen in the code references below.

I believe the real issue is that when the release.openshift.io/delete: "true" annotation is used to delete a resource, the include.release.openshift.io/(profile): "true" annotation would also have to be set for the relevant cluster profiles. If this isn't done, then the manifest would be excluded from the reconcilation process for those profiles, and the deletion wouldn't go through. From testing, I can see the message of this exclusion in the Cluster Version Operator (CVO) logs with the default cluster profile (self-managed-high-availability) and the current manifest.

payload.go:210] excluding Filename: "0000_70_cluster-network-operator_02_rbac.yaml" Group: "rbac.authorization.k8s.io" Kind: "ClusterRoleBinding" Name: "default-account-cluster-network-operator": unrecognized value include.release.openshift.io/self-managed-high-availability=false

This error is from here and called by this.

@lance5890
Copy link
Contributor

@jcaamano,

Thanks for taking a look at this PR. After your comments, I took another look at this.

Firstly, for reference, here is the enhancement for cluster profiles.

Regarding your specific question, I think I was slightly incorrect earlier when I referred to include.release.openshift.io/(profile): "false" as an "invalid" annotation. It is probably more accurate to say that it is redundant as leaving the annotation out would have the same effect as using "false". This can be seen in the code references below.

I believe the real issue is that when the release.openshift.io/delete: "true" annotation is used to delete a resource, the include.release.openshift.io/(profile): "true" annotation would also have to be set for the relevant cluster profiles. If this isn't done, then the manifest would be excluded from the reconcilation process for those profiles, and the deletion wouldn't go through. From testing, I can see the message of this exclusion in the Cluster Version Operator (CVO) logs with the default cluster profile (self-managed-high-availability) and the current manifest.

payload.go:210] excluding Filename: "0000_70_cluster-network-operator_02_rbac.yaml" Group: "rbac.authorization.k8s.io" Kind: "ClusterRoleBinding" Name: "default-account-cluster-network-operator": unrecognized value include.release.openshift.io/self-managed-high-availability=false

This error is from here and called by this.

yes, I think @clarku find the issue that #2084 has not set the true annotation value, this will not add the yaml into cvo Appropriately, and i forgot to double check the file too

roleRef:
kind: ClusterRole
name: cluster-admin
apiGroup: rbac.authorization.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a new line?

Copy link
Author

Choose a reason for hiding this comment

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

Done

Adds default-account-cluster-network-operator back to the Cluster
Version Operator (CVO) manifests with fixed annotations so that
it will be deleted properly.

Signed-off-by: Clark Uthayakumar <[email protected]>
@jcaamano
Copy link
Contributor

/lgtm
/approve

Should this be considered a critical fix? If so, why?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2025
Copy link
Contributor

openshift-ci bot commented Aug 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clarku, jcaamano

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 Aug 28, 2025
@clarku
Copy link
Author

clarku commented Aug 29, 2025

Should this be considered a critical fix? If so, why?

I don't think so and here is my reasoning.

  • There is a easy workaround by deleting the role binding.
  • This has no direct impact on the cluster.
  • Although this bug means that there can be a unused role binding that binds to cluster admin, I don't believe that this represents a major security risk by itself.

@openshift-bot
Copy link
Contributor

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

@openshift-ci-robot openshift-ci-robot added jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. and removed jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Sep 2, 2025
@openshift-ci-robot
Copy link
Contributor

@openshift-bot: This pull request references Jira Issue OCPBUGS-60032, which is invalid:

  • expected the bug to target either version "4.21." or "openshift-4.21.", but it targets "4.20" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

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 openshift-eng/jira-lifecycle-plugin repository.

@clarku
Copy link
Author

clarku commented Sep 2, 2025

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 2, 2025
@openshift-ci-robot
Copy link
Contributor

@clarku: This pull request references Jira Issue OCPBUGS-60032, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.21.0) matches configured target version for branch (4.21.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

@clarku
Copy link
Author

clarku commented Sep 2, 2025

/retest

@jcaamano
Copy link
Contributor

jcaamano commented Sep 8, 2025

/retest
/tide refresh

@clarku
Copy link
Author

clarku commented Sep 12, 2025

/retest

Copy link
Contributor

openshift-ci bot commented Sep 12, 2025

@clarku: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6 6330205 link false /test e2e-vsphere-ovn-dualstack-primaryv6
ci/prow/e2e-aws-ovn-upgrade 269365e link true /test e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn-serial 269365e link false /test e2e-aws-ovn-serial
ci/prow/e2e-aws-hypershift-ovn-kubevirt 269365e link false /test e2e-aws-hypershift-ovn-kubevirt
ci/prow/4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade 269365e link false /test 4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade
ci/prow/4.20-upgrade-from-stable-4.19-e2e-gcp-ovn-upgrade 269365e link false /test 4.20-upgrade-from-stable-4.19-e2e-gcp-ovn-upgrade
ci/prow/4.20-upgrade-from-stable-4.19-e2e-azure-ovn-upgrade 269365e link false /test 4.20-upgrade-from-stable-4.19-e2e-azure-ovn-upgrade
ci/prow/security 269365e link false /test security

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants