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

chore: Update indirect dependency to knative.dev/serving v0.37.5 #468

Merged
merged 10 commits into from
Jan 25, 2024

Conversation

ckadner
Copy link
Member

@ckadner ckadner commented Dec 30, 2023

Motivation

Address https://github.com/kserve/modelmesh-serving/security/dependabot/11

Modifications

Override/replace indirect dependency to knative.dev/serving v0.37.5


Problems with using `knative.dev/serving v0.39.0`

The Mitigation steps for the Security Vulnerability report suggest updating to knative.dev/serving v0.39.0.

However this forces an upgrade of several other direct/indirect dependencies causing incompatibilities:

  • with this upgrade, also k8s.io/api and k8s.io/client-go are upgraded to v0.27.+, but this causes compilation errors:
    # sigs.k8s.io/controller-runtime/pkg/cache
    /root/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/cache/multi_namespace_cache.go:308:9:
    cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration)
    as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement:
    map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration
    does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced)
    
  • now controller-runtime v0.15.0 onward supports HasSynced:
  • but we can't upgrade to controller-runtime v0.15.0 without breaking the KServe (0.11.2) API integration
    could not import github.com/kserve/kserve/pkg/apis/serving/v1alpha1 (-: # github.com/kserve/kserve/pkg/apis/serving/v1alpha1
    /root/go/pkg/mod/github.com/kserve/kserve@v0.11.2/pkg/apis/serving/v1alpha1/inference_graph_validation.go:61:27: cannot use &InferenceGraph{} (value of type *InferenceGraph) as admission.Validator value in variable declaration: *InferenceGraph does not implement admission.Validator (wrong type for method ValidateCreate)
                    have ValidateCreate() error
                    want ValidateCreate() (admission.Warnings, error)
    /root/go/pkg/mod/github.com/kserve/kserve@v0.11.2/pkg/apis/serving/v1alpha1/trainedmodel_webhook.go:51:27: cannot use &TrainedModel{} (value of type *TrainedModel) as admission.Validator value in variable declaration: *TrainedModel does not implement admission.Validator (wrong type for method ValidateCreate)
                    have ValidateCreate() error
                    want ValidateCreate() (admission.Warnings, error)) (typecheck)
            "github.com/kserve/kserve/pkg/apis/serving/v1alpha1"
    
  • so we need to pin "down" k8s.io/api and k8s.io/client-go below v0.27.0 until AddEventHandler return type error kubernetes-sigs/controller-runtime#2302 is addressed.
      k8s.io/api => k8s.io/api v0.26.4
      k8s.io/client-go => k8s.io/client-go v0.26.4
      k8s.io/code-generator => k8s.io/code-generator v0.26.4
    
  • that downgrade however would re-introduce another vulnerability we previously mitigated
      // Fixes github.com/elazarl/goproxy Denial of Service (DoS)
      // This dependency was removed from apimachinery 0.27.0
      // remove after upgrade to controller-runtime 0.15.x or apimachinery to 0.27.x
      k8s.io/apimachinery => k8s.io/apimachinery v0.27.0
    
  • once we upgrade to KServe v0.12.0 along with Go 1.21 we should be able to upgrade to controller-runtime
    to v0.15.0 and things should get a bit less messy

@spolti

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@ckadner ckadner self-assigned this Jan 10, 2024
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
@kserve kserve deleted a comment from oss-prow-bot bot Jan 24, 2024
@kserve kserve deleted a comment from oss-prow-bot bot Jan 24, 2024
@ckadner ckadner marked this pull request as ready for review January 24, 2024 22:23
@oss-prow-bot oss-prow-bot bot requested a review from joerunde January 24, 2024 22:23
@ckadner ckadner requested review from spolti and removed request for joerunde and rafvasq January 24, 2024 22:23
@ckadner ckadner requested a review from rafvasq January 24, 2024 23:40
@ckadner
Copy link
Member Author

ckadner commented Jan 25, 2024

Thanks @spolti -- took quite a bit of finagling to get this update to work. Should get easier once we moved to Go 1.21, KServe 0.12

go.mod Show resolved Hide resolved
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
@kserve kserve deleted a comment from oss-prow-bot bot Jan 25, 2024
go.mod Outdated Show resolved Hide resolved
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
@ckadner ckadner changed the title chore: Update indirect dependency to knative.dev/serving v0.39.0 chore: Update indirect dependency to knative.dev/serving v0.37.5 Jan 25, 2024
Copy link

oss-prow-bot bot commented Jan 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, spolti

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

@kserve kserve deleted a comment from oss-prow-bot bot Jan 25, 2024
@ckadner ckadner merged commit ded61b9 into kserve:main Jan 25, 2024
9 of 10 checks passed
@ckadner
Copy link
Member Author

ckadner commented Jan 25, 2024

Thanks @spolti for several rounds of reviews! 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants