-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: support Envoy extra args #2489
Conversation
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
+ Coverage 64.65% 64.70% +0.05%
==========================================
Files 115 115
Lines 17472 17475 +3
==========================================
+ Hits 11296 11307 +11
+ Misses 5454 5447 -7
+ Partials 722 721 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
@@ -67,6 +67,12 @@ type EnvoyProxySpec struct { | |||
// +optional | |||
Concurrency *int32 `json:"concurrency,omitempty"` | |||
|
|||
// ExtraArgs defines additional command line options that are provided to Envoy. | |||
// More info: https://www.envoyproxy.io/docs/envoy/latest/operations/cli#command-line-options | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a warning message about duplicate arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add a generic message or specify all internal args that can't be used(--service-cluster
, --service-node
, --log-level
, --conponent-log-level
, --concurrency
, --config-yaml
, --cpuset-threads
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to not list all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be useful to tell users which args can/can't be configured through ExtraArgs
in the comment here. So they won't have to try and fail. And we can add validation here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
Should I also add a sample usage in the user guide? |
yes, that would be better, and you can do that with a follow up PR. |
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, defer to @arkodg
hey @shahar-h can you run |
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
done |
What this PR does / why we need it:
Add support for providing Envoy extra command line options via
EnvoyProxy
resource.Sample use case: disable some Envoy extension by providing --disable-extensions arg.
Which issue(s) this PR fixes:
Fixes #2479