Skip to content

OCPBUGS-48425: ovn-k, rbac: Enable users read & modify UserDefinedNetwork CRs #2619

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

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

ormergi
Copy link
Contributor

@ormergi ormergi commented Jan 15, 2025

This PR introduce new ClusterRole allowing non clsuter-admin users (i.e.: project-admins) create & modify UserDefinedNetwork CRs, allowing self-servicing OVN-K overlay networks w/o the cluster-admin intervention.
Addressing [1].

[1] https://issues.redhat.com/browse/RFE-5530

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

/cc @phoracek @maiqueb

Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

/lgtm

Did you check this works manually ?

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

maiqueb commented Jan 15, 2025

@ormergi need to adjust the unit tests as well.

@ormergi ormergi changed the title ovn-k, rbac: Enable users read & modify UserDefinedNetwork CRs OCPBUGS-48425: ovn-k, rbac: Enable users read & modify UserDefinedNetwork CRs Jan 15, 2025
@openshift-ci-robot openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jan 15, 2025
@openshift-ci-robot
Copy link
Contributor

@ormergi: This pull request references Jira Issue OCPBUGS-48425, which is invalid:

  • expected the bug to target the "4.19.0" version, but no target version was set

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.

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

In response to this:

This PR introduce new ClusterRole allowing non clsuter-admin users (i.e.: project-admins) create & modify UserDefinedNetwork CRs, allowing self-servicing OVN-K overlay networks w/o the cluster-admin intervention.
Addressing [1].

[1] https://issues.redhat.com/browse/RFE-5530

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.

Enable non cluster-admin users read UserDefinedNetwork CRs.

Signed-off-by: Or Mergi <[email protected]>
@ormergi ormergi force-pushed the ovn-k-cudn-missing-rbac branch from 4514299 to 6b36e6e Compare January 15, 2025 16:48
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025
@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

Rebased

@ormergi ormergi force-pushed the ovn-k-cudn-missing-rbac branch from 6b36e6e to 1f56bdf Compare January 15, 2025 16:50
@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

Changes: Fix UT, consider net-seg FG

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

@maiqueb PTAL 🙏

@ormergi ormergi force-pushed the ovn-k-cudn-missing-rbac branch from 1f56bdf to c389698 Compare January 15, 2025 16:56
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -4052,7 +4052,7 @@ func Test_renderOVNKubernetes(t *testing.T) {
client: cnofake.NewFakeClient(),
featureGates: udnFeatureGate,
},
expectNumObjs: 41,
expectNumObjs: 42,
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to add another test without the feat gate enabled and keep expecting 41 ?...

Just a nit, probably makes no sense to test the feature gate ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two other test right above this one that run with and w/o the network-segmentation FG

name: "default",
args: args{
conf: fakeNetworkConf,
bootstrapResult: fakeBootstrapResultOVN,
manifestDir: manifestDirOvn,
client: cnofake.NewFakeClient(),
featureGates: noFeatureGates,
},
expectNumObjs: 37,
},
{
name: "render routeadvertisements",
args: args{
conf: func() *operv1.NetworkSpec {
config := fakeNetworkConf()
config.DefaultNetwork.OVNKubernetesConfig.RouteAdvertisements = operv1.RouteAdvertisementsEnabled
return config
},
bootstrapResult: fakeBootstrapResultOVN,
manifestDir: manifestDirOvn,
client: cnofake.NewFakeClient(),
featureGates: noFeatureGates,
},
expectNumObjs: 38,
},

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

/jira refresh

@openshift-ci-robot
Copy link
Contributor

@ormergi: This pull request references Jira Issue OCPBUGS-48425, which is invalid:

  • expected the bug to target the "4.19.0" version, but no target version was set

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

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.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

/jira refresh

@openshift-ci-robot
Copy link
Contributor

@ormergi: This pull request references Jira Issue OCPBUGS-48425, which is invalid:

  • expected the bug to target only the "4.19.0" version, but multiple target versions were set

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

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.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2025

/jira refresh

@openshift-ci-robot openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jan 15, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2025
The UserDefinedNetwork (UDN) CRD is targeted for non cluster-admin (e.g:
project-admins) users enabling self-service OVN-K overlay networks
w/o the cluster-amdin intervention [1].

The existing "openshift-ovn-kubernetes-cluster-reader" cluster-role
doesn't affect all non-admin users in the cluster, requiring the admin to
grant cluster-reader permissions.

Add UDN reader cluster-role, utilizing cluster-role aggregation [2] to
add the permissions to the built-in "view" cluster-role.

Add UDN editor clsuter-role, utilizing cluster-role aggregation [2] to
add the permissions to the built-in "edit" cluster-role.

For completeness, add UDN read permissions to the existing cluster-role:
"openshift-ovn-kubernetes-cluster-reader"

[1] https://issues.redhat.com/browse/RFE-5530
[2] https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles

Signed-off-by: Or Mergi <[email protected]>
@ormergi ormergi force-pushed the ovn-k-cudn-missing-rbac branch from 5592aff to 976710d Compare January 20, 2025 01:27
@ormergi
Copy link
Contributor Author

ormergi commented Jan 20, 2025

Changes:

  • Rename edit role
  • Update commit message
  • The cluster-reader addition was not sufficient to enable non-admin users read UDN CRs out-of-the-box.
    Add additional UDN reader role using cluster-role aggregation to the built-in viewer role, affecting non-admin users.
    With the additional role, users are able to read UDN CRs in the namespace they have permissions to out-of-the-box.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 20, 2025

/lgtm

Did you check this works manually ?

Verified against cluster-bot cluster using the following command launch openshift/api#1997,openshift/cluster-network-operator#2619 aws,ovn.
I configured htpasswd OAUTH provider and few users to simulate multiple non-admin users
https://docs.openshift.com/container-platform/4.17/authentication/identity_providers/configuring-htpasswd-identity-provider.html
It works as expected, non-admin user can read & modify UDN CRs, in the namespace it has permissions to, out-of-the-box.
Users cannot view or modify UDN CRs in namespaces they dont have permissions to.

@maiqueb PTAL

@@ -13,6 +13,9 @@ rules:
- egressqoses
- egressservices
- adminpolicybasedexternalroutes
{{- if .OVN_NETWORK_SEGMENTATION_ENABLE }}
- userdefinednetworks
Copy link
Contributor

Choose a reason for hiding this comment

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

What about CUDN?

Copy link
Contributor Author

@ormergi ormergi Jan 20, 2025

Choose a reason for hiding this comment

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

CUDN CRD is targeted for cluster-admin users, I think it should not be part of the ovn cluster-reader role, at least not by default. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Other resources in this group are also cluster wide objects meant to be configured by admins.
I don't mind adding it as a followup.

@kyrtapz
Copy link
Contributor

kyrtapz commented Jan 20, 2025

/lgtm

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

openshift-ci bot commented Jan 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyrtapz, maiqueb, ormergi

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 Jan 20, 2025
@ormergi
Copy link
Contributor Author

ormergi commented Jan 20, 2025

/retest-required

2 similar comments
@ormergi
Copy link
Contributor Author

ormergi commented Jan 20, 2025

/retest-required

@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/retest-required

@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/retest

@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jan 21, 2025
@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/test ci/prow/security

Copy link
Contributor

openshift-ci bot commented Jan 21, 2025

@ormergi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test 4.18-upgrade-from-stable-4.17-images
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-upgrade
/test e2e-aws-ovn-windows
/test e2e-azure-ovn-upgrade
/test e2e-gcp-ovn
/test e2e-gcp-ovn-techpreview
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-ipv6-ipsec
/test e2e-ovn-ipsec-step-registry
/test images
/test lint
/test unit
/test verify

The following commands are available to trigger optional jobs:

/test 4.18-upgrade-from-stable-4.17-e2e-aws-ovn-upgrade
/test 4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade
/test 4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade
/test e2e-aws-hypershift-ovn-kubevirt
/test e2e-aws-ovn-ipsec-serial
/test e2e-aws-ovn-ipsec-upgrade
/test e2e-aws-ovn-local-to-shared-gateway-mode-migration
/test e2e-aws-ovn-serial
/test e2e-aws-ovn-shared-to-local-gateway-mode-migration
/test e2e-aws-ovn-single-node
/test e2e-aws-ovn-techpreview-serial
/test e2e-azure-ovn
/test e2e-azure-ovn-dualstack
/test e2e-azure-ovn-manual-oidc
/test e2e-network-mtu-migration-ovn-ipv4
/test e2e-network-mtu-migration-ovn-ipv6
/test e2e-openstack-ovn
/test e2e-ovn-hybrid-step-registry
/test e2e-ovn-step-registry
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-dualstack
/test e2e-vsphere-ovn-dualstack-primaryv6
/test e2e-vsphere-ovn-windows
/test frrk8s-e2e
/test okd-scos-e2e-aws-ovn
/test okd-scos-images
/test qe-perfscale-aws-ovn-medium-cluster-density
/test qe-perfscale-aws-ovn-medium-node-density-cni
/test qe-perfscale-aws-ovn-small-cluster-density
/test qe-perfscale-aws-ovn-small-node-density-cni
/test security

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-cluster-network-operator-master-4.18-upgrade-from-stable-4.17-e2e-aws-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-4.18-upgrade-from-stable-4.17-images
pull-ci-openshift-cluster-network-operator-master-e2e-aws-hypershift-ovn-kubevirt
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-hypershift-conformance
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-local-to-shared-gateway-mode-migration
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-serial
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-shared-to-local-gateway-mode-migration
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-single-node
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-windows
pull-ci-openshift-cluster-network-operator-master-e2e-azure-ovn
pull-ci-openshift-cluster-network-operator-master-e2e-azure-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-e2e-gcp-ovn
pull-ci-openshift-cluster-network-operator-master-e2e-gcp-ovn-techpreview
pull-ci-openshift-cluster-network-operator-master-e2e-gcp-ovn-upgrade
pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-ipv6
pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-ipv6-ipsec
pull-ci-openshift-cluster-network-operator-master-e2e-network-mtu-migration-ovn-ipv4
pull-ci-openshift-cluster-network-operator-master-e2e-network-mtu-migration-ovn-ipv6
pull-ci-openshift-cluster-network-operator-master-e2e-openstack-ovn
pull-ci-openshift-cluster-network-operator-master-e2e-ovn-hybrid-step-registry
pull-ci-openshift-cluster-network-operator-master-e2e-ovn-ipsec-step-registry
pull-ci-openshift-cluster-network-operator-master-e2e-ovn-step-registry
pull-ci-openshift-cluster-network-operator-master-e2e-vsphere-ovn
pull-ci-openshift-cluster-network-operator-master-e2e-vsphere-ovn-dualstack
pull-ci-openshift-cluster-network-operator-master-e2e-vsphere-ovn-dualstack-primaryv6
pull-ci-openshift-cluster-network-operator-master-images
pull-ci-openshift-cluster-network-operator-master-lint
pull-ci-openshift-cluster-network-operator-master-okd-scos-e2e-aws-ovn
pull-ci-openshift-cluster-network-operator-master-security
pull-ci-openshift-cluster-network-operator-master-unit
pull-ci-openshift-cluster-network-operator-master-verify

In response to this:

/test ci/prow/security

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.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/test security

@openshift-merge-bot openshift-merge-bot bot merged commit 0dfccc1 into openshift:master Jan 21, 2025
27 of 35 checks passed
@openshift-ci-robot
Copy link
Contributor

@ormergi: Jira Issue OCPBUGS-48425: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-48425 has been moved to the MODIFIED state.

In response to this:

This PR introduce new ClusterRole allowing non clsuter-admin users (i.e.: project-admins) create & modify UserDefinedNetwork CRs, allowing self-servicing OVN-K overlay networks w/o the cluster-admin intervention.
Addressing [1].

[1] https://issues.redhat.com/browse/RFE-5530

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.

@openshift-cherrypick-robot

@ormergi: #2619 failed to apply on top of branch "release-4.18":

Applying: ovn-k,rbac: Add UDN to cluster-reader
Applying: ovn-k,rbac: Enable non-admin users create & modify UDN CRs
Using index info to reconstruct a base tree...
M	pkg/network/ovn_kubernetes_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/network/ovn_kubernetes_test.go
CONFLICT (content): Merge conflict in pkg/network/ovn_kubernetes_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 ovn-k,rbac: Enable non-admin users create & modify UDN CRs

In response to this:

/cherry-pick release-4.18

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.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 21, 2025

/cherry-pick release-4.18

@openshift-cherrypick-robot

@ormergi: #2619 failed to apply on top of branch "release-4.18":

Applying: ovn-k,rbac: Add UDN to cluster-reader
Applying: ovn-k,rbac: Enable non-admin users create & modify UDN CRs
Using index info to reconstruct a base tree...
M	pkg/network/ovn_kubernetes_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/network/ovn_kubernetes_test.go
CONFLICT (content): Merge conflict in pkg/network/ovn_kubernetes_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 ovn-k,rbac: Enable non-admin users create & modify UDN CRs

In response to this:

/cherry-pick release-4.18

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.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-network-operator
This PR has been included in build cluster-network-operator-container-v4.19.0-202501211607.p0.g0dfccc1.assembly.stream.el9.
All builds following this will include this PR.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 23, 2025

/cherry-pick release-4.18

@openshift-cherrypick-robot

@ormergi: #2619 failed to apply on top of branch "release-4.18":

Applying: ovn-k,rbac: Add UDN to cluster-reader
Applying: ovn-k,rbac: Enable non-admin users create & modify UDN CRs
Using index info to reconstruct a base tree...
M	pkg/network/ovn_kubernetes_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/network/ovn_kubernetes_test.go
CONFLICT (content): Merge conflict in pkg/network/ovn_kubernetes_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 ovn-k,rbac: Enable non-admin users create & modify UDN CRs

In response to this:

/cherry-pick release-4.18

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. 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.

7 participants