admission_test.go(TestAdmit): compare SecurityContexts instead of particular members

[php-coder]

This makes the test close to the upstream implementation.

Extracted from #16934 (comment)

PTAL @liggitt @pweil- @adelton @simo5

[pweil-]

I see that this is the way it was in the previous tests but now I'm wondering why it's hard coded.

[php-coder]

I had a version where SELinux Level was passed to the function, but later I realized that we always passing the same value and I decided to inline it. With the current approach we need to pass only the values that differs between tests.

Do you think that I should step back and continue to passing a level as an argument?

[pweil-]

The value assumption is correct but we should be pulling it from the namespace. Guess that was missed in a refactor somewhere:

func CreateNamespaceForTest() *kapi.Namespace {
	return &kapi.Namespace{
		ObjectMeta: metav1.ObjectMeta{
			Name: "default",
			Annotations: map[string]string{
				allocator.UIDRangeAnnotation:           "1/3",
				allocator.MCSAnnotation:                "s0:c1,c0",
				allocator.SupplementalGroupsAnnotation: "2/3",
			},
		},
	}
}

[php-coder]

I updated the code and started to use a variable for that. PTAL.

[legionus]

@php-coder Can you split the TestAdmit to separate tests ? Very difficult to read the test which spans few pages. For example, you can make a helper and call it for each element in testCases.

[php-coder]

Can you split the TestAdmit to separate tests ?

@legionus Ok, I split it to 2 functions :) Is it better now?

[php-coder]

The old flake: #16878
/test extended_conformance_gce

Another old flake: #13046
/test end_to_end

[php-coder]

PTAL @pweil-

[pweil-]

LGTM pending the comment

[pweil-]

We can't pull this from the namespace? I took a quick look but didn't see that it was available. I guess the whole point is that if the ns method changes all of these tests are broken. Easy to change since it's in one place. If that's how it has to be then I'm ok with it.

[php-coder]

@pweil- I added what you've asked: 268aea6 PTAL. I don't see a way of how to do it better than that.

[pweil-]

you could make the setupClientSet call return the namespace that was used. But yes, that's what I was looking to see so that if it is changed somewhere it won't break anything. Thanks for the update.

[php-coder]

We need a namespace only in a single place, so I think it's better to keep this function as-is.

[php-coder]

/test extended_conformance_gce

@pweil- Could you merge it?

[php-coder]

/test extended_conformance_gce

[openshift-ci-robot]

@php-coder: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_gce	268aea6	link	/test extended_conformance_gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[php-coder]

extended_conformance_gce is failing with errors like Gave up after waiting 5m0s for pod or timed out waiting for the condition by unrelated reasons. This PR modifies only a single file with unit tests, so it's unlikely that it causes failure of the conformance tests. I suspect that they're failing because our nodes under pressure this time.

[php-coder]

@openshift/sig-security As because @pweil- on PTO (and will be there next 2 days), please, merge this PR. This refactoring is required for the #16934 that fixes P1 issue.

[simo5]

/lgtm

[eparis]

/approve

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, php-coder, simo5

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• pkg/security/OWNERS [eparis]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[openshift-merge-robot]

Automatic merge from submit-queue (batch tested with PRs 17369, 17296).