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

Add support for pulling the image secret #88

Merged
merged 5 commits into from
Apr 10, 2020

Conversation

kevinyu98
Copy link
Contributor

Which issue is resolved by this Pull Request:

Related #54
Description of your changes:
We need image pull secret so that we can pull images from private repo. kfp-tekton compiler will generated Tekton pipeline based on the kubeflow pipeline DSL, if the DSL contains image secret, the compiler will generate a service account, and store the secret under the service account. A better approach will be adding the image pull secret into a podtemplate, but currently Tekton doesn't have this support. We opened an issue 2339 there.
Environment tested:

  • Python Version (use python --version):
    Python 3.7.4
  • Tekton Version (use tkn version):
    Client version: 0.8.0
    Pipeline version: v0.11.0-rc2
  • Kubernetes Version (use kubectl version):

Openshift Client Version: v4.3.1
Kubernetes Version: v1.16.2

  • OS (e.g. from /etc/os-release):
    macOS Mojave 10.14.6

@kubeflow-bot
Copy link

This change is Reviewable

@kevinyu98
Copy link
Contributor Author

@Tomcli

@@ -0,0 +1,59 @@
# Copyright 2018 Google LLC
Copy link
Member

@fenglixa fenglixa Apr 9, 2020

Choose a reason for hiding this comment

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

# Copyright 2020 kubeflow.org

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, done

'metadata': {'name': 'secrets'}
}
service_template['imagePullSecrets'] = []
image_pull_secrets = []
Copy link
Member

Choose a reason for hiding this comment

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

should no need this line, same as service_template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove image_pull_secrets = [], can't directly use image_pull_secrets to do the append, it complains variable not defined. I removed service_template.

dsl.get_pipeline_conf()\
.set_image_pull_secrets([k8s_client.V1ObjectReference(name="secretA")])

if __name__ == '__main__':
Copy link
Member

Choose a reason for hiding this comment

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

Remove kfp.compiler, use TektonCompiler instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks.

@@ -0,0 +1,58 @@
apiVersion: tekton.dev/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

Add license header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@fenglixa
Copy link
Member

fenglixa commented Apr 9, 2020

Thanks @kevinyu98 , please address the comments.

@fenglixa
Copy link
Member

fenglixa commented Apr 9, 2020

/lgtm

/assign @animeshsingh

@fenglixa
Copy link
Member

fenglixa commented Apr 9, 2020

/hold

@fenglixa
Copy link
Member

fenglixa commented Apr 9, 2020

@kevinyu98 , seems you didn't add testing in compiler_tests.py, please add it there. Thanks.

@fenglixa
Copy link
Member

fenglixa commented Apr 9, 2020

/lgtm cancel

/unhold

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

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

We also need to add the missing compiler_tests.py test as Feng pointed out.

}
image_pull_secrets = []
for image_pull_secret in pipeline_conf.image_pull_secrets:
image_pull_secrets.append(convert_k8s_obj_to_json(image_pull_secret))
Copy link
Member

Choose a reason for hiding this comment

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

It looks like image_pull_secrets is not being used for service_template, maybe you can remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@kevinyu98 kevinyu98 force-pushed the kfp-tekton-pull-secrets branch from 6c15eb2 to 42bfac9 Compare April 10, 2020 00:09
@Tomcli
Copy link
Member

Tomcli commented Apr 10, 2020

/lgtm
We probably still want to keep track of #2339 if the Tekton community do agree moving it to podTemplate.

@kevinyu98
Copy link
Contributor Author

sure.

service_template = {
'apiVersion': 'v1',
'kind': 'ServiceAccount',
'metadata': {'name': 'secrets'}
Copy link
Member

Choose a reason for hiding this comment

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

Actually can we make the generated service account to be based on the pipelinerun name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@Tomcli
Copy link
Member

Tomcli commented Apr 10, 2020

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 10, 2020
@Tomcli
Copy link
Member

Tomcli commented Apr 10, 2020

@kevinyu98 you also need to update your test yaml.

@kevinyu98
Copy link
Contributor Author

@Tomcli done.

@Tomcli
Copy link
Member

Tomcli commented Apr 10, 2020

/lgtm

@animeshsingh
Copy link
Collaborator

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: animeshsingh

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 cf8e998 into kubeflow:master Apr 10, 2020
HumairAK referenced this pull request in red-hat-data-services/data-science-pipelines-tekton May 16, 2023
Add/Enable Customizable Sample Pipeline to APIServer
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.

6 participants