Mixer panics if attribute is absent for strongly typed field in template

[manlinl]

For example message Template { string api_operation = 1; }

In instance config

Spec: api_operation = api.operation | ""

Mixer panic saying converting nil interface{} as string if api.operation is absent in request (via mixc tool). The expected behavior is if api.operation isn't in attribute bag, empty string is assigned to api_operation field.

[manlinl]

This may be related to issues:

https://github.com/istio/mixer/issues/1220 https://github.com/istio/mixer/issues/1216

[guptasu]

Thanks for reporting. I can take a look into this. @manlinl Do you happen to have a call stack ?

[mandarjog]

@manlinl Can you place the call stack here?

[manlinl]

Sorry, I exit browser before saving my response. Here is the backtrace.

(gdb) bt
#0  runtime.gopanic (e=...)
    at /usr/local/google/home/manlinl/.cache/bazel/_bazel_manlinl/08e301ee4ed94ae96bc791a94af69d42/external/go_sdk/src/runtime/panic.go:414
#1  runtime.panicdottypeE (have=, want=, iface=)
    at /usr/local/google/home/manlinl/.cache/bazel/_bazel_manlinl/08e301ee4ed94ae96bc791a94af69d42/external/go_sdk/src/runtime/iface.go:172
Python Exception <class 'gdb.error'> Attempt to take contents of a non-pointer value.: 
#2  istio.io/mixer/template.glob..func40 (ctx=..., 
    insts=map[string]github.com/gogo/protobuf/proto.Message, attrs=..., 
    mapper=..., handler=..., ~r5=...)
    at bazel-out/local-dbg/genfiles/template/template.gen.go:949
#3  istio.io/mixer/pkg/runtime.(*dispatcher).Report.func1.1 (ctx=..., ~r1=)
    at pkg/runtime/dispatcher.go:191
#4  istio.io/mixer/pkg/runtime.safeDispatch (ctx=..., do=
    {void (context.Context, struct istio.io/mixer/pkg/runtime.result **)} , 
    op="svcctrlreport:testhandler.svcctrl.istio-system(svcctrl)", res=)
    at pkg/runtime/dispatcher.go:370
#5  istio.io/mixer/pkg/runtime.(*dispatcher).runAsync.func1 ()
    at pkg/runtime/dispatcher.go:390
#6  istio.io/mixer/pkg/pool.(*GoroutinePool).AddWorkers.func1 (gp=)
    at pkg/pool/goroutine.go:66

[manlinl]

BTW, I have live repro if debug is needed.

[guptasu]

Looking into it now

[mandarjog]

@guptasu IL is not fully compliant with all the tests I think. pkg/expr has eval_tests that should be run against IL. I don’t think IL tests cover the equivalent functions.

[guptasu]

@mandarjog

So far based on my investigation, the Eval code return nil when expression is something like absent.attr | "" and therefore generated code panics when it tries to cast the evaluated value into string. We can change the generated code to use EvalString (Which itself currently panics in this case, but that can be easily fixed to handler such case) and prevent panic. However, in the scenario when the datatype of the field is ValueType, we will end up passing nil to the adapter as oppose to empty string, which is bad. So the right fix in my opinion is to find a way such that Eval method returns empty string and not nil if the evaluated expression's value is empty string.

Please refer to code here that make Eval return nil.

[douglas-reid]

@guptasu see also: https://github.com/istio/mixer/issues/934

[guptasu]

I see, https://github.com/istio/mixer/issues/934 is now worsen because of panic from generated code. Fixing the panic from generated code is trivial, but ideal solution is to fix #934, imo

[guptasu]

Assigning to @ozevren since his change of replacing nil as empty str "" fixes this issue.

[ozevren]

Fixed by https://github.com/istio/istio/pull/1322

[guptasu]

@ozevren let's close this once your other PR that adds the test is also submitted: This particular piece of code : https://github.com/istio/istio/pull/1588/files#diff-2a580837695f112651fc7e7ffad9c3c3R799

I also don't have permissions but we can mark is as closed or ask Martin to close it.

[manlinl]

Close, since fix is in. @guptasu @ozevren