Skip to content

Conversation

@howardjohn
Copy link
Member

@howardjohn howardjohn commented Mar 8, 2021

This adds a new blog post to describe upcoming changes, and how users can detect and plan for the change

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 8, 2021
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 8, 2021
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 8, 2021
@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@howardjohn howardjohn changed the title WIP: Add blog on inbound forwarding changes Add blog on inbound forwarding changes Mar 25, 2021
@howardjohn howardjohn force-pushed the blog/inbound-forwarding branch from 09786a9 to 139da7c Compare March 25, 2021 18:25
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Apr 7, 2021
@howardjohn howardjohn marked this pull request as ready for review April 7, 2021 23:31
@howardjohn howardjohn requested a review from a team as a code owner April 7, 2021 23:31
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 7, 2021
@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Apr 8, 2021
For new users, this change should only be an improvement.
However, if you are an existing user, you may have come to depend on the old behavior, intentionally or accidentally.

To help detect these situations, we have added a check to find pods that will be impacted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What version of istioctl has these checks? Is it already in 1.9? If we post the blog before Istio 1.10 is released, you should mention what version they need to use to do the checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

1.10 only unfortunately, and not easy to backport. Ill add a note

However, if you are an existing user, you may have come to depend on the old behavior, intentionally or accidentally.

To help detect these situations, we have added a check to find pods that will be impacted.
You can run the `istioctl precheck` command to get a report of any pods binding to `lo` on a port exposed in a `Service`.
Copy link
Contributor

Choose a reason for hiding this comment

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

istioctl experimental precheck right?

@linsun
Copy link
Member

linsun commented Apr 9, 2021

Q @howardjohn on the background and current behavior diagram, the application containers are 2 app containers? Are they really complete 2 different containers? Think of our samples like sleep or product page, it is just 1 app container.

@howardjohn
Copy link
Member Author

Q @howardjohn on the background and current behavior diagram, the application containers are 2 app containers? Are they really complete 2 different containers? Think of our samples like sleep or product page, it is just 1 app container.

No, its supposed to be one app. Its just saying it can bind to one or both network interfaces. Maybe it would be more clear to have one blob that is the app

To help detect these situations, we have added a check to find pods that will be impacted.
You can run the `istioctl experimental precheck` command to get a report of any pods binding to `lo` on a port exposed in a `Service`.
This command is available in Istio 1.10+.
Without action, these ports will no longer be accessible upon upgrade.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Without action, these ports will no longer be accessible upon upgrade.
**Without action, these ports will no longer be accessible upon upgrade.**

Suggest highlight this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to say if the error (since you are changing the msg from warning to error) has occurred, you must use revision based upgrade to thoroughly test your application continues to work with the new control plane.

If you are currently binding to `lo`, you have a few options:

* Switch your application to bind to all interfaces (`0.0.0.0` or `::`).
* Explicitly configure the port using the [`Sidecar` ingress configuration](/docs/reference/config/networking/sidecar/#IstioIngressListener) to send to `lo`, preserving the old behavior.
Copy link
Member

Choose a reason for hiding this comment

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

Is the configuration for the port or defaultEndpoint field in sidecar resource? A simple example would be really helpful here.

Walking through a migration scenario, if I create the sidecar resource for my istio 1.10 cp, and also created my app with 1.10's data plane, would that sidecar resource break traffic for my app with 1.9's data plane? (Correct me if I am wrong, I think our sidecar resource is global for a ns regardless of which control plane revision?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the configuration for the port or defaultEndpoint field in sidecar resource

Both are required fields

The steps would be:

  • Create sidecar in 1.9. This is scoped to a workload. It has no impact.
  • Upgrade to 1.10. Because of sidecar explicitly saying to connect to localhost, there is no change in behavior

Copy link
Member

Choose a reason for hiding this comment

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

Yea was trying to see if we could isolate the sidecar only for 1.10. I don't think we could. In that case, user should validate sidecar resource has no impact to the app (1.9 based) first before upgrading to 1.10.

Note: some of the stateful apps, we ask users to change certain configs to listen on localhost, probably a good time to revert that change after testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can with istio.io/rev, but there is no real reason to. I already changed that in https://preliminary.istio.io/latest/about/faq#applications.

@linsun
Copy link
Member

linsun commented Apr 9, 2021

Q @howardjohn on the background and current behavior diagram, the application containers are 2 app containers? Are they really complete 2 different containers? Think of our samples like sleep or product page, it is just 1 app container.

No, its supposed to be one app. Its just saying it can bind to one or both network interfaces. Maybe it would be more clear to have one blob that is the app

Yes, I think it would.

@mandarjog
Copy link
Contributor

@howardjohn the blog title is too vague.
“Upcoming changes ...”

We need to make it more about the specific topic about inbound networking and the fast that it is a breaking change.

@howardjohn howardjohn added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Apr 13, 2021
@@ -0,0 +1,91 @@
---
title: "Upcoming changes to Istio networking In Istio 1.10"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: "Upcoming changes to Istio networking In Istio 1.10"
title: "Upcoming networking changes In Istio 1.10"

Comment on lines 72 to 89
For example, to configure request to be sent to `localhost` for the `ratings` application:

{{< text yaml >}}
apiVersion: networking.istio.io/v1beta1
kind: Sidecar
metadata:
name: ratings
spec:
workloadSelector:
labels:
app: ratings
ingress:
- port:
number: 8080
protocol: HTTP
name: http
defaultEndpoint: 127.0.0.1:8080
{{< /text >}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For example, to configure request to be sent to `localhost` for the `ratings` application:
{{< text yaml >}}
apiVersion: networking.istio.io/v1beta1
kind: Sidecar
metadata:
name: ratings
spec:
workloadSelector:
labels:
app: ratings
ingress:
- port:
number: 8080
protocol: HTTP
name: http
defaultEndpoint: 127.0.0.1:8080
{{< /text >}}
For example, to configure request to be sent to `localhost` for the `ratings` application:
{{< text yaml >}}
apiVersion: networking.istio.io/v1beta1
kind: Sidecar
metadata:
name: ratings
spec:
workloadSelector:
labels:
app: ratings
ingress:
- port:
number: 8080
protocol: HTTP
name: http
defaultEndpoint: 127.0.0.1:8080
{{< /text >}}

defaultEndpoint: 127.0.0.1:8080
{{< /text >}}

* Disable the change entirely with the `PILOT_ENABLE_INBOUND_PASSTHROUGH=false` environment variable in Istiod. This option will be removed in the future.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Disable the change entirely with the `PILOT_ENABLE_INBOUND_PASSTHROUGH=false` environment variable in Istiod. This option will be removed in the future.
* Disable the change entirely with the `PILOT_ENABLE_INBOUND_PASSTHROUGH=false` environment variable in Istiod to enable the same behavior as prior to Istio 1.10. This option will be removed in the future.

@linsun
Copy link
Member

linsun commented Apr 14, 2021

@howardjohn thank you - it is pretty clear to me!

@howardjohn
Copy link
Member Author

@linsun @frankbu thanks, I applied your comments

@google-cla
Copy link

google-cla bot commented Apr 14, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. labels Apr 14, 2021
@craigbox
Copy link
Contributor

@frankbu please pay homage to Googlebot when you have a chance.

@howardjohn howardjohn added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Apr 14, 2021
Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>
@google-cla
Copy link

google-cla bot commented Apr 15, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. labels Apr 15, 2021
@howardjohn howardjohn added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Apr 15, 2021
@howardjohn
Copy link
Member Author

Does anyone mind if I make a post on #announcenents to get some extra eyes on this?

@frankbu
Copy link
Collaborator

frankbu commented Apr 15, 2021

/test doc.test.profile_default_istio.io

@ericvn
Copy link
Contributor

ericvn commented Apr 15, 2021

/retest

@istio-testing
Copy link
Contributor

In response to a cherrypick label: new pull request created: #9532

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.

9 participants