Switch to GRPC sockets rather than using tcp ports #631

saschagrunert · 2021-11-02T12:59:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The enricher as well as the bpf recorder require host network for cgroup
determination. This means if we already serve something on the GRPC tcp
ports, then the operator will fail to deploy.

To work around this error-case, we now rely on unix domain sockets
rather than TCP ports.

Which issue(s) this PR fixes:

None

Does this PR have test?

None

Special notes for your reviewer:

Refers to #618 (comment)

Does this PR introduce a user-facing change?

Switched to unix domain sockets for the GRPC servers.

codecov-commenter · 2021-11-02T13:03:32Z

Codecov Report

Merging #631 (7fd255c) into master (73b1235) will increase coverage by 0.05%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
+ Coverage   43.76%   43.82%   +0.05%     
==========================================
  Files          29       29              
  Lines        1565     1570       +5     
==========================================
+ Hits          685      688       +3     
- Misses        834      835       +1     
- Partials       46       47       +1

saschagrunert · 2021-11-02T13:12:54Z

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-11-02T15:30:18Z

@jhrozek @JAORMX PTAL

JAORMX · 2021-11-02T17:52:31Z

Lgtm. Is the e2e-fedora test working at all? Shall it be rekicked?

saschagrunert · 2021-11-03T08:08:25Z

Lgtm. Is the e2e-fedora test working at all? Shall it be rekicked?

I re-triggered the CI, let's see why it hung.

jhrozek

In general I think it's a nice improvement, just see one inline question about permissions

internal/pkg/daemon/bpfrecorder/bpfrecorder.go

saschagrunert · 2021-11-03T08:54:59Z

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-11-03T11:09:08Z

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-11-04T09:32:26Z

/retest
e2e-fedora seems to hang and requires more debugging

test/tc_selinux_base_usage_test.go

jhrozek · 2021-11-04T15:39:26Z

On Thu, Nov 04, 2021 at 06:35:26AM -0700, Sascha Grunert wrote: @saschagrunert commented on this pull request. > - e.logf("removing policy") - e.kubectl("delete", "selinuxprofile", "errorlogger") - - e.logf("assert policy was removed") - e.assertSelinuxPolicyIsRemoved(nodes, rawPolicyName, maxNodeIterations, sleepBetweenIterations) This seems to block the fedora selinux e2e test and I have no idea how it is related to my code change. @jhrozek do you have any insight here?

Not off-bat, but I'll try running the tests locally in vagrant.

saschagrunert · 2021-11-04T15:59:05Z

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-11-04T16:09:07Z

/retest

jhrozek · 2021-11-04T19:20:06Z

Here's what I'm seeing running the e2e tests locally. The SPO pod restarts (still trying to figure out why) and then:

$RUN kubectl logs spod-8hwd2 security-profiles-operator -nsecurity-profiles-operator
I1104 19:17:02.358924   60463 deleg.go:130] setup "msg"="starting component: spod"  "buildDate"="1980-01-01T00:00:00Z" "compiler"="gc" "gitCommit"="081a559d7cf38c42455fd8329501776b9221820a" "gitTreeState"="clean" "goVersion"="go1.16.9" "libseccomp"="2.5.1" "platform"="linux/amd64" "version"="0.4.0-dev"
I1104 19:17:03.067711   60463 deleg.go:130] controller-runtime/metrics "msg"="metrics server is starting to listen"  "addr"=":8080"
I1104 19:17:03.069201   60463 metrics.go:159] metrics "msg"="Registering metric: seccomp_profile_total"
I1104 19:17:03.069559   60463 metrics.go:159] metrics "msg"="Registering metric: seccomp_profile_audit_total"
I1104 19:17:03.069716   60463 metrics.go:159] metrics "msg"="Registering metric: seccomp_profile_error_total"
I1104 19:17:03.069861   60463 metrics.go:159] metrics "msg"="Registering metric: selinux_profile_total"
I1104 19:17:03.069999   60463 metrics.go:159] metrics "msg"="Registering metric: selinux_profile_audit_total"
I1104 19:17:03.070142   60463 metrics.go:159] metrics "msg"="Registering metric: selinux_profile_error_total"
E1104 19:17:03.070967   60463 deleg.go:144] setup "msg"="running security-profiles-operator" "error"="start metrics grpc server: create listener: listen unix /var/run/grpc/metrics.sock: bind: address already in use"

jhrozek · 2021-11-04T19:35:01Z

huh:

I1104 19:34:02.701789   57516 selinuxpolicy_controller.go:356] selinuxprofile "msg"="Removing policy file" "Request.Name"="errorlogger" "Request.Namespace"="default" "policyPath"="/etc/selinux.d/errorlogger_default.cil"
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x172683e]

goroutine 383 [running]:
github.com/crossplane/crossplane-runtime/pkg/event.Warning(0x1cbb0ef, 0x19, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        github.com/crossplane/crossplane-runtime@v0.14.1-0.20210713194031-85b19c28ea88/pkg/event/event.go:63 +0x5e
sigs.k8s.io/security-profiles-operator/internal/pkg/daemon/selinuxprofile.(*ReconcileSP).Reconcile(0xc000163540, 0x1f1e038, 0xc00002d860, 0xc0007a3639, 0x7, 0xc0007a3624, 0xb, 0xc00002d860, 0xc00002d830, 0xc00054fdb0, ...)
        sigs.k8s.io/security-profiles-operator/internal/pkg/daemon/selinuxprofile/selinuxpolicy_controller.go:185 +0x6be
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0xc000181540, 0x1f1e038, 0xc00002d830, 0xc0007a3639, 0x7, 0xc0007a3624, 0xb, 0xc00002d800, 0x0, 0x0, ...)
        sigs.k8s.io/controller-runtime@v0.10.2/pkg/internal/controller/controller.go:114 +0x247
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc000181540, 0x1f1df90, 0xc000162f00, 0x1adff00, 0xc000048220)
        sigs.k8s.io/controller-runtime@v0.10.2/pkg/internal/controller/controller.go:311 +0x305
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc000181540, 0x1f1df90, 0xc000162f00, 0x0)
        sigs.k8s.io/controller-runtime@v0.10.2/pkg/internal/controller/controller.go:266 +0x205
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2(0xc0004b1950, 0xc000181540, 0x1f1df90, 0xc000162f00)
        sigs.k8s.io/controller-runtime@v0.10.2/pkg/internal/controller/controller.go:227 +0x6b
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
        sigs.k8s.io/controller-runtime@v0.10.2/pkg/internal/controller/controller.go:223 +0x425
{"level":"info","ts":1636054442.7021089,"logger":"file-watcher","caller":"daemon/daemon.go:89","msg":"Removing policy","file":"/etc/selinux.d/errorlogger_default.cil"}
{"level":"info","ts":1636054442.8720913,"caller":"semanage/semanage.go:89","msg":"Removing last errorlogger_default module (no other errorlogger_default module exists at another priority)."}
{"level":"info","ts":1636054450.455779,"logger":"policy-installer","caller":"daemon/daemon.go:130","msg":"The operation was successful","operation":"remove - /etc/selinux.d/errorlogger_default.cil"}

jhrozek · 2021-11-04T21:14:57Z

Can you try if the tests work better if you include #640 in the PR?
(btw I am not working tomorrow, I might not be able to check any comments to that PR until Monday..)

jhrozek · 2021-11-04T21:15:40Z

oh and that PR definitely does not fix the issue related to container restart failing with address already in use, just the crash itself..

saschagrunert · 2021-11-05T08:29:11Z

oh and that PR definitely does not fix the issue related to container restart failing with address already in use, just the crash itself..

We have to remove the socket if the file already exists. Good catch!

The enricher as well as the bpf recorder require host network for cgroup determination. This means if we already serve something on the GRPC tcp ports, then the operator will fail to deploy. To work around this error-case, we now rely on unix domain sockets rather than TCP ports. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert · 2021-11-05T08:43:54Z

/test pull-security-profiles-operator-test-e2e

saschagrunert · 2021-11-05T08:57:01Z

/retest

saschagrunert · 2021-11-05T09:10:19Z

/retest

saschagrunert · 2021-11-05T09:24:28Z

@jhrozek tests are green now :)

saschagrunert · 2021-11-08T13:52:33Z

@JAORMX @pjbgf PTAL

pjbgf

/lgtm

k8s-ci-robot · 2021-11-08T14:19:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pjbgf, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pjbgf,saschagrunert]

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

jhrozek · 2021-11-08T14:57:56Z

/lgtm
thank you, great work on avoiding the 777 permissions

k8s-ci-robot requested review from cmurphy and hasheddan November 2, 2021 12:59

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2021

saschagrunert mentioned this pull request Nov 2, 2021

Add bpf based syscall recorder #618

Merged

7 tasks

saschagrunert force-pushed the grpc-sockets branch from 3c264a5 to 6456c3f Compare November 2, 2021 15:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2021

saschagrunert force-pushed the grpc-sockets branch 2 times, most recently from 2a6f372 to c755643 Compare November 2, 2021 15:22

jhrozek reviewed Nov 3, 2021

View reviewed changes

internal/pkg/daemon/bpfrecorder/bpfrecorder.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the grpc-sockets branch 3 times, most recently from f034a91 to 4249649 Compare November 3, 2021 08:35

saschagrunert force-pushed the grpc-sockets branch from 4249649 to 52ebf48 Compare November 3, 2021 10:52

saschagrunert force-pushed the grpc-sockets branch 2 times, most recently from 7fd255c to eee3589 Compare November 4, 2021 08:53

saschagrunert force-pushed the grpc-sockets branch 2 times, most recently from 252ab13 to 9851e04 Compare November 4, 2021 11:41

saschagrunert commented Nov 4, 2021

View reviewed changes

test/tc_selinux_base_usage_test.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the grpc-sockets branch from 9851e04 to 081a559 Compare November 4, 2021 15:41

saschagrunert force-pushed the grpc-sockets branch from 081a559 to 69f312d Compare November 5, 2021 08:28

saschagrunert force-pushed the grpc-sockets branch from 69f312d to fc4aaca Compare November 5, 2021 08:29

saschagrunert force-pushed the grpc-sockets branch from fc4aaca to dd07307 Compare November 5, 2021 08:31

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2021

pjbgf approved these changes Nov 8, 2021

View reviewed changes

k8s-ci-robot assigned pjbgf Nov 8, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2021

k8s-ci-robot merged commit ac8a762 into kubernetes-sigs:master Nov 8, 2021

saschagrunert deleted the grpc-sockets branch November 8, 2021 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to GRPC sockets rather than using tcp ports #631

Switch to GRPC sockets rather than using tcp ports #631

saschagrunert commented Nov 2, 2021 •

edited

codecov-commenter commented Nov 2, 2021 •

edited

saschagrunert commented Nov 2, 2021

saschagrunert commented Nov 2, 2021

JAORMX commented Nov 2, 2021

saschagrunert commented Nov 3, 2021

jhrozek left a comment

saschagrunert commented Nov 3, 2021

saschagrunert commented Nov 3, 2021

saschagrunert commented Nov 4, 2021

jhrozek commented Nov 4, 2021 via email

saschagrunert commented Nov 4, 2021

saschagrunert commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 8, 2021

pjbgf left a comment

k8s-ci-robot commented Nov 8, 2021

jhrozek commented Nov 8, 2021

Switch to GRPC sockets rather than using tcp ports #631

Switch to GRPC sockets rather than using tcp ports #631

Conversation

saschagrunert commented Nov 2, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR have test?

Special notes for your reviewer:

Does this PR introduce a user-facing change?

codecov-commenter commented Nov 2, 2021 • edited

Codecov Report

saschagrunert commented Nov 2, 2021

saschagrunert commented Nov 2, 2021

JAORMX commented Nov 2, 2021

saschagrunert commented Nov 3, 2021

jhrozek left a comment

Choose a reason for hiding this comment

saschagrunert commented Nov 3, 2021

saschagrunert commented Nov 3, 2021

saschagrunert commented Nov 4, 2021

jhrozek commented Nov 4, 2021 via email

saschagrunert commented Nov 4, 2021

saschagrunert commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

jhrozek commented Nov 4, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 5, 2021

saschagrunert commented Nov 8, 2021

pjbgf left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 8, 2021

jhrozek commented Nov 8, 2021

saschagrunert commented Nov 2, 2021 •

edited

codecov-commenter commented Nov 2, 2021 •

edited