Added security context and changed init container logic for onos #221

breathbath · 2020-11-12T11:00:17Z

…rds atomix

Andrea-Campanella

@pierventre @charlesmcchan @kuujo @hwchiu can you please take a look at this one too ?

Andrea-Campanella · 2020-11-12T11:31:22Z

onos-classic/values.yaml

@@ -54,3 +58,14 @@ ports:
 #     log4j2.appender.console.name = Console
 #     log4j2.appender.console.layout.type = PatternLayout
 #     log4j2.appender.console.layout.pattern = %d{RFC3339} %-5level [%c{1}] %msg%n%throwable
+
+podSecurityContext:


again might be worth run-in with these two disabled by default @kuujo thoughts ?

no because default should be a secure setup and if a user wants to make it insecure he should do it explicitly otherwise we provide a non-secure definition by default

hwchiu · 2020-11-13T05:38:24Z

onos-classic/values.yaml

+
+podSecurityContext:
+  runAsUser: 1000
+  fsGroup: 2000


Do we have any user with UID 1000 and group with GID 2000 in all images?

I think this will be enforced by k8s

charlesmcchan

Everything else LGTM. We should test this in TOST/SEBA.

onos-classic/templates/statefulset.yaml

charlesmcchan · 2020-11-13T05:57:31Z

onos-classic/values.yaml

@@ -54,3 +58,14 @@ ports:
 #     log4j2.appender.console.name = Console
 #     log4j2.appender.console.layout.type = PatternLayout
 #     log4j2.appender.console.layout.pattern = %d{RFC3339} %-5level [%c{1}] %msg%n%throwable
+
+podSecurityContext:
+  runAsUser: 1000


It is not very clear what this user and group are. May be better to use name or add a comment.

no, k8s security context identifies a root user as a user having non-numbers in the id, so even admin or some123 will be considered to be root so we have to use numeric ids

hwchiu · 2020-11-17T22:26:08Z

I ran your changed in my environment but the ONOS container failed to start up.

╰─$ kc get pods
kNAME                         READY   STATUS    RESTARTS   AGE
debug-pod-5b5c57fccd-9ztxv   1/1     Running   0          6d3h
github-221-atomix-0          1/1     Running   0          49s
github-221-atomix-1          1/1     Running   0          49s
github-221-atomix-2          1/1     Running   0          49s
github-221-onos-classic-0    0/1     Error     2          49s
github-221-onos-classic-1    0/1     Error     2          49s
github-221-onos-classic-2    0/1     Error     2          49s
╰─$ kc logs -f github-221-onos-classic-0
./bin/onos-service: line 26: cd: /root/onos: Permission denied

It looks like some files are root accessed only and are installed during the docker build time ?
cc @Andrea-Campanella @charlesmcchan

Andrea-Campanella · 2020-11-18T11:07:27Z

I ran your changed in my environment but the ONOS container failed to start up.

╰─$ kc get pods
kNAME                         READY   STATUS    RESTARTS   AGE
debug-pod-5b5c57fccd-9ztxv   1/1     Running   0          6d3h
github-221-atomix-0          1/1     Running   0          49s
github-221-atomix-1          1/1     Running   0          49s
github-221-atomix-2          1/1     Running   0          49s
github-221-onos-classic-0    0/1     Error     2          49s
github-221-onos-classic-1    0/1     Error     2          49s
github-221-onos-classic-2    0/1     Error     2          49s
╰─$ kc logs -f github-221-onos-classic-0
./bin/onos-service: line 26: cd: /root/onos: Permission denied

It looks like some files are root accessed only and are installed during the docker build time ?
cc @Andrea-Campanella @charlesmcchan

it's missing the changes required in the ONOS docker file @breathbath is upstreaming those (pending ONOS gerrit issues here in Europe)

Andrea-Campanella · 2020-11-20T08:58:41Z

I ran your changed in my environment but the ONOS container failed to start up.
╰─$ kc get pods
kNAME                         READY   STATUS    RESTARTS   AGE
debug-pod-5b5c57fccd-9ztxv   1/1     Running   0          6d3h
github-221-atomix-0          1/1     Running   0          49s
github-221-atomix-1          1/1     Running   0          49s
github-221-atomix-2          1/1     Running   0          49s
github-221-onos-classic-0    0/1     Error     2          49s
github-221-onos-classic-1    0/1     Error     2          49s
github-221-onos-classic-2    0/1     Error     2          49s
╰─$ kc logs -f github-221-onos-classic-0
./bin/onos-service: line 26: cd: /root/onos: Permission denied
It looks like some files are root accessed only and are installed during the docker build time ?
cc @Andrea-Campanella @charlesmcchan
it's missing the changes required in the ONOS docker file @breathbath is upstreaming those (pending ONOS gerrit issues here in Europe)

@hwchiu can you try building ONOS with https://gerrit.onosproject.org/c/onos/+/24146 and then using this chart ?

charlesmcchan · 2020-11-20T09:11:23Z

Be careful. Our released docker images were not built with this new Dockerfile.
This helm chart change still will not work with our released docker images.

breathbath · 2020-11-23T09:18:38Z

Be careful. Our released docker images were not built with this new Dockerfile.
This helm chart change still will not work with our released docker images.

That's why the helm charts should reference hardcoded references to released docker images otherwise you cannot guarantee compatibility in future incompatible changes as well. Obviously you cannot prevent the incompatible changes to appear.

charlesmcchan · 2020-11-23T18:41:21Z

Be careful. Our released docker images were not built with this new Dockerfile.
This helm chart change still will not work with our released docker images.

That's why the helm charts should reference hardcoded references to released docker images otherwise you cannot guarantee compatibility in future incompatible changes as well. Obviously you cannot prevent the incompatible changes to appear.

Yes. I was just saying that we need to release a new ONOS and TOST image based on the new Dockerfile, and update the default image version in the helm chart accordingly before we can merge this.

hwchiu · 2020-11-23T19:06:16Z

I ran your changed in my environment but the ONOS container failed to start up.
╰─$ kc get pods
kNAME                         READY   STATUS    RESTARTS   AGE
debug-pod-5b5c57fccd-9ztxv   1/1     Running   0          6d3h
github-221-atomix-0          1/1     Running   0          49s
github-221-atomix-1          1/1     Running   0          49s
github-221-atomix-2          1/1     Running   0          49s
github-221-onos-classic-0    0/1     Error     2          49s
github-221-onos-classic-1    0/1     Error     2          49s
github-221-onos-classic-2    0/1     Error     2          49s
╰─$ kc logs -f github-221-onos-classic-0
./bin/onos-service: line 26: cd: /root/onos: Permission denied
It looks like some files are root accessed only and are installed during the docker build time ?
cc @Andrea-Campanella @charlesmcchan
it's missing the changes required in the ONOS docker file @breathbath is upstreaming those (pending ONOS gerrit issues here in Europe)
@hwchiu can you try building ONOS with https://gerrit.onosproject.org/c/onos/+/24146 and then using this chart ?

ok, I will test it in the next two days.

hwchiu · 2020-11-23T23:42:55Z

@breathbath @Andrea-Campanella
I tried the Gerrit patch you pointed out but get the following error.

touch: cannot touch '/src/onos/apps/org.onosproject.openflow-base/active': Permission denied

Update:
I fixed the issue by changing the owner after copying files.
Kubernetes pods are running but the readiness probe still fail.

Readiness probe failed: sh: 1: /root/onos/bin/check-onos-status: Permission denied

It looks like we need to fix location of all mounted files.

breathbath · 2020-11-26T09:30:07Z

hey @hwchiu thanks for testing. I've made some changes to the dockerfile, can we verify the setup with the latest changes?
https://gerrit.onosproject.org/c/onos/+/24146/6/Dockerfile?

hwchiu · 2020-11-26T18:08:27Z

hey @hwchiu thanks for testing. I've made some changes to the dockerfile, can we verify the setup with the latest changes?
https://gerrit.onosproject.org/c/onos/+/24146/6/Dockerfile?

I tried but had the different issue. see my post above.
Readiness probe failed: sh: 1: /root/onos/bin/check-onos-status: Permission denied

These files are mounted to the container in the run time, if we aren't the root, we should not mount files into /root directory.

hwchiu · 2021-08-24T16:49:13Z

@breathbath
Since you changed the WORKDIR from the root/onos to src/onos in this patch https://gerrit.onosproject.org/c/onos/+/24146/6/Dockerfile, please help to mount point in the k8s yaml so that all required files are mounted into the correct location src/onos.

onf-bot · 2021-08-24T16:49:15Z

Can one of the admins verify this patch?

Added security context and changed init container logic for onos towa…

f1757c0

…rds atomix

Andrea-Campanella approved these changes Nov 12, 2020

View reviewed changes

Andrea-Campanella reviewed Nov 12, 2020

View reviewed changes

charlesmcchan requested a review from hwchiu November 13, 2020 05:30

hwchiu reviewed Nov 13, 2020

View reviewed changes

charlesmcchan reviewed Nov 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added security context and changed init container logic for onos #221

Added security context and changed init container logic for onos #221

breathbath commented Nov 12, 2020

Andrea-Campanella left a comment •

edited

Loading

Andrea-Campanella Nov 12, 2020

breathbath Nov 13, 2020

hwchiu Nov 13, 2020

breathbath Nov 13, 2020

charlesmcchan left a comment

charlesmcchan Nov 13, 2020

breathbath Nov 13, 2020

hwchiu commented Nov 17, 2020

Andrea-Campanella commented Nov 18, 2020 •

edited

Loading

Andrea-Campanella commented Nov 20, 2020

charlesmcchan commented Nov 20, 2020 •

edited

Loading

breathbath commented Nov 23, 2020

charlesmcchan commented Nov 23, 2020

hwchiu commented Nov 23, 2020

hwchiu commented Nov 23, 2020 •

edited

Loading

breathbath commented Nov 26, 2020

hwchiu commented Nov 26, 2020

hwchiu commented Aug 24, 2021

onf-bot commented Aug 24, 2021

Added security context and changed init container logic for onos #221

Are you sure you want to change the base?

Added security context and changed init container logic for onos #221

Conversation

breathbath commented Nov 12, 2020

Andrea-Campanella left a comment • edited Loading

Choose a reason for hiding this comment

Andrea-Campanella Nov 12, 2020

Choose a reason for hiding this comment

breathbath Nov 13, 2020

Choose a reason for hiding this comment

hwchiu Nov 13, 2020

Choose a reason for hiding this comment

breathbath Nov 13, 2020

Choose a reason for hiding this comment

charlesmcchan left a comment

Choose a reason for hiding this comment

charlesmcchan Nov 13, 2020

Choose a reason for hiding this comment

breathbath Nov 13, 2020

Choose a reason for hiding this comment

hwchiu commented Nov 17, 2020

Andrea-Campanella commented Nov 18, 2020 • edited Loading

Andrea-Campanella commented Nov 20, 2020

charlesmcchan commented Nov 20, 2020 • edited Loading

breathbath commented Nov 23, 2020

charlesmcchan commented Nov 23, 2020

hwchiu commented Nov 23, 2020

hwchiu commented Nov 23, 2020 • edited Loading

breathbath commented Nov 26, 2020

hwchiu commented Nov 26, 2020

hwchiu commented Aug 24, 2021

onf-bot commented Aug 24, 2021

Andrea-Campanella left a comment •

edited

Loading

Andrea-Campanella commented Nov 18, 2020 •

edited

Loading

charlesmcchan commented Nov 20, 2020 •

edited

Loading

hwchiu commented Nov 23, 2020 •

edited

Loading