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

selinuxpolicy: updates to enable support for OCP 4.11+/K8s 1.24+ #107

Merged

Conversation

swatisehgal
Copy link
Collaborator

In container-selinux versions v2.186.0, v2.187.0 and v2.188.0, due to the updates here:
containers/container-selinux@cf704e4
selinuxpolicy updates are required (updating container_runtime_t to kubelet_t).

This is consumed in OCP 4.11 and K8s 1.24 and we need to make sure that the selinuxpolicy is
appropriately updated.

In addition to that, we need to continue supporting the older selinuxpolicy supplied in older
versions (e.g. 4.10) and that is the reason this patch is proposing an additional selinuxpolicy
and not completely replacing the original one.

Signed-off-by: Swati Sehgal [email protected]

@swatisehgal
Copy link
Collaborator Author

/cc @fromanirh

@swatisehgal swatisehgal requested review from cynepco3hahue, ffromani and Tal-or and removed request for cynepco3hahue July 13, 2022 11:53
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch from 1adbad8 to 9b7a2b3 Compare July 13, 2022 11:57
pkg/assets/rte/assets.go Outdated Show resolved Hide resolved
pkg/assets/rte/assets.go Outdated Show resolved Hide resolved
pkg/assets/rte/selinuxpolicy-latest.cil Outdated Show resolved Hide resolved
pkg/assets/rte/assets.go Outdated Show resolved Hide resolved
pkg/manifests/manifests.go Outdated Show resolved Hide resolved
pkg/manifests/manifests.go Outdated Show resolved Hide resolved
@ffromani
Copy link
Collaborator

very nice start!

@ffromani ffromani added the api-breaking This change will break API backwards compatibility label Jul 13, 2022
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch 3 times, most recently from 55fb124 to ce1d3eb Compare July 13, 2022 14:55
pkg/manifests/rte/rte.go Outdated Show resolved Hide resolved
pkg/assets/rte/assets.go Outdated Show resolved Hide resolved
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch 3 times, most recently from 62fcc9e to be11cad Compare July 14, 2022 11:03
@ffromani
Copy link
Collaborator

ffromani commented Jul 15, 2022

thanks for the updates. Looks very good. It seems to me that we only miss to update the pkg/commands/deploy.go source/command (we're running out of good names!), like this:

diff --git a/pkg/commands/deploy.go b/pkg/commands/deploy.go
index 4bdcba0..8d24a7c 100644
--- a/pkg/commands/deploy.go
+++ b/pkg/commands/deploy.go
@@ -30,6 +30,7 @@ import (
 
 type DeployOptions struct {
        clusterPlatform platform.Platform
+       clusterVersion  platform.Version
        waitCompletion  bool
 }
 
@@ -62,6 +63,11 @@ func NewRemoveCommand(commonOpts *CommonOptions) *cobra.Command {
                        if opts.clusterPlatform == platform.Unknown {
                                return fmt.Errorf("cannot autodetect the platform, and no platform given")
                        }
+                       verDetect := detectVersion(commonOpts.DebugLog, platDetect.Discovered, commonOpts.UserPlatformVersion)
+                       opts.clusterVersion = verDetect.Discovered
+                       if opts.clusterVersion == platform.MissingVersion {
+                               return fmt.Errorf("cannot autodetect the platform version, and no version given")
+                       }

(note your naming is actually better than the one I used in the quick example above, please use yours :) )

similar changes would be needed all across pkg/commands/deploy.go if I'm right
Unfortunately it's likely the existing e2e test don't cover well enough this flow. If nothing else, we're still on 1.23 in our CI lane.
Surely improving CI can be deferred to another PR.

Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

minor improvements inside

test/e2e/positive.go Show resolved Hide resolved
pkg/assets/rte/assets.go Show resolved Hide resolved
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch from 9b193f6 to 1367489 Compare July 15, 2022 10:34
@swatisehgal
Copy link
Collaborator Author

Need to tidy up a couple of things, on it now.

In container-selinux versions v2.186.0, v2.187.0 and v2.188.0, due to the updates here:
containers/container-selinux@cf704e4
selinuxpolicy updates are required (updating `container_runtime_t` to `kubelet_t`).

This is consumed in OCP 4.11 and K8s 1.24 and we need to make sure that the selinuxpolicy is
appropriately updated.

In addition to that, we need to continue supporting the older selinuxpolicy supplied in older
versions (e.g. 4.10) and that is the reason this patch is proposing an additional selinuxpolicy
and not completely replacing the original one.

Signed-off-by: Swati Sehgal <[email protected]>
Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

almost there. You properly detect the version in the right places, but we need to pass it through another layer

pkg/commands/deploy.go Show resolved Hide resolved
pkg/commands/deploy.go Show resolved Hide resolved
pkg/commands/deploy.go Show resolved Hide resolved
pkg/commands/deploy.go Show resolved Hide resolved
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch from 1367489 to 563d050 Compare July 15, 2022 10:41
Rather than calling detect.Version in the manifest package, we obtain
the version from the top level CommonOptions and pass it all the way
to the manifest package where the platform version is taken into
consideration when loading the selinux policy.

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal swatisehgal force-pushed the selinux-policy-update branch from 563d050 to fb42881 Compare July 15, 2022 10:57
Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

LGTM
we detect the version in more places than is strictly needed. But let's keep it this way, should make the overall UX less surprising.

@ffromani ffromani merged commit 2f9de87 into k8stopologyawareschedwg:main Jul 15, 2022
@swatisehgal swatisehgal deleted the selinux-policy-update branch July 15, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking This change will break API backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants