Skip to content

Move webhook to pilot component#20280

Merged
istio-testing merged 1 commit intoistio:masterfrom
howardjohn:inject/move-to-pilot-component
Jan 22, 2020
Merged

Move webhook to pilot component#20280
istio-testing merged 1 commit intoistio:masterfrom
howardjohn:inject/move-to-pilot-component

Conversation

@howardjohn
Copy link
Copy Markdown
Member

@howardjohn howardjohn commented Jan 17, 2020 •
edited
Loading

This will allow the old injector to be pruned on upgrade

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 17, 2020
@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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 17, 2020
@howardjohn howardjohn marked this pull request as ready for review January 17, 2020 20:55
@howardjohn howardjohn requested a review from a team as a code owner January 17, 2020 20:55
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 17, 2020
@elfinhe elfinhe self-assigned this Jan 17, 2020
@howardjohn howardjohn force-pushed the inject/move-to-pilot-component branch 2 times, most recently from 7175e06 to 15e5383 Compare January 21, 2020 04:34
elfinhe
elfinhe reviewed Jan 21, 2020
View reviewed changes
operator/data/profiles/default.yaml Outdated
Copy link
Copy Markdown
Member

@elfinhe elfinhe Jan 21, 2020

Choose a reason for hiding this comment

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

is this going to 1) turn down injector feature or 2) disable the inject deployment and service but still have the feature enabled in pilot?

if 1), why do we turn it off in default profile?

Copy link
Copy Markdown
Member Author

@howardjohn howardjohn Jan 21, 2020

Choose a reason for hiding this comment

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

We want the feature "auto injection" to be enabled. We want the component, sidecarInjector to be disabled (because it is in pilot).

trying again from master, I think its actually behaving differently now, and completely broken. Even if we have injector=enabled, we still prune it (!!), likely because of the change we made about the component name changing?

Copy link
Copy Markdown
Member Author

@howardjohn howardjohn Jan 21, 2020

Choose a reason for hiding this comment

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

see #20387

Copy link
Copy Markdown
Member

@elfinhe elfinhe Jan 21, 2020

Choose a reason for hiding this comment

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

that makes sense, somehow we may need delete sidecarInjector from IOP API in the future.

@howardjohn howardjohn force-pushed the inject/move-to-pilot-component branch from 15e5383 to da15ab2 Compare January 22, 2020 00:26
@howardjohn howardjohn force-pushed the inject/move-to-pilot-component branch from da15ab2 to 08e12ec Compare January 22, 2020 03:51
@howardjohn howardjohn mentioned this pull request Jan 22, 2020
Closed
@istio-testing istio-testing merged commit 77b9ea4 into istio:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@elfinhe elfinhe elfinhe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@elfinhe elfinhe

@ostromart ostromart

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@howardjohn @elfinhe @googlebot @istio-testing @ostromart