-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: log level present in extra args using envoy-extra-args annotation : NET-2190 #133
fix: log level present in extra args using envoy-extra-args annotation : NET-2190 #133
Conversation
LGTM on a high level. I'd defer the approval to someone who has more context around this |
@absolutelightning I just merged #115 which refactored the Envoy |
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.
Looks pretty good. As Mike said in another comment, you will need to rebase off of main and change several things as proxy config changed.
@@ -146,3 +146,129 @@ func testOutputPath() string { | |||
fmt.Sprintf("test-output-%x.json", time.Now().UnixNano()+int64(os.Getpid())), | |||
) | |||
} | |||
|
|||
func TestProxy_OverridingLoggerAndExtraArgs(t *testing.T) { |
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.
Do these tests run for you locally? They fail for me with failed to unmarshal output file: invalid character '\r' in string literal
(Also, CI is not running the unit tests)
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.
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 figured out what the problem was for me. I had the brew install base64
instead of the GNU brew install gbase64
. This was causing fake-envoy
to not run at all....
To save you time in the future, most of use Kind for local testing. It runs locally and it is very nice to be able to delete and recreate your cluster very quickly. I barely ever use EKS/GKE/AKS in my testing unless I am specifically testing a feature needed on those Kubernetes versions. Here is my demo repo and a Makefile that I use for quickly setting things up. Copy and replace with your directories/dockerhub. I sometimes run |
711f7f3
to
65ba8a3
Compare
Thanks @curtbushko . Really helpful. |
Also I have rebased my branch. |
I built my own version of the consul-dataplane image and I am running the acceptance tests against it here |
PR fixes - hashicorp/consul-k8s#1846
Short description of Issue - The consul.hashicorp.com/envoy-extra-args annotation is not compatible with Consul DataPlane. When we pass extra
loglevel
in consul.hashicorp.com/envoy-extra-args pod shows that the consul-dataplane container already has log-level explicitly set to INFO and service errors out.Solution -
Discussed and agreed upon solution is to override the log level in case extra args also has log level given.
Tested this PR by creating a docker image for consul dataplane, pushing it hub and using the image in this tutorial
I have tested by using my image in tutorial -
imageConsulDataplane: "absolutelightning/consul-dataplane:latest"
Here are the tests results -
Logs of consul data plane -