Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions hack/patches/006-version.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
diff --git a/vendor/knative.dev/pkg/environment/client_config.go b/vendor/knative.dev/pkg/environment/client_config.go
index 04d4220b..d094bf0c 100644
--- a/vendor/knative.dev/pkg/environment/client_config.go
+++ b/vendor/knative.dev/pkg/environment/client_config.go
@@ -42,9 +42,10 @@ func (c *ClientConfig) InitFlags(fs *flag.FlagSet) {
fs.StringVar(&c.ServerURL, "server", "",
"The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")

- fs.StringVar(&c.Kubeconfig, "kubeconfig", os.Getenv("KUBECONFIG"),
- "Path to a kubeconfig. Only required if out-of-cluster.")
-
+ if fs.Lookup("kubeconfig") == nil {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is required due to double vendoring for controller runtime and go client.

+ fs.StringVar(&c.Kubeconfig, "kubeconfig", os.Getenv("KUBECONFIG"),
+ "Path to a kubeconfig. Only required if out-of-cluster.")
+ }
fs.IntVar(&c.Burst, "kube-api-burst", 0, "Maximum burst for throttle.")

fs.Float64Var(&c.QPS, "kube-api-qps", 0, "Maximum QPS to the server from the client.")
diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go b/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go
index 235a7e45..d4110dc9 100644
--- a/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go
+++ b/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go
@@ -35,9 +35,12 @@ var (
)

func init() {
- // TODO: Fix this to allow double vendoring this library but still register flags on behalf of users
- flag.StringVar(&kubeconfig, "kubeconfig", "",
- "Paths to a kubeconfig. Only required if out-of-cluster.")
+ // Need to avoid conflict with knative sharedmain kubeconfig parsing
+ // For more check here: https://github.com/kubernetes-sigs/controller-runtime/issues/878
+ if flag.Lookup("kubeconfig") == nil {
+ flag.StringVar(&kubeconfig, "kubeconfig", "",
+ "Paths to a kubeconfig. Only required if out-of-cluster.")
+ }
}

// GetConfig creates a *rest.Config for talking to a Kubernetes API server.
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,8 @@ func (r *ReconcileKnativeKafka) transform(manifest *mf.Manifest, instance *serve
return err
}
}
m, err := manifest.Transform(
tfs := []mf.Transformer{}
tfs = append(append(tfs,
mf.InjectOwner(instance),
common.SetAnnotations(map[string]string{
common.KafkaOwnerName: instance.Name,
Expand All @@ -317,8 +318,8 @@ func (r *ReconcileKnativeKafka) transform(manifest *mf.Manifest, instance *serve
ImageTransform(common.BuildImageOverrideMapFromEnviron(os.Environ(), "KAFKA_IMAGE_")),
socommon.VersionedJobNameTransform(),
socommon.ConfigMapVolumeChecksumTransform(context.Background(), r.client, sets.NewString("config-tracing", "kafka-config-logging")),
rbacProxyTranform,
)
rbacProxyTranform), socommon.DeprecatedAPIsTranformersFromConfig()...)
m, err := manifest.Transform(tfs...)
if err != nil {
return fmt.Errorf("failed to transform manifest: %w", err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,9 @@ spec:
runAsNonRoot: true
capabilities:
drop:
- all
- ALL
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Using all will not work. Warnings still appear. Need capital letters for capabilities.

seccompProfile:
type: RuntimeDefault
# Openshift specific extensions in the "old" operator format. Ships KnativeKafka and
# other Openshift specifica that have not yet been moved to the operator above.
- name: knative-openshift
Expand All @@ -515,6 +517,14 @@ spec:
volumeMounts:
- mountPath: /cli-artifacts
name: cli-artifacts
securityContext:
allowPrivilegeEscalation: false
runAsNonRoot: true
capabilities:
drop:
- ALL
seccompProfile:
type: RuntimeDefault
Comment on lines +520 to +527
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we see if we can land those on upstream? Or directly on midstream first ?

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Sep 20, 2022

Choose a reason for hiding this comment

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

For 1.26 yes that is also the goal for 1.25 we want to move fast. That is why I didnt start with main branch.
Upstream there is a ticket for fixing stuff for 1.8 let's see what we can port back.

containers:
- name: knative-openshift
# This reference will be replaced in local builds and CI via hack/lib/catalogsource.bash.
Expand Down Expand Up @@ -657,7 +667,9 @@ spec:
runAsNonRoot: true
capabilities:
drop:
- all
- ALL
seccompProfile:
type: RuntimeDefault
volumes:
- name: cli-artifacts
emptyDir: {}
Expand Down Expand Up @@ -703,7 +715,9 @@ spec:
runAsNonRoot: true
capabilities:
drop:
- all
- ALL
seccompProfile:
type: RuntimeDefault
webhookdefinitions:
- generateName: validating.knativeeventings.operator.serverless.openshift.io
type: ValidatingAdmissionWebhook
Expand Down
177 changes: 177 additions & 0 deletions openshift-knative-operator/pkg/common/api.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
package common

import (
"fmt"

"strings"

batchv1 "k8s.io/api/batch/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/discovery"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"

"github.com/blang/semver/v4"
mf "github.com/manifestival/manifestival"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"

"knative.dev/pkg/ptr"
"sigs.k8s.io/controller-runtime/pkg/client/config"
)

// UpgradePodDisruptionBudget upgrade the API version to policy/v1
func UpgradePodDisruptionBudget() mf.Transformer {
return func(u *unstructured.Unstructured) error {
if u.GetKind() != "PodDisruptionBudget" {
return nil
}
if u.GetAPIVersion() != "policy/v1beta1" {
return nil
}
u.SetAPIVersion("policy/v1")
return nil
}
}

// UpgradeHorizontalPodAutoscaler upgrade the API version to autoscaling/v2
func UpgradeHorizontalPodAutoscaler() mf.Transformer {
return func(u *unstructured.Unstructured) error {
if u.GetKind() != "HorizontalPodAutoscaler" {
return nil
}
if u.GetAPIVersion() != "autoscaling/v2beta2" {
return nil
}
u.SetAPIVersion("autoscaling/v2")
return nil
}
}

// SetSecurityContextForAdmissionController set the required pod security context to avoid issues on K8s 1.25+.
// For more check: https://connect.redhat.com/en/blog/important-openshift-changes-pod-security-standards
func SetSecurityContextForAdmissionController() mf.Transformer {
return func(u *unstructured.Unstructured) error {
switch u.GetKind() {
case "Deployment":
deployment := &appsv1.Deployment{}
if err := scheme.Scheme.Convert(u, deployment, nil); err != nil {
return fmt.Errorf("failed to convert Unstructured to Deployment: %w", err)
}
obj := deployment
podSpec := &deployment.Spec.Template.Spec
containers := podSpec.Containers
for i := range containers {
setPodSecurityContext(&containers[i])
}
if err := scheme.Scheme.Convert(obj, u, nil); err != nil {
return err
}
Comment on lines +56 to +69
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Eventing needs stateful set support as well.

I can log an issue and I can follow up if you don't want to do it in this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I can do it.

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Sep 19, 2022

Choose a reason for hiding this comment

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

Btw I dont see any manifests with StatefulSet.Could you point me to them? Didn't see them in audit reports.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

they are coming in a future release, I thought this will land on main first.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh ok wanted to unblock testing that is why I did it on 1.25 first.

case "Job":
job := &batchv1.Job{}
if err := scheme.Scheme.Convert(u, job, nil); err != nil {
return fmt.Errorf("failed to convert Unstructured to Job: %w", err)
}
obj := job
podSpec := &job.Spec.Template.Spec
containers := podSpec.Containers
for i := range containers {
setPodSecurityContext(&containers[i])
}
if err := scheme.Scheme.Convert(obj, u, nil); err != nil {
return err
}
}
return nil
}
}

func setPodSecurityContext(container *corev1.Container) {
if container.SecurityContext == nil {
container.SecurityContext = &corev1.SecurityContext{
AllowPrivilegeEscalation: ptr.Bool(false),
ReadOnlyRootFilesystem: ptr.Bool(true),
RunAsNonRoot: ptr.Bool(true),
Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}},
SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault},
}
} else {
if container.SecurityContext.RunAsNonRoot == nil {
container.SecurityContext.RunAsNonRoot = ptr.Bool(true)
}
if container.SecurityContext.ReadOnlyRootFilesystem == nil {
container.SecurityContext.ReadOnlyRootFilesystem = ptr.Bool(true)
}
if container.SecurityContext.AllowPrivilegeEscalation == nil {
container.SecurityContext.AllowPrivilegeEscalation = ptr.Bool(false)
}
container.SecurityContext.Capabilities = &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}
if container.SecurityContext.SeccompProfile == nil {
container.SecurityContext.SeccompProfile = &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}
}
}
}

// CheckMinimumVersion checks if current K8s version we are on is higher than the one passed.
// If an error is returned then we
func CheckMinimumVersion(versioner discovery.ServerVersionInterface, version string) error {
v, err := versioner.ServerVersion()
if err != nil {
return err
}
currentVersion, err := semver.Make(normalizeVersion(v.GitVersion))
if err != nil {
return err
}

minimumVersion, err := semver.Make(normalizeVersion(version))
if err != nil {
return err
}

// If no specific pre-release requirement is set, we default to "-0" to always allow
// pre-release versions of the same Major.Minor.Patch version.
if len(minimumVersion.Pre) == 0 {
minimumVersion.Pre = []semver.PRVersion{{VersionNum: 0, IsNum: true}}
}

if currentVersion.LT(minimumVersion) {
return fmt.Errorf("kubernetes version %q is not compatible, need at least %q",
currentVersion, minimumVersion)
}
return nil
}

// DeprecatedAPIsTranformersFromConfig check if we are on the right K8s version and return
// the related transformers. Meant to be used by the knative-openshift operator which uses controller runtime
// and for which we need to construct the discovery value.
func DeprecatedAPIsTranformersFromConfig() []mf.Transformer {
cfg, err := config.GetConfig()
if err != nil {
panic(err)
}
clset := kubernetes.NewForConfigOrDie(cfg)
return DeprecatedAPIsTranformers(clset.Discovery())
}

// DeprecatedAPIsTranformers check if we are on the right K8s version and return
// the related transformers.
func DeprecatedAPIsTranformers(d discovery.DiscoveryInterface) []mf.Transformer {
transformers := []mf.Transformer{}
// Enforce the new version, try to upgrade existing resources for 4.11 to also avoid warnings.
// The policy/v1beta1 API version of PodDisruptionBudget will no longer be served in v1.25.
// The autoscaling/v2beta2 API version of HorizontalPodAutoscaler will no longer be served in v1.26
// TODO: When we move away from releases that bring v1beta1 we can remove this part
if err := CheckMinimumVersion(d, "1.24.0"); err == nil {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Picking 1.24 instead of 1.25 here to avoid warnings 4.11 and a scenario where restricted policy is enforced.

transformers = append(transformers, UpgradePodDisruptionBudget(), UpgradeHorizontalPodAutoscaler(), SetSecurityContextForAdmissionController())
}
Copy link
Copy Markdown
Member

@pierDipi pierDipi Sep 19, 2022

Choose a reason for hiding this comment

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

In the other case, should we try to do the opposite (downgrading)?

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Sep 19, 2022

Choose a reason for hiding this comment

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

I mentioned in the description that this is an alternative. I see upgrading easier because otherwise we will have to patch 1.4 midstream branches (more work). Does not make sense to only patch the downloaded manifests in S-O repo you have to fix the mid stream ones too to be consistent. We can downgrade on main if that works better but dont see any benefits.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When a manifest is at v1 (upgraded version) but the k8s version doesn't understand v1 because it was introduced in a future release.

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Sep 19, 2022

Choose a reason for hiding this comment

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

Yes if we have midstream manifests that are already upgraded yes. Do we have any? For Serving we might have in future branches eg 1.5+. But that is a concern for the main patch I guess.

return transformers
}

func normalizeVersion(v string) string {
if strings.HasPrefix(v, "v") {
// No need to account for unicode widths.
return v[1:]
}
return v
}
Loading