Changing the WasmPlugin.Priority field from int64 to int32 #2577

jonathanvila · 2022-11-23T11:21:51Z

Protobuf is generating an int64 into a string, and this is generating issues with regular marshallers in order to generate a K8s resource from a proto, because K8s expects priority to be a number not a string.

Also considering that this is a Priority, looks like the possibility to need an int64 is not a common one but a remote one, and also considering that Envoy filter priority field is an int32.

istio-policy-bot · 2022-11-23T11:21:53Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

istio-policy-bot · 2022-11-23T11:21:55Z

😊 Welcome @jonathanvila! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

google-cla · 2022-11-23T11:21:55Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

istio-testing · 2022-11-23T11:22:02Z

Hi @jonathanvila. Thanks for your PR.

I'm waiting for a istio 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.

howardjohn

I am not sure this is a compatible protobuf change, won't the message encoding be different?

jonathanvila · 2022-11-23T18:30:39Z

I am not sure this is a compatible protobuf change, won't the message encoding be different?

You mean that already existing messages for WasmPlugin will not be compatible with this new change ?

jonathanvila · 2022-11-23T18:35:32Z

@howardjohn following this doc about protobuf looks like int32 and int64 are compatible:
https://developers.google.com/protocol-buffers/docs/proto#updating

int32, uint32, int64, uint64, and bool are all compatible – this means you can change a field from one of these types to another without breaking forwards- or backwards-compatibility. If a number is parsed from the wire which doesn't fit in the corresponding type, you will get the same effect as if you had cast the number to that type in C++ (for example, if a 64-bit number is read as an int32, it will be truncated to 32 bits).

howardjohn · 2022-11-23T18:40:19Z

Its not an int64 though its a Int64Value

jonathanvila · 2022-11-23T18:47:39Z

Its not an int64 though its a Int64Value

I know, but they are the same wrappers for an internal field that is the one that changes.......

message Int64Value {
  // The int64 value.
  int64 value = 1;
}

vs

message Int32Value {
  // The int32 value.
  int32 value = 1;
}

jonathanvila · 2022-11-28T09:36:20Z

@howardjohn is there anything we could do to progress in this PR ?
or do you think it's better to change the CRD accepting int or string instead of changing the proto ?

hanxiaop · 2023-05-23T07:23:55Z

/ok-to-test

istio-testing · 2023-05-23T07:24:38Z

@jonathanvila: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_api	`6ec3de5`	link	false	`/test release-notes`

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. I understand the commands that are listed here.

howardjohn · 2023-05-23T20:51:31Z

/easycla

linux-foundation-easycla · 2023-05-23T20:51:36Z

The committers listed above are authorized under a signed CLA.

✅ login: jonathanvila / name: Jonathan Vila (6ec3de5)

ericvn · 2023-05-24T12:54:53Z

Note that this broke the pipeline: https://prow.istio.io/view/gs/istio-prow/logs/update_client-go_dep_client-go_postsubmit/1661344222390784000:

pilot/pkg/model/addressmap.go:1: : # istio.io/istio/pilot/pkg/model [istio.io/istio/pilot/pkg/model.test]
pilot/pkg/model/push_context.go:1862:17: cannot use prio.Value (variable of type int32) as int64 value in assignment
pilot/pkg/model/push_context.go:1866:17: cannot use prio.Value (variable of type int32) as int64 value in assignment
pilot/pkg/model/push_context_test.go:478:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:486:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:502:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:514:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:521:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:539:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal
pilot/pkg/model/push_context_test.go:557:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal (typecheck)

howardjohn · 2023-05-24T15:07:10Z

I think that is ok, we just need to manually update it. We only offer yaml+proto compatibility which this change meets, just breaks Go.

…

On Wed, May 24, 2023 at 5:55 AM Eric Van Norman ***@***.***> wrote: Note that this broke the pipeline: https://prow.istio.io/view/gs/istio-prow/logs/update_client-go_dep_client-go_postsubmit/1661344222390784000 : pilot/pkg/model/addressmap.go:1: : # istio.io/istio/pilot/pkg/model [istio.io/istio/pilot/pkg/model.test] pilot/pkg/model/push_context.go:1862:17: cannot use prio.Value (variable of type int32) as int64 value in assignment pilot/pkg/model/push_context.go:1866:17: cannot use prio.Value (variable of type int32) as int64 value in assignment pilot/pkg/model/push_context_test.go:478:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:486:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:502:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:514:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:521:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:539:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal pilot/pkg/model/push_context_test.go:557:15: cannot use &wrappers.Int64Value{…} (value of type *wrapperspb.Int64Value) as *wrapperspb.Int32Value value in struct literal (typecheck) — Reply to this email directly, view it on GitHub <#2577 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNZJZQLJFK65TTX563XHYAKPANCNFSM6AAAAAASI3FORE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ericvn · 2023-05-24T16:00:47Z

istio/istio#45103 created.

Changing the WasmPlugin.Priority field from int64 to int32

6ec3de5

jonathanvila requested a review from a team as a code owner November 23, 2022 11:21

istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test labels Nov 23, 2022

howardjohn reviewed Nov 23, 2022

View reviewed changes

hzxuzhonghu approved these changes May 23, 2023

View reviewed changes

kebe7jun approved these changes May 23, 2023

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels May 23, 2023

howardjohn approved these changes May 23, 2023

View reviewed changes

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label May 23, 2023

istio-testing merged commit 0f5a7e9 into istio:master May 24, 2023

ericvn mentioned this pull request May 24, 2023

Run update_deps (minus ztunnel and client-go changes) istio/istio#45097

Merged

ericvn mentioned this pull request May 24, 2023

Update client-go (requires manual changes) istio/istio#45103

Merged

johnlanni mentioned this pull request Mar 26, 2024

fix priority type of wasmplugin alibaba/higress#881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing the WasmPlugin.Priority field from int64 to int32 #2577

Changing the WasmPlugin.Priority field from int64 to int32 #2577

jonathanvila commented Nov 23, 2022

istio-policy-bot commented Nov 23, 2022

istio-policy-bot commented Nov 23, 2022

google-cla bot commented Nov 23, 2022

istio-testing commented Nov 23, 2022

howardjohn left a comment

jonathanvila commented Nov 23, 2022

jonathanvila commented Nov 23, 2022

howardjohn commented Nov 23, 2022

jonathanvila commented Nov 23, 2022

jonathanvila commented Nov 28, 2022 •

edited

Loading

hanxiaop commented May 23, 2023

istio-testing commented May 23, 2023

howardjohn commented May 23, 2023

linux-foundation-easycla bot commented May 23, 2023 •

edited

Loading

ericvn commented May 24, 2023

howardjohn commented May 24, 2023 via email

ericvn commented May 24, 2023

Changing the WasmPlugin.Priority field from int64 to int32 #2577

Changing the WasmPlugin.Priority field from int64 to int32 #2577

Conversation

jonathanvila commented Nov 23, 2022

istio-policy-bot commented Nov 23, 2022

istio-policy-bot commented Nov 23, 2022

google-cla bot commented Nov 23, 2022

istio-testing commented Nov 23, 2022

howardjohn left a comment

Choose a reason for hiding this comment

jonathanvila commented Nov 23, 2022

jonathanvila commented Nov 23, 2022

howardjohn commented Nov 23, 2022

jonathanvila commented Nov 23, 2022

jonathanvila commented Nov 28, 2022 • edited Loading

hanxiaop commented May 23, 2023

istio-testing commented May 23, 2023

howardjohn commented May 23, 2023

linux-foundation-easycla bot commented May 23, 2023 • edited Loading

ericvn commented May 24, 2023

howardjohn commented May 24, 2023 via email

ericvn commented May 24, 2023

jonathanvila commented Nov 28, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 23, 2023 •

edited

Loading