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.

Sign up for GitHub

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

Jump to bottom

Request to deprecate compiler.build_python_component(build_image=,..) flag #1661

Closed
SinaChavoshi opened this issue Jul 24, 2019 · 6 comments · Fixed by #1657
Closed

Request to deprecate compiler.build_python_component(build_image=,..) flag #1661

SinaChavoshi opened this issue Jul 24, 2019 · 6 comments · Fixed by #1657
Assignees
@Ark-kun
Labels
kind/bug priority/p0

Comments

@SinaChavoshi
Copy link
Contributor

This is a request to eliminate redundant methods of building docker / components. specifically to remove compiler.build_python_component(build_image=,..) flag. This flag eliminates the container build operation from build_python_component however it will not inject the python function into the container or pass it on. This allows for modification of component definition without changing the base container image. However this is redundant as dsl.ContainerOp provides the same functionality.

This will ensure that

  1. methods of building docker and component are kept update as there are fewer of them to maintain.
  2. The path for building components and docker is clear ( less confusing ) by reducing the multiple ways that the same code can be written.
@hongye-sun
Copy link
Contributor

@gaoning777, will it be covered by your component builder work?

@Ark-kun Ark-kun mentioned this issue Jul 24, 2019
Merged
@Ark-kun Ark-kun self-assigned this Jul 24, 2019
@gaoning777
Copy link
Contributor

This flag was added initially to avoid rebuilding the image if the image already exists. E.g. when users has run the notebook once and need to run it again without building the image, setting the flag will only generate the containerop.

Since we will have a fast build that takes very little time, the new version will not have this configuration any more. I will cover that.
@Ark-kun, if you do not mind, I will address this issue in my efforts for the new component build.

@Ark-kun
Copy link
Contributor

Ark-kun commented Jul 24, 2019

setting the flag will only generate the containerop

But that ContainerOp might be in compatible with the container (e.g. when the function has changed).
Can the user just save component to a file (build_python_component(target_component_file='my_component.yaml', ...)) once and then call load_component_from_file('my_component.yaml') in subsequent runs?

@Ark-kun, if you do not mind, I will address this issue in my efforts for the new component build.

Sure.

@Ark-kun
Copy link
Contributor

Ark-kun commented Jul 29, 2019

Waiting for @gaoning777 's refactoring to complete.

@gaoning777
Copy link
Contributor

Completed. Go ahead.

@Ark-kun
Copy link
Contributor

Ark-kun commented Aug 6, 2019

@gaoning777 Can you please LGTM #1657 ?

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this issue Oct 22, 2023
@elukey @haijohn
Improvements to self-signed-ca.sh (kubeflow#1661)
9995b0e 
* Add SANs to server.crt

On my testing environment (debian 11, openssl 1.1.1k, minikube with k8s 1.20,
go 1.15, kfserving 0.5) I get some TLS errors when testing kfserving,
namely
"x509: certificate relies on legacy Common Name field, use SANs or temporarily enable Common Name matching"
when trying to deploy a simple InferenceService resource.

After some digging I found this problem:

```
openssl req -noout -text -in server.csr | grep "Subject Alternative Name" -A 1
            X509v3 Subject Alternative Name:
                DNS:kfserving-webhook-server-service, DNS:kfserving-webhook-server-service.kfserving-system, DNS:kfserving-webhook-server-service.kfserving-system.svc, DNS:kfserving-webhook-server-service.kfserving-system.svc.cluster, DNS:kfserving-webhook-server-service.kfserving-system.svc.cluster.local

openssl x509 -in server.crt -text -noout | grep "Subject Alternative Name" -A 1
```

Meanwhile if I add this patch:

```
openssl x509 -in server.crt -text -noout | grep "Subject Alternative Name" -A 1
            X509v3 Subject Alternative Name:
                DNS:kfserving-webhook-server-service, DNS:kfserving-webhook-server-service.kfserving-system, DNS:kfserving-webhook-server-service.kfserving-system.svc, DNS:kfserving-webhook-server-service.kfserving-system.svc.cluster, DNS:kfserving-webhook-server-service.kfserving-system.svc.cluster.local

```

As far as I got go 1.15 introduces a stricter policy for TLS certs, and
openssl by default doesn't copy SANs from a .csr to .crt if not
explicitly told.

gh: kubeflow#1657

* Add patch for conversion webhook in self-signed-ca.sh

Co-authored-by: haijohn <haijohnzju@gmail.com>
gh: kubeflow#1657

Co-authored-by: haijohn <haijohnzju@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Ark-kun Ark-kun

Labels
kind/bug priority/p0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

SDK - Removed the build_image parameter from build_python_component function Ark-kun/pipelines
5 participants
@Ark-kun @gaoning777 @SinaChavoshi @jessiezcc @hongye-sun