Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

feat: add container security context support #995

Merged
merged 3 commits into from
Jul 19, 2019
Merged

feat: add container security context support #995

merged 3 commits into from
Jul 19, 2019

Conversation

arnaud-deprez
Copy link
Contributor

@arnaud-deprez arnaud-deprez commented Jul 9, 2019

Closes #973

Description

Should fix #973

Motivation and Context

Allow to define Container Security Context for both hub and selenium grid.
Attention the same value is reused for both Pods.

How Has This Been Tested?

Unit tests and manual integration tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@arnaud-deprez
Copy link
Contributor Author

Hi @davidcollom, @diemol, @martincfo,

Sorry for the delay, I was quite busy these days.

I did a change in #995 that is equivalent to #904 but for Container Security Context.
So in short, this allows to configure Container Security Context for both hub and grid but it does not allow to use different context/configuration.

In order to do that, I think we should foresee deeper/heavier changes. While it is still possible to implement such a feature as it is, I feel the chart and the number of environment variables to configure it all starts getting too heavy and confusing.
For example for Pod Security Context or Container Security Context, we should allow user to set json value in environment variable as it is impossible to guess all combination.

So instead of adding again some environment variable, I was thinking about using a configuration file which will look like a lot to Pod for the grid.

However, this idea is not clear yet in my mind as there are a lot of environment variables used for docker and docker swarm setup and so we will need to define what takes precedence over what or review the whole thing.

So basically, I propose to integrate this change as-is and then we can think about a better design/solution to fully customise zalenium deployment.

What do you think ?

@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #995 into master will decrease coverage by 0.2%.
The diff coverage is 62.5%.

@@             Coverage Diff              @@
##             master     #995      +/-   ##
============================================
- Coverage     55.03%   54.83%   -0.21%     
+ Complexity      667      665       -2     
============================================
  Files            57       57              
  Lines          4519     4532      +13     
  Branches        425      425              
============================================
- Hits           2487     2485       -2     
- Misses         1799     1816      +17     
+ Partials        233      231       -2

@arnaud-deprez arnaud-deprez changed the title [WIP] feat: add container security context support feat: add container security context support Jul 10, 2019
@diemol
Copy link
Contributor

diemol commented Jul 19, 2019

I think so far it looks good @arnaud-deprez, thanks!
If we need something more granunar in the future, then we could revise this.

@diemol diemol merged commit 03559f5 into zalando:master Jul 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement container-scoped securityContext configuration
3 participants