Authn Filter Config for Beta Authn Policy#19422
Conversation
|
I went through the fields in the filter config and policy. Haven't done a e2e test, but I think its ready for review, excepting adding more test cases. @diemtvu |
| PrincipalBinding: authn_alpha.PrincipalBinding_USE_ORIGIN, | ||
| } | ||
| // TODO: does it matter if this is empty but not nil? | ||
| jwtLocation := map[string]string{} |
There was a problem hiding this comment.
I think this can be deprecated now. It was used to find the payload, but since we moved to dynamic metadata, the field was obsolete and unused.
| } | ||
| // TODO: does it matter if this is empty but not nil? | ||
| jwtLocation := map[string]string{} | ||
| for _, jwt := range jwtRules { |
There was a problem hiding this comment.
You may want to sort the rule alphabetically by issuer (ignore this if you already did it when calling this function)
There was a problem hiding this comment.
can we do this in the applier init?
There was a problem hiding this comment.
There was a problem hiding this comment.
You can. Feel free to change that one.
There was a problem hiding this comment.
done. sorted and add one more test case.
| Name: authn_model.AuthnFilterName, | ||
| } | ||
| filterConfigProto := convertToIstioAuthnFilterConfig(a.processedJwtRules) | ||
| if filterConfigProto == nil { |
There was a problem hiding this comment.
nit. convertToIstioAuthnFilterConfig never returns nil (should it?), so this is not necessary.
There was a problem hiding this comment.
i think that's fine. caller be more agnostic of the actual implementation is a good thing IMO.
| t.Run(c.name, func(t *testing.T) { | ||
| got := NewPolicyApplier(c.in).AuthNFilter(model.SidecarProxy, false) | ||
| if !reflect.DeepEqual(c.expected, got) { | ||
| // t.Logf("struct literal\n%#v\n", got.ConfigType) |
|
I'll delete the code links for the fields reference before the merge. |
Beta authn policy still uses the old authn filter, with specially crafted configuration.