feat(deployment): update and secure metacontroller

[juliusvonkohout]

@Bobgy @zijianjoy @orugantichetan
Fixes #5578

I

• upgraded to 2.0.4
• removed insane permissions (e.g. privileged root container)
• removed insane cpu and memory ressources (4 cores 4 GB)
• restricted the namespaces to kubeflow profile namespaces
• Reduced to a sane amount of logging
• Omitted networkattachmentdefinitions for rootless istio-cni on Openshift and i did not update the profile-controller accordingly. I can still add them if someone is interested. This would also add something to pipelines-profile-controller sync.py https://istio.io/latest/docs/setup/platform-setup/openshift/#additional-requirements-for-the-application-namespace. I can also create a second pull request for that. In the same pull request i could also add the poddefault for authentication with serviceaccounts Add authentication with ServiceAccountToken #5138 (comment) @bartgras

Sadly it still needs cluster-admin rights, but maybe someone has an idea on how to restrict it even further.

[google-oss-robot]

Hi @juliusvonkohout. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Bobgy]

/lgtm
/assign @zijianjoy
For final approval

[Bobgy]

What does this arg do? If a new namespace is created can required resources get created right away?

[juliusvonkohout]

Yes the resources are created right away in my installation.

From the documentation https://metacontroller.github.io/metacontroller/guide/configuration.html?highlight=discovery-interval#command-line-flags

--discovery-interval | How often to refresh discovery cache to pick up newly-installed resources (e.g. --discovery-interval=10s). I think that means CRDs. A low value creates too much log spam and on start the cache is refreshed anyway. I am also fine with 10 minutes if that is what you want.

--zap-log-level | ZapGws  Level to configure the verbosity of logging. Can be one of ‘debug’,  ‘info’, ‘error’, or any integer value > 0 which corresponds to custom  debug levels of increasing verbosity(e.g. --zap-log-level=5).  Level 4 logs Metacontroller's interaction with the API server. Levels 5  and up additionally log details of Metacontroller's invocation of  lambda hooks. See the troubleshooting guide for more.

What you are worrying about is https://metacontroller.github.io/metacontroller/api/compositecontroller.html?highlight=discovery%20cache#resync-period but even that can be set to a high value in the compositecontroller "Sometimes you may want to sync periodically even if nothing has changed in the Kubernetes API objects"

[juliusvonkohout]

Is this detailed enough, or should i test something for you ?

[zijianjoy]

/approve

Thank you @juliusvonkohout

[juliusvonkohout]

/approve

Thank you @juliusvonkohout

Thank you too.

"To complete the pull request process, please ask for approval from bobgy after the PR has been reviewed." So i think @zijianjoy approval is not enough @Bobgy

[Bobgy]

/lgtm
/approve
Thank you for the great contribution!

[google-oss-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, zijianjoy

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:

• manifests/kustomize/OWNERS [Bobgy]

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

[sebastien-prudhomme]

@juliusvonkohout are you sure about the way "restricted the namespaces to kubeflow profile namespaces" is done?

It seems that here is no labelSelector field in parentResource field of CompositeController.

[juliusvonkohout]

@juliusvonkohout are you sure about the way "restricted the namespaces to kubeflow profile namespaces" is done?

It seems that here is no labelSelector field in parentResource field of CompositeController.

I am not sure anymore.
Please try to move the labelselector one level higher and report back whether it makes a difference.

According to https://metacontroller.github.io/metacontroller/api/compositecontroller.html#label-selector you might have to alter sync.py to to add the labels to the child resources "Children you create must have labels that satisfy the parent's selector, or else they will be immediately orphaned and you'll never see them again."

Please also test profile/namespace deletion and creation.

[sebastien-prudhomme]

@juliusvonkohout the error I got with this commit is with K8S itself which does'nt want to create the CompositeController resource because labelSelector is an unknown field.

As I'm not using a standard environment, can you just confirm it works on your side?

[juliusvonkohout]

Correction: it seems to be fine with the label selector. It is the same on my clusters. Please delete your cluster, use a proper kubernetes 1.19-1.21 and try again.

[sebastien-prudhomme]

@juliusvonkohout found the problem: kubeflow-pipelines is not using the official CRD for CompositeController and I'm using the official one which has not the preserveUnknownFields: true option.

[juliusvonkohout]

@sebastien-prudhomme then please create a new issue and pull request such that it is fixed long term for every one.