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

feat: Add application and common labels to KFP and various fixes #1374

Merged
merged 14 commits into from
Jul 13, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Jul 10, 2020

Description of your changes:
Meets Kubeflow application requirement: https://github.com/kubeflow/community/blob/master/guidelines/application_requirements.md

There were also a few small fixes in this PR:

  • kfp profile controller, using deployment v1 api instead, because v1beta1 is deprecated after K8s 1.14
  • change userid-header=x-goog-authenticated-user-email to lowercase
  • fix kfp profile controller b64encoding exception

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

@kubeflow-bot
Copy link
Contributor

This change is Reviewable

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 10, 2020

/assign @jlewi

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 10, 2020

The test failure is

=== RUN   TestCommonLabelsImmutable
   TestCommonLabelsImmutable: validate_resources_test.go:62: Skipping directory ../.git
   TestCommonLabelsImmutable: validate_resources_test.go:62: Skipping directory ../.github
   TestCommonLabelsImmutable: validate_resources_test.go:88: ../pipeline/installs/generic/kustomization.yaml has forbidden commonLabel app.kubernetes.io/part-of
   TestCommonLabelsImmutable: validate_resources_test.go:88: ../pipeline/installs/multi-user/kustomization.yaml has forbidden commonLabel app.kubernetes.io/part-of
   TestCommonLabelsImmutable: validate_resources_test.go:62: Skipping directory ../tests

We haven't published pipeline installation publically, can we bypass the test failure and merge it?

@jlewi
Copy link
Contributor

jlewi commented Jul 10, 2020

@Bobgy What does it not being public have to do with anything. Its the part-of label that is problematic. Can you just remove it?

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 10, 2020

May I ask why part of is forbidden?
Which should I use instead?

@jlewi
Copy link
Contributor

jlewi commented Jul 10, 2020

kustomize applies commonLabels to all selectors. As a result they need to be immutable across versions otherwise "apply" will fail because it will try to change selector labels.

This was in part because of how part-of labels were being used in previous editions of Kubeflow; i.e. part-of was referring to a string identifying the Kubeflow version.

You have a couple options

  1. Set part-of labels but don't use commonLabels to do it so they don't get applied to selectors
  2. Add an exception to the test (
    forbiddenLabels := []string{VersionLabel, ManagedByLabel, InstanceLabel, PartOfLabel}
    ) to allow part-of labels for KFP
  3. Just remove part-of from commonLabels

Unless you have a strong reason for wanting the part-of label to be set; its probably just easiest to remove it for now.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 11, 2020

OK, I will use the name label then. Thanks

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 13, 2020

/assign @jlewi
Can you take a look again?

@Bobgy
Copy link
Contributor Author

Bobgy commented Jul 13, 2020

@jlewi I changed userid-header=x-goog-authenticated-user-email in params.env to lowercase. KFP multi user mode seems to require it to be all lowercase (I tested the original config, but KFP api server fails to find user headers). Is this correct? Do others tolerate casing?

@Bobgy Bobgy changed the title feat: Add application and common labels to KFP feat: Add application and common labels to KFP and various fixes Jul 13, 2020
@jlewi
Copy link
Contributor

jlewi commented Jul 13, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@k8s-ci-robot k8s-ci-robot merged commit 5cc6fd3 into kubeflow:master Jul 13, 2020
Bobgy added a commit to Bobgy/manifests that referenced this pull request Jul 14, 2020
…eflow#1374)

* Add common labels to kfp components

* Add KFP application

* update snapshot

* Use json format for json patch, because yaml will look like a resource and fail tests

* Remove part of label

* update snapshots

* Fix profile controller deployment version

* update snapshot

* Fix userid-header for gcp

* update snapshot

* Fix b64encode exception

* update snapshot

* update snapshot
k8s-ci-robot pushed a commit that referenced this pull request Jul 14, 2020
* feat: KFP multi user mode PR1 - enable multi user mode without istio authorization (#1342)

* Add argo to stacks/generic

* Pull pipelines manifest from upstream

* Updated kfp

* Minio v3 manifests

* Rename minio configmap

* Add generic minio install

* Generate new test data

* Mysql kustomize v3 manifest - generic install

* Add mysql gcp pd install

* Generate test data

* Pipelines kustomize v3 manifests

* Add kfp ui virtual service

* Add metadata deployment to stacks/generic

* Use common cluster domain

* Deploy metadata writer

* Add kfp cache server

* Update test data

* Enable KFP multi user mode without istio security

* Fix persistence agent watch namespace

* Fix namespace env for some deployments

* Fix cluster roles and bindings

* fix rename

* Fix pipelines ui role

* Updated kfp to rc2

* simplify pipeline v3 manifest using updated kfp rc2 manifest

* Fix pipeline-install-config

* remove redundant configmap

* update tests

* updated to kfp 1.0.0-rc.3

* Adapt to kfp 1.0rc3 refactoring

* update test snapshots

* fix pull kfp script to detect empty dir

* fix example ref

* update snapshot

* fix gcp pd manifest

* Update stacks ref

* revert alice example to gcp stack

* update snapshot

* fix profile controller iam binding

* Update kfp profile controller can be configured to different images and
istio sidecar

* add missing viewer controller cluster roles

* Use python3 for sync.py

* Revert gcp stack back to use non multi user kfp

* revert unintended changes

* revert upstream changes

* Use kubeflow userid header and prefix config for KFP servers (#1365)

* feat: KFP multi user mode PR2 - secure KFP with istio mTLS and authz (#1368)

* Add argo to stacks/generic

* Pull pipelines manifest from upstream

* Updated kfp

* Minio v3 manifests

* Rename minio configmap

* Add generic minio install

* Generate new test data

* Mysql kustomize v3 manifest - generic install

* Add mysql gcp pd install

* Generate test data

* Pipelines kustomize v3 manifests

* Add kfp ui virtual service

* Add metadata deployment to stacks/generic

* Use common cluster domain

* Deploy metadata writer

* Add kfp cache server

* Update test data

* Enable KFP multi user mode without istio security

* Fix persistence agent watch namespace

* Fix namespace env for some deployments

* Fix cluster roles and bindings

* fix rename

* Fix pipelines ui role

* Updated kfp to rc2

* simplify pipeline v3 manifest using updated kfp rc2 manifest

* Fix pipeline-install-config

* remove redundant configmap

* update tests

* updated to kfp 1.0.0-rc.3

* Adapt to kfp 1.0rc3 refactoring

* update test snapshots

* fix pull kfp script to detect empty dir

* fix example ref

* update snapshot

* fix gcp pd manifest

* Update stacks ref

* revert alice example to gcp stack

* update snapshot

* fix profile controller iam binding

* Update kfp profile controller can be configured to different images and
istio sidecar

* add missing viewer controller cluster roles

* Use python3 for sync.py

* Revert gcp stack back to use non multi user kfp

* revert unintended changes

* revert upstream changes

* Secure kfp multi user mode with istio authorization

* patch minio to disable istio sidecar injection

* fix cache server istio authz

* enable istio sidecar for profiles deploy

* enable istio sidecar for centraldashboard

* Do not protect profile controller with istio

* Allow admission webhook traffic to cache-server

* revert gcp stack back to pipeline generic

* Reuse minio generic install as base for gcp-pd and ibm

* update snapshot

* refactor: pipelines profile controller should get minio access keys from the secret (#1372)

* refactor: pipelines profile controller should get minio access keys from the secret

* do not print secrets in log

* feat: Use KFP multi user mode for GCP (#1373)

* refactor: pipelines profile controller should get minio access keys from the secret

* do not print secrets in log

* use kfp multi user mode for gcp stacks

* update snapshot

* feat: Add application and common labels to KFP and various fixes (#1374)

* Add common labels to kfp components

* Add KFP application

* update snapshot

* Use json format for json patch, because yaml will look like a resource and fail tests

* Remove part of label

* update snapshots

* Fix profile controller deployment version

* update snapshot

* Fix userid-header for gcp

* update snapshot

* Fix b64encode exception

* update snapshot

* update snapshot
@Bobgy Bobgy deleted the kfp_labels_and_application branch July 20, 2020 04:29
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.

5 participants