Skip to content
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

Che Operator Security Context Defined Posture Parameters #811

Merged
merged 12 commits into from
May 12, 2021
Merged

Che Operator Security Context Defined Posture Parameters #811

merged 12 commits into from
May 12, 2021

Conversation

ArvinB
Copy link
Contributor

@ArvinB ArvinB commented Apr 30, 2021

What does this PR do?

It fully declares the securityContext under operator Pod

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#19737

How to test this PR?

Tested on OpenShift 4.7.8 using Wazi Developer for Workspaces 1.2.5 (based on Red Hat CodeReady Workspaces 2.7 (Che 7.26)).

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

Seems OK, but I don't know the ramifications of this change. Do we also need to set new defaults in che.properties or in user docs / guides?

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArvinB, nickboldt

The full list of commands accepted by this bot can be found 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

@ArvinB
Copy link
Contributor Author

ArvinB commented May 5, 2021

@ArvinB: The following test failed, say /retest to rerun all failed tests:
Test name Commit Details Rerun command
ci/prow/v7-devworkspace-happy-path 5d21736 link /test v7-devworkspace-happy-path

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.

It would seem that most PRs are failing to pass the happy-path test suite. Perhaps a /retest might work?

@tolusha
Copy link
Contributor

tolusha commented May 6, 2021

Changes in operator.yaml are pretty safe because they reflect their default values
But setting runAsNonRoot and allowPrivilegeEscalation might have side effects.

I assume that the target platform of this changes is OpenShift and deployment method is olm.
So, I propose the following:

  1. Remove runAsNonRoot and allowPrivilegeEscalation from operator.yaml
  2. Revert all other changes in csv files.
  3. If we need something specifically for OpenShift then let's update OpenShift csv file like here [1]
  4. Launch olm/update-resources.sh to generate csv files.

[1] https://github.com/eclipse-che/che-operator/blob/main/olm/update-resources.sh#L192-L221

ArvinB added 2 commits May 6, 2021 11:36
per @tolusha recommendation, I'm delivering:

[X] Remove runAsNonRoot and allowPrivilegeEscalation from operator.yaml
[X] Revert all other changes in csv files.
[X] If we need something specifically for OpenShift then let's update OpenShift csv file like here [1]

[1] https://github.com/eclipse-che/che-operator/blob/main/olm/update-resources.sh#L192-L221
@ArvinB
Copy link
Contributor Author

ArvinB commented May 6, 2021

@tolusha I completed your recommendation.

I assume running:

Launch olm/update-resources.sh to generate csv files.

...is for testing purposes only? I was able to successfully generate the csv files, but I did not deliver them into this PR?

Thanks.

@tolusha
Copy link
Contributor

tolusha commented May 7, 2021

@ArvinB

.is for testing purposes only? I was able to successfully generate the csv files, but I did not deliver them into this PR?

pls add them to the PR.

@ArvinB
Copy link
Contributor Author

ArvinB commented May 7, 2021

@tolusha Update the PR thank you.

olm/update-resources.sh Outdated Show resolved Hide resolved
olm/update-resources.sh Outdated Show resolved Hide resolved
olm/update-resources.sh Outdated Show resolved Hide resolved
ArvinB added 2 commits May 7, 2021 11:04
@tolusha Updates made based on your feedback. Again thank you very much!
@ArvinB
Copy link
Contributor Author

ArvinB commented May 7, 2021

/retest

@ArvinB
Copy link
Contributor Author

ArvinB commented May 10, 2021

@tolusha Good to merge?

@tolusha
Copy link
Contributor

tolusha commented May 11, 2021

@ArvinB
I've run the PR checks.

@tolusha
Copy link
Contributor

tolusha commented May 11, 2021

We can ignore minikube tests for now, they are known problem.
Regarding resources and bundle-version checks. It turned out that fields with default values doesn't appear in CSV file.
My bad, we need those changes [1]. Once you add them pls run olm/update-resources.sh and commit changes. Thank you.

[1] 416f78b#diff-82b7ba34bc00ed0fcaa5f46c8eea16961909f41152e2477c016195ac74b66729R227-R229

@ArvinB
Copy link
Contributor Author

ArvinB commented May 11, 2021

We can ignore minikube tests for now, they are known problem.
Regarding resources and bundle-version checks. It turned out that fields with default values doesn't appear in CSV file.
My bad, we need those changes [1]. Once you add them pls run olm/update-resources.sh and commit changes. Thank you.

[1] 416f78b#diff-82b7ba34bc00ed0fcaa5f46c8eea16961909f41152e2477c016195ac74b66729R227-R229

Changes made @tolusha
Thank you!

@tolusha
Copy link
Contributor

tolusha commented May 11, 2021

bundle-version version check failed.
Pls run olm/update-resources.sh one more time and commit changes (it will increase nightly bundle version from 174 to 175).
Thank you.

@ArvinB
Copy link
Contributor Author

ArvinB commented May 11, 2021

@tolusha Done. However, I get a conflict after pushing in your requested changes. Any suggestions?

@tolusha
Copy link
Contributor

tolusha commented May 11, 2021

No problem. Resolve conflicts by taking your changes.

@ArvinB
Copy link
Contributor Author

ArvinB commented May 12, 2021

/retest

@tolusha
Copy link
Contributor

tolusha commented May 12, 2021

/test v7-devworkspace-happy-path

@openshift-ci
Copy link

openshift-ci bot commented May 12, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArvinB, nickboldt, tolusha

The full list of commands accepted by this bot can be found 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

@tolusha
Copy link
Contributor

tolusha commented May 12, 2021

/test v7-devworkspace-happy-path

@tolusha tolusha merged commit ae699a7 into eclipse-che:main May 12, 2021
@ArvinB ArvinB deleted the Operator_Security_Context_Params branch May 12, 2021 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants