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

PSS labels for the profile controller #2778

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

biswajit-9776
Copy link
Contributor

Pull Request Template for Kubeflow manifests Issues

  • Please include a summary of changes and the related issue.
  • List any dependencies that are required for this change.
  • Please delete the options that are not relevant.
  • The following checklist will help you to satisfy the requirements.

✏️ A brief description of the changes

I patched PSS labels for namespaces in contrib/dynamic kustomize component

📦 List any dependencies that are required for this change

My PR depends on #

🐛 If this PR is related to an issue, please put the link of the issue here.

The following issues are related, because ...

✅ Unit Test Checklist

  • 🛠️ Make sure you have installed kustomize == 5.2.1+
  • ✍️ Have you written new tests for your core changes, as applicable?
  • 🔄 Have you successfully run existing tests with your changes ?
  • 🚀 Have you successfully run existing and new tests with your changes ?

✅ Contributor checklist


You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Jun 30, 2024

@biswajit-9776 where is profiles-system coming from? The patch should touch only the in the Kubeflow namespace at build time and you need to keep the existing labels https://github.com/kubeflow/manifests/blob/60348d70f1614d1661d365ae868c844f7bca5e3d/apps/profiles/upstream/base/namespace-labels.yaml#L15C1-L18C46 .

When the cluster is running and new namespaces are generated dynamically, the labels from this file will be applied to each.

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Jun 30, 2024
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Jun 30, 2024

No, it works differently. Please check out https://github.com/kubeflow/manifests/blob/60348d70f1614d1661d365ae868c844f7bca5e3d/apps/profiles/upstream/base/kustomization.yaml#L17C1-L20C26 to understand how the configuration map is created for the profile controller. You have to patch the config map for the dynamic stuff. At runtime the profile controller will use this configmap to create namespace of yet unknown names based on Profile CRs. The static patch for the kubeflow namespace is something completely different and belongs to the static folder.

@biswajit-9776
Copy link
Contributor Author

Okay I will push a commit later to this PR

@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Jun 30, 2024

Okay so if our kustomize component is going to patch the https://github.com/kubeflow/manifests/blob/60348d70f1614d1661d365ae868c844f7bca5e3d/apps/profiles/upstream/base/namespace-labels.yaml with out PSS labels, can we do it by providing the path as resources to our kustomize component?

apiVersion: kustomize.config.k8s.io/v1alpha1
kind: Component

resources: ../../../../../apps/profiles/upstream/base/namespace-labels.yaml
patches:
- path: patches/profiles-labels.yaml

If not this then may I know which namespace the configmap namespace-labels-data is mounted to, in which case I can just patch the configmap with PSS labels to that namespace?

@juliusvonkohout
Copy link
Member

Always consider that gpt4 or Gemini can guide you a bit on such general ideas ;-)

Yes just try it and the profile controller and therefore the configmap is in the kubeflow namespace.

@juliusvonkohout juliusvonkohout changed the title Patched PSS labels with profiles Patched PSS labels for the profile controller Jul 1, 2024
@juliusvonkohout juliusvonkohout changed the title Patched PSS labels for the profile controller PSS labels for the profile controller Jul 1, 2024
@juliusvonkohout juliusvonkohout self-assigned this Jul 2, 2024
@biswajit-9776
Copy link
Contributor Author

Yeah it works now, but I'd also like to point out a few things that occurs with the above changes

@biswajit-9776
Copy link
Contributor Author

So, we have to use the below change in the base kustomization:

options:
  disableNameSuffixHash: true

I tried without it and as a result the configMap instead of getting overridden, still exists along with a new configMap with our changes.

@biswajit-9776
Copy link
Contributor Author

After making the above change, I tried commenting the unrequired section of namespacea-labels.yam and after applying our component, I discovered this:

Name:         namespace-labels-data
Namespace:    kubeflow
Labels:       kustomize.component=profiles
Annotations:  <none>

Data
====
namespace-labels.yaml:
----

# # Below is a list of labels to be set by default.
# #
# # To add a namespace label, use `key: 'value'`, for example:
# # istio.io/rev: 'asm-191-1'
# #
# # To remove a namespace label, use `key: ''`. For example:
# # istio-injection: ''
# #
# # Profile controller will not replace a namespace label if its key already
# # exists. If you want to override the value of a previously applied label, you
# # need to:
# # 1. Remove the label by using `key: ''` and deploy.
# # 2. Add the label by using `key: 'value'` and deploy.

# katib.kubeflow.org/metrics-collector-injection: "enabled"
# serving.kubeflow.org/inferenceservice: "enabled"
# pipelines.kubeflow.org/enabled: "true"
# app.kubernetes.io/part-of: "kubeflow-profile"
pod-security.kubernetes.io/enforce: "baseline"

what I believe is that instead of adding the uncommented stuff, it replaced the everything else with the new stuff we tried to add. To deal with this temporarily, I wish to have an exact copy of base namespace-label.yaml in my component's directory along with PSS labels.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Jul 2, 2024

After making the above change, I tried commenting the unrequired section of namespacea-labels.yam and after applying our component, I discovered this:

Name:         namespace-labels-data
Namespace:    kubeflow
Labels:       kustomize.component=profiles
Annotations:  <none>

Data
====
namespace-labels.yaml:
----

# # Below is a list of labels to be set by default.
# #
# # To add a namespace label, use `key: 'value'`, for example:
# # istio.io/rev: 'asm-191-1'
# #
# # To remove a namespace label, use `key: ''`. For example:
# # istio-injection: ''
# #
# # Profile controller will not replace a namespace label if its key already
# # exists. If you want to override the value of a previously applied label, you
# # need to:
# # 1. Remove the label by using `key: ''` and deploy.
# # 2. Add the label by using `key: 'value'` and deploy.

# katib.kubeflow.org/metrics-collector-injection: "enabled"
# serving.kubeflow.org/inferenceservice: "enabled"
# pipelines.kubeflow.org/enabled: "true"
# app.kubernetes.io/part-of: "kubeflow-profile"
pod-security.kubernetes.io/enforce: "baseline"

what I believe is that instead of adding the uncommented stuff, it replaced the everything else with the new stuff we tried to add. To deal with this temporarily, I wish to have an exact copy of base namespace-label.yaml in my component's directory along with PSS labels.

Please investigate the officially documented behavior of merge in the configmapgenerator. We need to be sure.
I am strongly opposed to duplicating stuff and increasing the maintenance burden.

Signed-off-by: biswajit-9776 <[email protected]>
Signed-off-by: biswajit-9776 <[email protected]>
@juliusvonkohout
Copy link
Member

/hold
/approve
/lgtm

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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:

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

@biswajit-9776
Copy link
Contributor Author

biswajit-9776 commented Jul 23, 2024

Should I rebase my branch locally and push it?

@juliusvonkohout
Copy link
Member

/unhold

@google-oss-prow google-oss-prow bot merged commit ace875b into kubeflow:master Jul 23, 2024
6 checks passed
@juliusvonkohout juliusvonkohout linked an issue Aug 12, 2024 that may be closed by this pull request
pschoen-itsc pushed a commit to pschoen-itsc/kf-manifests that referenced this pull request Sep 3, 2024
* Patched PSS labels with profiles

Signed-off-by: biswajit-9776 <[email protected]>

* Patched profiles/upstream/base/namespace-labels.yaml and updated example.yaml

Signed-off-by: biswajit-9776 <[email protected]>

* Tried to override the base configMap

Signed-off-by: biswajit-9776 <[email protected]>

* Made changes for overriding base configMap with kustomize component

Signed-off-by: biswajit-9776 <[email protected]>

* Undone changes to profiles/upstream/base

Signed-off-by: biswajit-9776 <[email protected]>

* Added comments for duplicated file

Signed-off-by: biswajit-9776 <[email protected]>

* Resolved conflict

Signed-off-by: biswajit-9776 <[email protected]>

* Fixed yaml lint to example

Signed-off-by: biswajit-9776 <[email protected]>

---------

Signed-off-by: biswajit-9776 <[email protected]>
Signed-off-by: Patrick Schönthaler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rootless Kubeflow
2 participants