USHIFT-346: MicroShift: skip sig-auth tests #27404

pacevedom · 2022-09-07T07:50:04Z

No description provided.

pacevedom · 2022-09-13T05:57:53Z

/hold
Need to rework.

pacevedom · 2022-09-15T13:47:12Z

/unhold
Ready for review. Depends on #27345
/assign @ingvagabund

test/extended/util/client.go

ingvagabund · 2022-09-19T15:43:16Z

test/extended/security/scc.go

+		// This test should use image.ShellImage but this requires having a local image
+		// registry, which not all deployment types have. Using LDAP test image guarantees
+		// capsh presence.
+		pod, err := exutil.NewPodExecutor(oc, "restrictedcapsh", image.OpenLDAPTestImage())


@stlaz for taking a look

@openshift/openshift-team-auth PTAL

@pacevedom is there a less complex image than LDAP? Something on the same level of complexity as the shell image? Complexity = minimum rpm installation/configuration inside the image. I assume you choosed OpenLDAPTestImage since it's publicly available from quay.io/openshifttest/ldap:1.2. I wonder whether we run these tests in a disconnected environment.

Exactly, took this one mainly for being publicly available. I checked other public images having capsh for size and number of RPMs and got:

quay.io/redhat-developer/test-build-simples2i:1.2. 216 MB, 186 RPMs.

quay.io/redhat-developer/test-build-roots2i:1.2. 216 MB, 186 RPMs.

quay.io/openshifttest/multicast:1.1. 267 MB, 240 RPMs.

quay.io/openshifttest/ldap:1.2. 375 MB, 306 RPMs.

So I will update it with either of the first two.

Sounds good

test/extended/oauth/server_headers.go

test/extended/oauth/http1.go

pacevedom · 2022-09-23T14:40:37Z

/hold
Need to get of microshift checks

pacevedom · 2022-09-26T08:00:29Z

/unhold

pacevedom · 2022-09-26T11:30:43Z

/retest-required

pacevedom · 2022-09-26T14:33:48Z

/retest-required

pacevedom · 2022-09-26T19:13:26Z

/retest-required

ingvagabund · 2022-09-29T10:23:55Z

test/extended/authorization/rbac/groups_default_rules.go

+			operatorsCoreOSGroup,
+			imageGroup,
+		),
+		func() {


Would you please put func() { right after the previous line as (, func() {? To reduce the diff. If that does not help you might move fmt.Sprintf next to g.It(.

ingvagabund · 2022-09-29T10:29:51Z

test/extended/authorization/rbac/groups_default_rules.go

-		kubeInformers := informers.NewSharedInformerFactory(oc.AdminKubeClient(), 20*time.Minute)
-		ruleResolver := exutil.NewRuleResolver(kubeInformers.Rbac().V1()) // signal what informers we want to use early
+	g.It(
+		fmt.Sprintf("should have correct RBAC rules [apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s]",


This is quite complex. allUnauthenticatedRules is the source of so many groups. I'd rather turn allUnauthenticatedRules into a api-group-key map and loop over all apigroups over g.It("should have correct RBAC rules", func() { rather than have all groups in a single test name.

Mind if we change this in the next PR?

Making the change later would put the next work into backlog most likely. Better to do it now.

ingvagabund · 2022-09-29T10:32:58Z

test/extended/oauth/http1.go

 )

-var _ = g.Describe("[sig-auth][Feature:OAuthServer] OAuth server", func() {
+var _ = g.Describe("[sig-auth][Feature:OAuthServer] OAuth server [apigroup:auth.openshift.io]", func() {


Is the requirement coming from oc get --raw /.well-known/oauth-authorization-server?

EDIT: From the previous review comment: "// MicroShift does not have the oauth well known data path in the API Server". So it does.

ingvagabund · 2022-09-29T10:33:47Z

test/extended/security/scc.go

+		// This test should use image.ShellImage but this requires having a local image
+		// registry, which not all deployment types have. Using LDAP test image guarantees
+		// capsh presence.
+		pod, err := exutil.NewPodExecutor(oc, "restrictedcapsh", image.OpenLDAPTestImage())


@openshift/openshift-team-auth PTAL

ingvagabund · 2022-10-13T11:08:59Z

test/extended/authorization/rbac/groups_default_rules.go

 	oc := exutil.NewCLI("default-rbac-policy")

-	g.It("should have correct RBAC rules", func() {
+	g.It(fmt.Sprintf("should have correct RBAC rules [apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s][apigroup:%s]",


This is quite complex. allUnauthenticatedRules is the source of so many groups. I'd rather turn allUnauthenticatedRules into a api-group-key map and loop over all apigroups over g.It("should have correct RBAC rules", func() { rather than have all groups in a single test name.

I don't think it's safe to break the test apart. I am not sure why these groups are listed here though because you don't actually need the API groups, you only need the RBAC group, the rest are just strings.

You are right. Also, I don't see an easy way how to run the test minus missing apigroups since the test is doing set diff of both "what's currently available" and "what's expected to be available".

Obvious approach is to dynamically detect the apigroups based on exutil.DoesApiResourceExist, constructing both allAuthenticatedRules and allUnauthenticatedRules based on the apigroup presence and then running the comparision/coverage. Though, there's more than just api group detection which needs to be taken into account. E.g. https://github.com/openshift/origin/blob/master/test/extended/authorization/rbac/groups_default_rules.go#L91-L94.

https://github.com/openshift/origin/blob/master/test/extended/authorization/rbac/groups_default_rules.go#L97-L100 can be removed. Maybe the legacy groups in https://github.com/openshift/origin/blob/master/test/extended/authorization/rbac/groups_default_rules.go#L59-L66 as well? What about https://github.com/openshift/origin/blob/master/test/extended/authorization/rbac/groups_default_rules.go#L134-L142?

I'm taking a second look at this. It look like @stlaz is right, all [apigroup:*] seems wrong. Currently it complains about (fails due to) test data being too big which a nice mechanism except not right against microshift

Reverted these changes as this wasn't exercising [apigroups] - it was exercising RBAC about those apigroups so the test should run on MicroShift. If it fails then we should take a look what should we fix in MicroShift and how should we refactor test if needed

pmtk · 2022-10-24T15:56:04Z

/retest-required

ingvagabund · 2022-11-05T09:21:03Z

/lgtm

pmtk · 2022-11-05T10:25:44Z

/retest-required

pmtk · 2022-11-05T17:15:53Z

/retest-required

ggiguash · 2022-11-06T05:06:34Z

/retest-required

mfojtik · 2022-11-07T09:17:53Z

/approve

ggiguash · 2022-11-07T11:03:44Z

/retest-required

pmtk · 2022-11-07T11:06:47Z

/retest-required

It's no use, ci/prow/e2e-gcp-ovn-image-ecosystem seems to fail consistently on many PRs on the same tests, see: #27502 (comment)

ingvagabund · 2022-11-07T17:23:53Z

/lgtm

pmtk · 2022-11-07T21:14:11Z

/retest-required

ggiguash · 2022-11-08T06:45:24Z

/retest-required

pmtk · 2022-11-08T14:57:31Z

/test e2e-aws-ovn-fips
/test e2e-gcp-ovn

pmtk · 2022-11-09T08:02:03Z

/retest-required

pacevedom · 2022-11-09T10:38:42Z

/retest-required

pmtk · 2022-11-09T11:25:04Z

/lgtm
:>

openshift-ci · 2022-11-09T11:26:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, mfojtik, pacevedom, pmtk

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [mfojtik]

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

openshift-ci · 2022-11-09T11:26:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, mfojtik, pacevedom, pmtk

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [mfojtik]

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

ingvagabund · 2022-11-09T20:12:41Z

/retest-required

openshift-ci · 2022-11-09T20:17:13Z

@pacevedom: 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-gcp-ovn-image-ecosystem	`b79906d`	link	true	`/test e2e-gcp-ovn-image-ecosystem`
ci/prow/e2e-aws-ovn-single-node-upgrade	`2fdce66`	link	false	`/test e2e-aws-ovn-single-node-upgrade`
ci/prow/e2e-aws-ovn-single-node-serial	`2fdce66`	link	false	`/test e2e-aws-ovn-single-node-serial`
ci/prow/e2e-metal-ipi-sdn	`2fdce66`	link	false	`/test e2e-metal-ipi-sdn`
ci/prow/e2e-openstack-ovn	`2fdce66`	link	false	`/test e2e-openstack-ovn`

Full PR test history. Your PR dashboard.

Details

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 ibihim and knobunc September 7, 2022 07:50

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2022

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

pacevedom force-pushed the ushift-sig-auth branch from 4e4ceb0 to e1e5cb8 Compare September 15, 2022 13:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2022

openshift-ci bot assigned ingvagabund Sep 15, 2022

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

pacevedom force-pushed the ushift-sig-auth branch from e1e5cb8 to e389049 Compare September 16, 2022 11:27

ingvagabund suggested changes Sep 19, 2022

View reviewed changes

pacevedom force-pushed the ushift-sig-auth branch from e389049 to 4c7ef46 Compare September 20, 2022 07:48

ingvagabund reviewed Sep 21, 2022

View reviewed changes

test/extended/oauth/http1.go Outdated Show resolved Hide resolved

pacevedom force-pushed the ushift-sig-auth branch 3 times, most recently from 8b607fe to 184784a Compare September 23, 2022 07:50

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

pacevedom force-pushed the ushift-sig-auth branch from 184784a to 4b02c3d Compare September 26, 2022 07:58

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

ingvagabund reviewed Sep 29, 2022

View reviewed changes

ingvagabund suggested changes Sep 29, 2022

View reviewed changes

pacevedom force-pushed the ushift-sig-auth branch from 4b02c3d to e0f066a Compare October 10, 2022 13:34

ingvagabund reviewed Oct 13, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

pmtk force-pushed the ushift-sig-auth branch from b79906d to 01d98fd Compare November 7, 2022 17:18

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2022

pmtk force-pushed the ushift-sig-auth branch from 01d98fd to 24acde4 Compare November 8, 2022 18:21

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2022

microshift: skip sig-auth tests

2fdce66

pmtk force-pushed the ushift-sig-auth branch from 24acde4 to 2fdce66 Compare November 9, 2022 11:24

openshift-ci bot assigned pmtk Nov 9, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2022

openshift-merge-robot merged commit 8bd7286 into openshift:master Nov 9, 2022

USHIFT-346: MicroShift: skip sig-auth tests #27404

USHIFT-346: MicroShift: skip sig-auth tests #27404

Uh oh!

Conversation

pacevedom commented Sep 7, 2022

Uh oh!

pacevedom commented Sep 13, 2022

Uh oh!

pacevedom commented Sep 15, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pacevedom commented Sep 23, 2022

Uh oh!

pacevedom commented Sep 26, 2022

Uh oh!

pacevedom commented Sep 26, 2022

Uh oh!

pacevedom commented Sep 26, 2022

Uh oh!

pacevedom commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stlaz Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk commented Oct 24, 2022

Uh oh!

ingvagabund commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmtk commented Nov 5, 2022

Uh oh!

pmtk commented Nov 5, 2022

Uh oh!

ggiguash commented Nov 6, 2022

Uh oh!

mfojtik commented Nov 7, 2022

Uh oh!

ggiguash commented Nov 7, 2022

Uh oh!

ingvagabund Sep 29, 2022 •

edited

Loading

stlaz Oct 20, 2022 •

edited

Loading

pmtk Nov 3, 2022 •

edited

Loading

ingvagabund commented Nov 5, 2022 •

edited

Loading

pmtk commented Nov 7, 2022 •

edited

Loading