Skip to content

Add stats and metadata wasm plugin into proxy image#20259

Merged
istio-testing merged 3 commits intoistio:masterfrom
bianpengyuan:docker-wasm
Jan 21, 2020
Merged

Add stats and metadata wasm plugin into proxy image#20259
istio-testing merged 3 commits intoistio:masterfrom
bianpengyuan:docker-wasm

Conversation

@bianpengyuan
Copy link
Copy Markdown
Contributor

@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 17, 2020
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 17, 2020
Copy link
Copy Markdown
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how big are they?

Makefile.core.mk Outdated
export ISTIO_ENVOY_MACOS_RELEASE_PATH ?= ${ISTIO_ENVOY_MACOS_RELEASE_DIR}/${ISTIO_ENVOY_MACOS_RELEASE_NAME}

# Variable for WebAssembly plugins
export STATS_FILTER_WASM_URL ?= $(ISTIO_ENVOY_BASE_URL)/stats-$(ISTIO_ENVOY_VERSION).wasm
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just have a “list_of_plugins” var instead of specific variables per plugin.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified the variables with a for loop.

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

@howardjohn 1.8m combined.

@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Jan 17, 2020 via email

COPY envoy_telemetry.yaml.tmpl /etc/istio/proxy/envoy_telemetry.yaml.tmpl

COPY stats-filter.wasm /etc/istio/proxy/stats-filter.wasm
COPY metadata-exchange-filter.wasm /etc/istio/proxy/metadata-exchange-filter.wasm
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is the v8 one?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just allows us to use it, it doesn't require it right? The config is from the EnvoyFilter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah these are the v8 based plugins, and yeah there will be option to enable them if they pass stability and perf test.

@howardjohn howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jan 18, 2020
@howardjohn
Copy link
Copy Markdown
Member

I approved but put a hold on case others want to review

COPY envoy_policy.yaml.tmpl /etc/istio/proxy/envoy_policy.yaml.tmpl
COPY envoy_telemetry.yaml.tmpl /etc/istio/proxy/envoy_telemetry.yaml.tmpl

COPY stats-filter.wasm /etc/istio/proxy/stats-filter.wasm
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want this to be /etc/istio/proxy/extensions
That way we / customer can mount over this dir.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like mounting on the same path will remove these files. Shall we reserve /etc/istio/proxy/extensions for user provided plugins and keep these two plugins where they are now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overmounting indeed replaces the directory , it is not unionfs.

Rethinking, We should treat the two wasm modules as if they are compiled in. Given that, it does not have to be in the extensions dir, since we expect extension names to be versioned and these names are not.

You can keep it as it is, I will change it later if needed .

@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Jan 18, 2020 via email

Copy link
Copy Markdown
Contributor

@mandarjog mandarjog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@mandarjog mandarjog removed the do-not-merge/hold Block automatic merging of a PR. label Jan 21, 2020
@istio-testing istio-testing merged commit 07dedc3 into istio:master Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants