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

Support for configuring envoy connection parameters in ingress, which istio currently supports. #873

Closed
wants to merge 3 commits into from

Conversation

shengyanli1982
Copy link

Changes

configuring envoy connection parameters from ingress annotations resource by user defined

/kind enhancement

Fixes #

Release Note

The current project only use default values envoy connection parameters and are not configured for connections to clusters created by ingress. now, can configuring envoy connection parameters from ingress annotations resource by user defined.

Unit Test

=== RUN   TestIngressTranslator
--- PASS: TestIngressTranslator (0.04s)
=== RUN   TestIngressTranslator/simple
    --- PASS: TestIngressTranslator/simple (0.01s)
=== RUN   TestIngressTranslator/tls
    --- PASS: TestIngressTranslator/tls (0.01s)
=== RUN   TestIngressTranslator/tls_redirect
    --- PASS: TestIngressTranslator/tls_redirect (0.01s)
=== RUN   TestIngressTranslator/tls_redirect_cluster_local
    --- PASS: TestIngressTranslator/tls_redirect_cluster_local (0.00s)
=== RUN   TestIngressTranslator/split
    --- PASS: TestIngressTranslator/split (0.01s)
=== RUN   TestIngressTranslator/path_defaulting
    --- PASS: TestIngressTranslator/path_defaulting (0.00s)
=== RUN   TestIngressTranslator/external_service
    --- PASS: TestIngressTranslator/external_service (0.00s)
=== RUN   TestIngressTranslator/external_service_without_service_port
    --- PASS: TestIngressTranslator/external_service_without_service_port (0.00s)
=== RUN   TestIngressTranslator/missing_service
{"level":"warn","ts":1657538878.371255,"logger":"fallback","caller":"generator/ingress_translator.go:193","msg":"Service 'servicens/servicename' not yet created"}
    --- PASS: TestIngressTranslator/missing_service (0.00s)
=== RUN   TestIngressTranslator/missing_endpoints
{"level":"warn","ts":1657538878.371395,"logger":"fallback","caller":"generator/ingress_translator.go:236","msg":"Endpoints 'servicens/servicename' not yet created"}
    --- PASS: TestIngressTranslator/missing_endpoints (0.00s)
PASS

Envoy Config Dump

"circuit_breakers": {
    "thresholds": [
     {
      "max_connections": 4294967295,
      "max_pending_requests": 4294967295,
      "max_requests": 4294967295,
      "max_retries": 10
     },
     {
      "priority": "HIGH",
      "max_connections": 4294967295,
      "max_pending_requests": 4294967295,
      "max_requests": 4294967295,
      "max_retries": 10
     }
    ]
}

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 11, 2022

CLA Missing ID CLA Not Signed

@knative-prow
Copy link

knative-prow bot commented Jul 11, 2022

Welcome @shengyanli1982! It looks like this is your first PR to knative-sandbox/net-kourier 🎉

@knative-prow knative-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2022
@knative-prow
Copy link

knative-prow bot commented Jul 11, 2022

Hi @shengyanli1982. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 11, 2022
@knative-prow
Copy link

knative-prow bot commented Jul 18, 2022

@shengyanli1982: The label(s) sig/apps cannot be applied, because the repository doesn't have them.

In response to this:

/sig apps

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@shengyanli1982
Copy link
Author

/assign @dprotaso

@shengyanli1982
Copy link
Author

/test all

@knative-prow
Copy link

knative-prow bot commented Jul 18, 2022

@shengyanli1982: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dprotaso
Copy link
Contributor

/assign @nak3
/unassign @dprotaso

Not familiar with kourier enough to review this

@knative-prow knative-prow bot assigned nak3 and unassigned dprotaso Jul 18, 2022
@dprotaso
Copy link
Contributor

