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

major version upgrade for rootless and ocp : solving #1689 #1770

Merged
merged 9 commits into from Mar 31, 2022

Conversation

neelasha-09
Copy link
Contributor

@neelasha-09 neelasha-09 commented Feb 3, 2022

We tested the solution in 3 places

  1. Openshift (with reduced perms)
  2. AWS (with access runAsNonRoot: false )
  3. Pure Vanila k8s

At this stage, we cannot change spilo image instead.
Reason: PGO does not call a wrapper shell script (sh), but directly the python process. If it were a shell, we could have added this logic there. But now, it's too late, and only place to do it at this time is PGO.

The solution is conditionally applying solution of #1689 when root-less

major version upgrade for rootless and ocp : solving zalando#1689
@neelasha-09
Copy link
Contributor Author

@FxKu @CyberDem0n @Jan-M @sdudoladov : Could you please review the PR ?

@neelasha-09
Copy link
Contributor Author

@FxKu : Could you please review and update ?

@FxKu
Copy link
Member

FxKu commented Feb 23, 2022

@neelasha-09 well you addressed one comment. What about the other? There's some redundant code where only the command
"/bin/bash" vs. "/bin/su" differ. It can be simplified.

@neelasha-09
Copy link
Contributor Author

@FxKu
execCommand has different number of args in each case
so we cannot simply have a string with 2 values, it's two different execCommands.
We agree with modifying the code if that is requried.

@FxKu
Copy link
Member

FxKu commented Feb 24, 2022

indeed, then still move all the redundant parts out of the if else block.

@neelasha-09
Copy link
Contributor Author

@FxKu Can you please suggest how can it be done ?

@neelasha-09
Copy link
Contributor Author

@FxKu : Please check and approve the PR

@FxKu
Copy link
Member

FxKu commented Mar 25, 2022

@neelasha-09 please fix the errors (e.g. define result variable etc.) so that unit tests will run.

@FxKu FxKu added this to the 1.8 milestone Mar 25, 2022
@neelasha-09
Copy link
Contributor Author

@FxKu the command at line 120 was added to define the variable as I'm not sure of the type of the variables to be defined for result and err
result, err := c.ExecCommand(podName, "/bin/bash")
Please confirm the same

@FxKu
Copy link
Member

FxKu commented Mar 29, 2022

You will know it if you check what ExecCommand function returns? Since err is already defined above is enough to just define var result string

@FxKu
Copy link
Member

FxKu commented Mar 30, 2022

Only minor issues left. Please remove some unnecessary extra lines that you have introduced. Another event can be changed to warning. Please, make sure go.fmt has been run across your code.

removed additional spaces
@CyberDem0n
Copy link
Contributor

👍

1 similar comment
@FxKu
Copy link
Member

FxKu commented Mar 31, 2022

👍

@FxKu FxKu merged commit f5cca1a into zalando:master Mar 31, 2022
@FxKu
Copy link
Member

FxKu commented Mar 31, 2022

Thanks @neelasha-09 for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants