Support namespace configuration when using dex with kfp

[akchinSTC]

Add user_namespace field to kfp metadata schema and
update kfp processor to allow submission of pipelines
to a dex enabled kfp cluster

Fixes #1053

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

[elyra-bot]

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link:

[lresende]

Could you please add the related issue to the description...

[lresende]

LGTM

[lresende]

@akchinSTC Should I wait for @reevejd feedback before merging?

[akchinSTC]

@lresende - yes lets wait until he responds. He will be testing first thing in the morning.

[kevin-bates]

These changes look good Alan - thanks.
(I'm assuming you confirmed that runtime_configuration.metadata['user_namespace'] is None when a user namespace value is not specified.)

[kevin-bates]

Thanks for the last update. I'm afk but will look into the metadata behavior tomorrow. I think we should be able to use something like .get('user_namespace') and should look why that isn't happening. I may also be misremembering things. 🙂

[kevin-bates]

I think the issue with user_namespace being the empty string when the intention is to not set is the front-end issue that they always set a value, in this case the empty string. I believe this is because they can't distinguish between null and the empty string in the UI. As a result, the user_namespace field will always have a "value" - and logic like Alan has provided will be necessary since that field is persisted with a value of an empty string.

Were the UI able to distinguish "set" vs. "not set", then they could simply not include user_namespace in the metadata when it is created in the first place resulting in the object's persistence w/o user_namespace.

In fact, we should probably add an existence check to see if the key (user_namespace) is present just in case this occurs. I believe the CLI tool could probably create such an instance (i.e., one where no user_namespace field is persisted).

[kevin-bates]

Per my last comment I recommend the following:

Suggested change

	if runtime_configuration.metadata['user_namespace']:
	if 'user_namespace' in runtime_configuration.metadata and runtime_configuration.metadata['user_namespace']:

[lresende]

or use get('user_namespace')

[akchinSTC]

@lresende - removing the if statement and using get('user_namespace') when using an empty user_namespace field in the runtime metadata returns the original error in the issue. Issue with using get atm is that it will never return None since the user_namespace key will always exist in the metadata dict, even when the field is empty.
@kevin-bates - Since the key will always exist, the extra check for the key in metadata is redundant. I think the current check should be enough where an empty string would be considered "falsy"

[kevin-bates]

I think once the front-end issue is resolved, the key won't exist when no value is present because it won't be persisted.

[kevin-bates]

This statement (directly from the docs) should produce an instance in which user_namespace is not persisted and is valid metadata:

elyra-metadata install runtimes --schema_name=kfp \
       --name=my_kfp \
       --display_name="My Kubeflow Pipeline Runtime" \
       --api_endpoint=https://kubernetes-service.ibm.com/pipeline \
       --api_username=username@email.com \
       --api_password=mypassword \
       --cos_endpoint=http://minio-service.kubeflow:9000 \
       --cos_username=minio \
       --cos_password=minio123 \
       --cos_bucket=test-bucket

[akchinSTC]

I think once the front-end issue is resolved, the key won't exist when no value is present because it won't be persisted.

Ive opened #1087 for this. Should we wait then until that issue is fixed and merged before merging this one?

[kevin-bates]

There's no need to revisit this if you add the field existence test now. This does mean a condition will be required before the .get() call.

[kevin-bates]

Also note that the condition can occur now via the CLI tool. You should be able to produce a KeyError if not setting a user_namespace value (and without adding the field existence test).

[reevejd]

@akchinSTC Should I wait for @reevejd feedback before merging?

I can confirm that @akchinSTC 's fix is working. Thanks a bunch!

[kevin-bates]

Excellent. Thank you for all your help with this James. Alan - great (and simple) fix! (The best kind there is!)