WINC-879: Introduce Pod Security admission labels by jrvaldes · Pull Request #1230 · openshift/windows-machine-config-operator

jrvaldes · 2022-09-01T13:43:06Z

This change adds the Pod Security admission enforcement labels to
the test namespace created in the e2e tests and to the WMCO namespace
created at deployment, allowing the automatic synchronization of privileged
pods directly without any intermediate pod controller, so that the serviceaccount
has enough privileges to run it. The WMCO namespace is a system namespace ("openshift-" prefixed)
and the security.openshift.io/scc.podSecurityLabelSync label is set as
true to ensure the automatic Pod Security label synchronization.

openshift-ci · 2022-09-01T13:43:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jrvaldes · 2022-09-01T13:43:23Z

/test vsphere-e2e-operator

jrvaldes · 2022-09-01T13:46:52Z

/test azure-e2e-operator

sebsoto · 2022-09-01T14:15:48Z

+  # required labels for WMCO namespace
+  declare -a NS_LABELS=(


Why is this being done here and not in the namespace we are creating when installing the operator:
https://github.com/openshift/windows-machine-config-operator/blob/master/config/manager/manager.yaml#L1

How is this going to work for actual users and not just CI?

I dont understand why in general we create the namespace before attempting to install the operator. The only thing i can see is that we want to create the cloud-private-key secret ahead of time for some reason. I dont see that as necesary

@sebsoto you are correct. As per standup, this is out of the scope in this PR and should be tackled in a follow-up story.

I dont understand why in general we create the namespace before attempting to install the operator. The only thing i can see is that we want to create the cloud-private-key secret ahead of time for some reason. I dont see that as necesary

After attempting to install the operator without creating the namespace, got:

FATA[0000] Failed to run packagemanifests: create catalog: error creating catalog source: namespaces "openshift-windows-machine-config-operator" not found Error from server (NotFound): namespaces "openshift-windows-machine-config-operator" not found

The namespace creation I think is being done by the UI based on the suggested namespace in the CSV unless something has changed recently.

That's correct when deploying the operator using the official deployment method using the CatalogSource along with an operator index.

However, for the e2e tests (run-ci-e2e-test.sh) the operator is deployed using OLM where the namespace is created first followed by the creation of the cloud-private-key secret. Therefore, the Pod Security Admission labels are required in this step.

jrvaldes · 2022-09-01T14:53:34Z

 		return err
 	}
 	if err == nil {
+		// TODO: ensure required labels


Will this be a valid corner case? What is the probability that another entity created the wmco-test namespace?

None. This is an e2e test. Lets not over think this.

Makes sense.

jrvaldes · 2022-09-01T15:00:42Z

/test azure-e2e-operator

mansikulkarni96 · 2022-09-01T15:14:01Z


+	workloadNamespaceLabels := map[string]string{
+		// turn off the automatic label synchronization required for PodSecurity admission
+		"security.openshift.io/scc.podSecurityLabelSync": "false",


Should this be true or false did we get a clarity on this?

As you pointed out, the test namespace (wmco-test) is not "openshift-" prefixed, hence false should be fine.

jrvaldes · 2022-09-01T16:14:48Z

/test vsphere-e2e-operator

jrvaldes · 2022-09-01T18:18:24Z

/test azure-e2e-operator

jrvaldes · 2022-09-01T19:59:42Z

+    "security.openshift.io/scc.podSecurityLabelSync=true"
+    # set pods security profile to privileged. See https://kubernetes.io/docs/concepts/security/pod-security-admission/#pod-security-levels
+    "pod-security.kubernetes.io/enforce=privileged"
+  )


WMCO namespace with labels applied:

$oc describe ns openshift-windows-machine-config-operator Name: openshift-windows-machine-config-operator Labels: kubernetes.io/metadata.name=openshift-windows-machine-config-operator olm.operatorgroup.uid/04ec388c-2739-4afb-88e4-50c10642787d= olm.operatorgroup.uid/573918ed-b56e-4294-bc58-c7d84338c639= openshift.io/cluster-monitoring=true pod-security.kubernetes.io/enforce=privileged security.openshift.io/scc.podSecurityLabelSync=true Annotations: openshift.io/sa.scc.mcs: s0:c27,c4 openshift.io/sa.scc.supplemental-groups: 1000710000/10000 openshift.io/sa.scc.uid-range: 1000710000/10000 Status: Active No resource quota. No LimitRange resource.

The openshift-wmco namespace should have podSecurityLabelSync=true right? Just want to confirm since the posted output says false

@saifshaikh48 you are correct. I updated the above output.

jrvaldes · 2022-09-02T15:01:18Z

/test azure-e2e-operator

openshift-ci · 2022-09-02T16:02:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, jrvaldes

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:

~~OWNERS~~ [aravindhp]

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

saifshaikh48

/hold

one comment in the e2e suite

saifshaikh48 · 2022-09-02T18:47:05Z

+    "security.openshift.io/scc.podSecurityLabelSync=true"
+    # set pods security profile to privileged. See https://kubernetes.io/docs/concepts/security/pod-security-admission/#pod-security-levels
+    "pod-security.kubernetes.io/enforce=privileged"
+  )


The openshift-wmco namespace should have podSecurityLabelSync=true right? Just want to confirm since the posted output says false

saifshaikh48 · 2022-09-02T18:52:17Z

+		// ensure namespace was properly created with the required labels
+		for k, expectedValue := range labels {
+			if foundValue, found := ns.Labels[k]; found && expectedValue != foundValue {
+				return fmt.Errorf("labels mismatch for namespace %s label: %s expected: %s found: %s",
+					name, k, expectedValue, foundValue)
+			}
+		}
+		// required labels present in namespace, nothing to do!


I'm not sure this logic is complete. What if the namespace already exists but without a required label? We should by applying the missing ones

What if the namespace already exists but without a required label?

Throws an error. The probability for this edge case to happen is very low since the namespace is maintained by the e2e test suite.

I was overthinking this, and now you are too 😃 .

Fair enough

The only use case I see is in our local testing where we are playing around with the labels and don't want the tests to fail if they don't exist.

jrvaldes · 2022-09-02T20:02:13Z

/test aws-e2e-operator

PR merged in the release repo to fix CI issues with AWS compute capacity

Prevent frequent m6a capacity overruns release#31828

saifshaikh48 · 2022-09-02T20:09:55Z

/lgtm

Feel free to remove the hold when we can get passed the AWS cluster issues

jrvaldes · 2022-09-02T21:05:34Z

/retest-required

jrvaldes · 2022-09-02T21:19:05Z

/hold cancel

Infrastructure resources successfully provisioned in AWS CI job

openshift-ci-robot · 2022-09-02T23:29:04Z

/retest-required

Remaining retests: 0 against base HEAD 9db93ab and 2 for PR HEAD e2aa015 in total

jrvaldes · 2022-09-03T01:00:11Z

/retest-required

aravindhp · 2022-09-03T16:59:03Z

/retest-required

jrvaldes · 2022-09-04T02:22:25Z

/hold

to investigate CI failure in aws-e2e-ccm-install jobs

time="2022-09-02T22:24:49Z" level=error msg="Bootstrap failed to complete: timed out waiting for the condition"
time="2022-09-02T22:24:49Z" level=error msg="Failed to wait for bootstrapping to complete. This error usually happens when there is a problem with control plane hosts that prevents the control plane operators from creating the control plane."

jrvaldes · 2022-09-05T17:14:01Z

/test aws-e2e-ccm-install

aravindhp · 2022-09-06T18:41:49Z

/retest-required

aravindhp · 2022-09-06T20:40:14Z

/hold cancel

Looks like the cluster came up and the WMCO tests are running

INFO[2022-09-06T19:41:08Z] Running step aws-e2e-ccm-install-ipi-install-install. 
INFO[2022-09-06T20:26:08Z] Step aws-e2e-ccm-install-ipi-install-install succeeded after 45m0s. 
INFO[2022-09-06T20:26:08Z] Running step aws-e2e-ccm-install-ipi-install-times-collection. 
INFO[2022-09-06T20:26:28Z] Step aws-e2e-ccm-install-ipi-install-times-collection succeeded after 20s. 
INFO[2022-09-06T20:26:28Z] Step phase pre succeeded after 47m10s.       
INFO[2022-09-06T20:26:28Z] Running multi-stage phase test               
INFO[2022-09-06T20:26:28Z] Running step aws-e2e-ccm-install-windows-e2e-operator-test.

jrvaldes · 2022-09-06T22:07:09Z

/hold cancel

openshift-ci · 2022-09-06T22:10:12Z

@jrvaldes: all tests passed!

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2022

aravindhp suggested changes Sep 1, 2022

View reviewed changes

Comment thread hack/common.sh

jrvaldes force-pushed the WINC-879 branch from 4f71611 to 54364dd Compare September 1, 2022 13:59

jrvaldes changed the title ~~WINC-879: [e2e] Add Pod Security admission labels to WMCO namespace~~ WINC-879: [e2e] Add Pod Security admission labels to test namespace Sep 1, 2022

jrvaldes force-pushed the WINC-879 branch from 54364dd to 5143e35 Compare September 1, 2022 14:00

sebsoto reviewed Sep 1, 2022

View reviewed changes

jrvaldes force-pushed the WINC-879 branch from 5143e35 to 7a80a1b Compare September 1, 2022 14:48

jrvaldes commented Sep 1, 2022

View reviewed changes

mansikulkarni96 reviewed Sep 1, 2022

View reviewed changes

jrvaldes force-pushed the WINC-879 branch from 7a80a1b to 905f391 Compare September 1, 2022 16:14

jrvaldes requested review from aravindhp, mansikulkarni96 and sebsoto and removed request for aravindhp, mansikulkarni96 and sebsoto September 1, 2022 16:15

jrvaldes force-pushed the WINC-879 branch from 905f391 to d487178 Compare September 1, 2022 17:06

jrvaldes changed the title ~~WINC-879: [e2e] Add Pod Security admission labels to test namespace~~ WINC-879: [e2e] Add Pod Security admission labels Sep 1, 2022

jrvaldes force-pushed the WINC-879 branch from 86a3e86 to 719219d Compare September 1, 2022 18:17

jrvaldes commented Sep 1, 2022

View reviewed changes

jrvaldes marked this pull request as ready for review September 1, 2022 20:01

jrvaldes requested review from aravindhp and removed request for mansikulkarni96 September 2, 2022 15:03

jrvaldes mentioned this pull request Sep 2, 2022

Point MachineSet references to the official documentation in the Red Hat released CSV #1232

Merged

aravindhp approved these changes Sep 2, 2022

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022

saifshaikh48 reviewed Sep 2, 2022

View reviewed changes

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 2, 2022

openshift-ci Bot assigned saifshaikh48 Sep 2, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 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 2, 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 4, 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 6, 2022

openshift-merge-robot merged commit 82a718c into openshift:master Sep 6, 2022

jrvaldes mentioned this pull request Sep 7, 2022

WINC-520: Stop downloading oc client and leverage test image binaries #1216

Merged

jrvaldes deleted the WINC-879 branch August 8, 2023 21:08

Conversation

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

Uh oh!

Uh oh!

openshift-ci Bot commented Sep 1, 2022

Uh oh!

jrvaldes commented Sep 1, 2022

Uh oh!

jrvaldes commented Sep 1, 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!

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!

jrvaldes commented Sep 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrvaldes commented Sep 1, 2022

Uh oh!

jrvaldes commented Sep 1, 2022

Uh oh!

jrvaldes Sep 1, 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!

jrvaldes commented Sep 2, 2022

Uh oh!

openshift-ci Bot commented Sep 2, 2022

Uh oh!

saifshaikh48 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

jrvaldes commented Sep 2, 2022

Uh oh!

saifshaikh48 commented Sep 2, 2022

Uh oh!

jrvaldes commented Sep 2, 2022

Uh oh!

jrvaldes commented Sep 2, 2022

Uh oh!

openshift-ci-robot commented Sep 2, 2022

Uh oh!

jrvaldes commented Sep 3, 2022

Uh oh!

aravindhp commented Sep 3, 2022

Uh oh!

jrvaldes commented Sep 4, 2022

Uh oh!

jrvaldes commented Sep 1, 2022 •

edited

Loading

jrvaldes Sep 1, 2022 •

edited

Loading