/ok-to-test

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2022
if mc, ok := ingress.Annotations[envoyClusterMaxConnections]; ok {
if v, err := strconv.Atoi(mc); err == nil {
if v > 0 && v < math.MaxUint32 {
connectionOpts.MaxConnections = uint32(v)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint32 without an upper bound check.
if mpr, ok := ingress.Annotations[envoyClusterMaxPendingRequests]; ok {
if v, err := strconv.Atoi(mpr); err == nil {
if v > 0 && v < math.MaxUint32 {
connectionOpts.MaxPendingRequests = uint32(v)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint32 without an upper bound check.
if mr, ok := ingress.Annotations[envoyClusterMaxRequests]; ok {
if v, err := strconv.Atoi(mr); err == nil {
if v > 0 && v < math.MaxUint32 {
connectionOpts.MaxRequests = uint32(v)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint32 without an upper bound check.
if mrt, ok := ingress.Annotations[envoyClusterMaxRetries]; ok {
if v, err := strconv.Atoi(mrt); err == nil {
if v > 0 && v < math.MaxUint32 {
connectionOpts.MaxRetries = uint32(v)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of an integer with architecture-dependent bit size from [strconv.Atoi](1) to a lower bit size type uint32 without an upper bound check.
@nak3
Copy link
Contributor

nak3 commented Jul 19, 2022

Thank you @shengyanli1982 for this PR.

I have read your draft of the release note:

The current project only use default values envoy connection parameters and are not configured for connections to clusters created by ingress. now, can configuring envoy connection parameters from ingress annotations resource by user defined.

Could you please describe what exact problems did you encounter with the current values?

@shengyanli1982
Copy link
Author

Could you please describe what exact problems did you encounter with the current values?

Of course. We used Net-Kourier + Kourier as the gateway for our backend knatvie applications. In the real world, we have a service with the concurrency mode between 10 and 60 pods. We are an e-commerce business, so the peak of requests will often be like a wave, and the enovy default of 1000 will often be stuck to our peak requests.

Our internal version has been modified and confirmed to be able to effectively support online services.

@knative-prow
Copy link

knative-prow bot commented Jul 25, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shengyanli1982
To complete the pull request process, please ask for approval from nak3 after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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

@shengyanli1982
Copy link
Author

/retest-required

@shengyanli1982
Copy link
Author

/assign @nak3

@shengyanli1982
Copy link
Author

/retest-required

@shengyanli1982
Copy link
Author

/retest

@nak3
Copy link
Contributor

nak3 commented Aug 3, 2022

Thank you for the explanation and sorry for the late response.
Could you please clean up the PR? I think there are some unexpected change in the PR.

Also, if possible I would like to avoid the annotation approach. Did you consider any alternative idea to support this without the annotation?

@shengyanli1982
Copy link
Author

/skip

@shengyanli1982
Copy link
Author

shengyanli1982 commented Aug 7, 2022

emmmm. Ingress forwards traffic for a service. In this case, ingress carries the service attribute. Ingress is also a CRD resource, so these contents naturally have a clear mapping relationship. Annotation is where I can think of the most efficient way to extend custom attributes, maybe you have a better idea.

In the real world, we use Labels to distinguish different services and business types, Annotation as an application public metadata attribute definition, and Env to define variables within different versions of POD. Clear data definition layering facilitates large-scale POD operation and management.

The above content is for your reference only.

@shengyanli1982
Copy link
Author

/assign @nak3

@github-actions
Copy link

github-actions bot commented Nov 6, 2022

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 6, 2022
@mbrancato
Copy link

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2022
@mbrancato
Copy link

@nak3 what would be needed to get this to a state where it can be merged? If this is no longer being worked on, I could takeover this and get it to a mergable state. If you need more info on the use-case, I too have an issue linked below that this addresses.

Fixes #839

@knative-prow-robot
Copy link

@shengyanli1982: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2023
@skonto
Copy link
Contributor

skonto commented Mar 22, 2023

@shengyanli1982 could rebase the PR? gentle ping.

@skonto
Copy link
Contributor

skonto commented Apr 27, 2023

@shengyanli1982 could rebase the PR? gentle ping.

@skonto
Copy link
Contributor

skonto commented Apr 27, 2023

@mbrancato could you help on this one?

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2023
@github-actions github-actions bot closed this Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants