make supplemental groups test working again #27664

atiratree · 2023-01-17T12:38:33Z

remove user group requirement for the test

Remove [Local] from test as it is not local anymore since #16408 . So this test has not been run in the CI for a long time. And locally as well, since it has been broken because of the removal of exec -p feature in kubernetes/kubernetes#76713.

atiratree · 2023-01-17T12:39:49Z

/assign @ingvagabund

atiratree · 2023-01-17T12:41:31Z

test/extended/security/supplemental_groups.go


 			fsGroup := int64(1111)
 			supGroup := int64(2222)

+			projectName := oc.Namespace()
+			sa := createServiceAccount(ctx, oc, projectName)


reusing this func from scc.go, should I moveit to a separate file like common.go?

I'm not sure whether it was me who originally reviewed the PR of that SA creation, but this wait piece of the code is weird:

origin/test/extended/security/scc.go

Lines 479 to 491 in 79baa1b

err = wait.Poll(100*time.Millisecond, 3*time.Minute, func() (bool, error) {

_, err := oc.AdminKubeClient().CoreV1().ServiceAccounts(namespace).Get(context.Background(), sa.Name, metav1.GetOptions{})

if err != nil {

// If we can't access the service accounts, let's wait till the controller

// create it.

if kapierrs.IsNotFound(err) || kapierrs.IsForbidden(err) {

e2e.Logf("Waiting for service account %q to be available: %v (will retry) ...", sa.Name, err)

return false, nil

}

return false, fmt.Errorf("Failed to get service account %q: %v", sa.Name, err)

}

return true, nil

})

If you're to re-use it, replace it with

origin/test/extended/util/framework.go

Lines 1023 to 1024 in 79baa1b

func WaitForServiceAccount(c corev1client.ServiceAccountInterface, name string) error {

waitFn := func() (bool, error) {

No need to export it or move it anywhere yet, I think.

+1, using the WaitForServiceAccount now

ingvagabund · 2023-01-17T15:25:21Z

/assign @stlaz
/unassign

stlaz · 2023-01-19T13:21:12Z

test/extended/security/supplemental_groups.go


 			fsGroup := int64(1111)
 			supGroup := int64(2222)

+			projectName := oc.Namespace()
+			sa := createServiceAccount(ctx, oc, projectName)


I'm not sure whether it was me who originally reviewed the PR of that SA creation, but this wait piece of the code is weird:

origin/test/extended/security/scc.go

Lines 479 to 491 in 79baa1b

err = wait.Poll(100*time.Millisecond, 3*time.Minute, func() (bool, error) {

_, err := oc.AdminKubeClient().CoreV1().ServiceAccounts(namespace).Get(context.Background(), sa.Name, metav1.GetOptions{})

if err != nil {

// If we can't access the service accounts, let's wait till the controller

// create it.

if kapierrs.IsNotFound(err) || kapierrs.IsForbidden(err) {

e2e.Logf("Waiting for service account %q to be available: %v (will retry) ...", sa.Name, err)

return false, nil

}

return false, fmt.Errorf("Failed to get service account %q: %v", sa.Name, err)

}

return true, nil

})

If you're to re-use it, replace it with

origin/test/extended/util/framework.go

Lines 1023 to 1024 in 79baa1b

func WaitForServiceAccount(c corev1client.ServiceAccountInterface, name string) error {

waitFn := func() (bool, error) {

No need to export it or move it anywhere yet, I think.

stlaz · 2023-01-19T13:29:31Z

test/extended/security/supplemental_groups.go

 			err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-				_, err := oc.AsAdmin().Run("adm").Args("policy", "add-scc-to-user", "anyuid", oc.Username()).Output()
+				// sa serves as a subject instead of the user
+				_, err := oc.AsAdmin().Run("adm").Args("policy", "add-scc-to-user", "anyuid", supSubject).Output()


IIRC -z <SA name> -n <namespace> is what should be used here, right?

both of these options should work, but yes this makes the intent more clearer

stlaz · 2023-01-19T13:57:04Z

test/extended/security/supplemental_groups.go

 			o.Expect(err).NotTo(o.HaveOccurred())

-			out, stderr, err := oc.Run("exec").Args("-p", supplementalGroupsPod, "--", "/usr/bin/id", "-G").Outputs()
+			out, stderr, err := oc.Run("exec").Args(supplementalGroupsPod, "--as", supSubject, "--", "/usr/bin/id", "-G").Outputs()


This should likely work even without that impersonation

it does not

"Error from server (Forbidden): pods \"supplemental-groups\" is forbidden: exec operation is not allowed because the pod's security context exceeds your permissions: pods \"supplemental-groups\" is forbidden: unable to validate against any security context constraint: [provider \"anyuid\": Forbidden: not usable by user or serviceaccount, provider restricted-v2: .spec.securityContext.fsGroup: Invalid value: []int64{1111}: 1111 is not an allowed group, provider \"restricted\": Forbidden: not usable by user or serviceaccount, provider \"nonroot-v2\": Forbidden: not usable by user or serviceaccount, provider \"nonroot\": Forbidden: not usable by user or serviceaccount, provider \"hostmount-anyuid\": Forbidden: not usable by user or serviceaccount, provider \"machine-api-termination-handler\": Forbidden: not usable by user or serviceaccount, provider \"hostnetwork-v2\": Forbidden: not usable by user or serviceaccount, provider \"hostnetwork\": Forbidden: not usable by user or serviceaccount, provider \"hostaccess\": Forbidden: not usable by user or serviceaccount, provider \"node-exporter\": Forbidden: not usable by user or serviceaccount, provider \"privileged\": Forbidden: not usable by user or serviceaccount]"

we have access to the service account which has elevated privileges.

Other option is to run this as an admin.

I thought admin was the default user. Yep, run it like so 👍

- remove user group requirement for the test - remove [Local] from test as it is not local anymore since 356a379

atiratree · 2023-01-20T19:48:10Z

/retest

stlaz · 2023-02-02T12:38:48Z

/lgtm

soltysh

/approve

openshift-ci · 2023-02-02T12:48:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh, stlaz

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:

~~test/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-02-02T15:06:33Z

/retest-required

Remaining retests: 0 against base HEAD 79d9b29 and 2 for PR HEAD 2a2c7c6 in total

openshift-ci-robot · 2023-02-02T17:38:16Z

/retest-required

Remaining retests: 0 against base HEAD 362a0e2 and 1 for PR HEAD 2a2c7c6 in total

openshift-ci-robot · 2023-02-02T22:38:23Z

/retest-required

Remaining retests: 0 against base HEAD 73d3250 and 0 for PR HEAD 2a2c7c6 in total

openshift-ci-robot · 2023-02-03T00:38:09Z

/hold

Revision 2a2c7c6 was retested 3 times: holding

atiratree · 2023-02-07T10:24:41Z

/hold cancel
/retest

openshift-ci-robot · 2023-02-07T15:43:13Z

/retest-required

Remaining retests: 0 against base HEAD a8e9bfe and 2 for PR HEAD 2a2c7c6 in total

openshift-ci · 2023-02-07T18:06:34Z

@atiratree: 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-azure-ovn-etcd-scaling	`2a2c7c6`	link	false	`/test e2e-azure-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-etcd-scaling	`2a2c7c6`	link	false	`/test e2e-aws-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-upgrade	`2a2c7c6`	link	false	`/test e2e-aws-ovn-upgrade`
ci/prow/e2e-aws-ovn-single-node-upgrade	`2a2c7c6`	link	false	`/test e2e-aws-ovn-single-node-upgrade`

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.

openshift-ci bot requested review from mfojtik and soltysh January 17, 2023 12:39

openshift-ci bot assigned ingvagabund Jan 17, 2023

atiratree commented Jan 17, 2023

View reviewed changes

openshift-ci bot assigned stlaz and unassigned ingvagabund Jan 17, 2023

atiratree force-pushed the supplemental-groups branch from e0d2ca0 to 08769b5 Compare January 17, 2023 17:14

stlaz suggested changes Jan 19, 2023

View reviewed changes

stlaz reviewed Jan 19, 2023

View reviewed changes

atiratree added 2 commits January 19, 2023 14:58

make supplemental groups test working again

785c6a7

- remove user group requirement for the test - remove [Local] from test as it is not local anymore since 356a379

use WaitForServiceAccount

2a2c7c6

atiratree force-pushed the supplemental-groups branch from 08769b5 to 2a2c7c6 Compare January 19, 2023 14:00

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2023

soltysh approved these changes Feb 2, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2023

openshift-merge-robot merged commit 60274b3 into openshift:master Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make supplemental groups test working again #27664

make supplemental groups test working again #27664

atiratree commented Jan 17, 2023

atiratree commented Jan 17, 2023

atiratree Jan 17, 2023

stlaz Jan 19, 2023

atiratree Jan 19, 2023

ingvagabund commented Jan 17, 2023

stlaz Jan 19, 2023

stlaz Jan 19, 2023

atiratree Jan 19, 2023

stlaz Jan 19, 2023

atiratree Jan 19, 2023

stlaz Jan 20, 2023

atiratree commented Jan 20, 2023

stlaz commented Feb 2, 2023

soltysh left a comment

openshift-ci bot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 3, 2023

atiratree commented Feb 7, 2023

openshift-ci-robot commented Feb 7, 2023

openshift-ci bot commented Feb 7, 2023

	err = wait.Poll(100time.Millisecond, 3time.Minute, func() (bool, error) {
	_, err := oc.AdminKubeClient().CoreV1().ServiceAccounts(namespace).Get(context.Background(), sa.Name, metav1.GetOptions{})
	if err != nil {
	// If we can't access the service accounts, let's wait till the controller
	// create it.
	if kapierrs.IsNotFound(err) \|\| kapierrs.IsForbidden(err) {
	e2e.Logf("Waiting for service account %q to be available: %v (will retry) ...", sa.Name, err)
	return false, nil
	}
	return false, fmt.Errorf("Failed to get service account %q: %v", sa.Name, err)
	}
	return true, nil
	})

	func WaitForServiceAccount(c corev1client.ServiceAccountInterface, name string) error {
	waitFn := func() (bool, error) {

make supplemental groups test working again #27664

make supplemental groups test working again #27664

Conversation

atiratree commented Jan 17, 2023

atiratree commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Jan 20, 2023

stlaz commented Feb 2, 2023

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci-robot commented Feb 3, 2023

atiratree commented Feb 7, 2023

openshift-ci-robot commented Feb 7, 2023

openshift-ci bot commented Feb 7, 2023