add support for configuring internal addresses #2312

ramaraochavali · 2022-04-05T12:15:21Z

Envoy now supports configuring cidrs that should be treated as internal addresses https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L209.

This affects how Envoy treats incoming connections as internal or external. In some of envs, RFC 1918 is not followed for internal addresses.

This allows a mesh to configure internal addresses cidrs

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

istio-policy-bot · 2022-04-05T12:15:23Z

😊 Welcome @ramaraochavali! 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.

costinm

I was also looking at something like this - not sure what your use case is, but I am trying to simplify the configuration of 'egress gateway' by redirecting all non-internal addresses to the gateway. Still working on a RFC..

What I was thinking was to actually extend the existing 'networks'. In other words, a user may have a multi-network mesh, with each network having a set of 'internal cidr ranges' - and we already have a config to define that via MeshNetworks.fromCidr.

The equivalent of what you propose is using
default:

fromCidr: ....

costinm · 2022-04-05T15:54:32Z

mesh/v1alpha1/config.proto

@@ -1088,8 +1088,12 @@ message MeshConfig {
  // Configuration of mTLS for traffic between workloads within the mesh.
  TLSConfig mesh_mTLS = 63;

+  // List of CIDR ranges that are treated as internal. If unset, then RFC1918 / RFC4193
+  // IP addresses will be considered internal.
+  repeated string internal_addresses = 64;


But what happens if it is set ? Are those extras ?

If this is set, the cidr range specified here would be considered as internal address instead of default one

do we need set all clusters' pod cidrs(in a multi clusters mesh) here for best practice?

Not for 'best practice' - it won't work otherwise... But I think if you add the ranges above it'll cover all the mesh.

do we need set all clusters' pod cidrs(in a multi clusters mesh) here for best practice
No.

Yes. If we add the ranges above, it covers the entire mesh

costinm · 2022-04-06T00:48:19Z

On Tue, Apr 5, 2022 at 9:13 AM Rama Chavali ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mesh/v1alpha1/config.proto <#2312 (comment)>: > @@ -1088,8 +1088,12 @@ message MeshConfig { // Configuration of mTLS for traffic between workloads within the mesh. TLSConfig mesh_mTLS = 63; + // List of CIDR ranges that are treated as internal. If unset, then RFC1918 / RFC4193 + // IP addresses will be considered internal. + repeated string internal_addresses = 64; If this is set, the cidr range specified here would be considered as internal address instead of default one

Maybe add this to the doc, and list the IP4 and IP6 ranges that are set as default ( so if the user wants to _add_ an extra network they don't miss some of the ranges in RFC1918/...)

…

Message ID: ***@***.***>

hzxuzhonghu · 2022-04-06T03:30:56Z

Can you elaborate how internal address is treated differently

ramaraochavali · 2022-04-06T05:13:15Z

@hzxuzhonghu Envoy sets "x-envoy-internal" header and services can use it to decide whether the call is from internal mesh/public. That is a common usecase.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-04-06T05:14:38Z

@costinm That makes sense. I updated the comments.

hzxuzhonghu · 2022-04-06T06:33:23Z

@hzxuzhonghu Envoy sets "x-envoy-internal" header and services can use it to decide whether the call is from internal mesh/public. That is a common usecase.

Why could not infer this from other headers?

ramaraochavali · 2022-04-06T06:37:39Z

Why could not infer this from other headers?

Which header?

hzxuzhonghu · 2022-04-06T06:38:59Z

x-envoy-xxx

ramaraochavali · 2022-04-06T06:41:22Z

x-envoy-xxx

which header? x-envoy-internal is the one which tells whether the address is internal or external and it is derived from xff (which is again based on this value)

hzxuzhonghu · 2022-04-06T07:25:23Z

Oh, I misunderstood a little about the background. Since for istio internal mesh sidecar-sidecar communication, no xff is appended. only external-call --> gateway--> server, xff is generated. By looking at this header, could know if it is external

ramaraochavali · 2022-04-06T08:06:22Z

Yes.

ramaraochavali · 2022-04-07T04:15:53Z

@costinm @hzxuzhonghu Can you please approve if you don't have any further questions?

costinm

few remaining issues:

please add the IP6 ranges as well to the doc - I bet users will forget to add them
I think we need more discussion if we want to add this here, or use the existing 'networks', which define the ranges of pods in each internal network.

In a multi-network case the user shouldn't have to duplicate the ranges both here and in networks. The networks also has the benefit that it is also associating the ranges with network names, so I would prefer that - we can get the same behavior you have here by setting the fromRanges on the 'default' network with no API changes.

costinm · 2022-04-07T14:48:08Z

mesh/v1alpha1/config.proto

@@ -1088,8 +1088,12 @@ message MeshConfig {
  // Configuration of mTLS for traffic between workloads within the mesh.
  TLSConfig mesh_mTLS = 63;

+  // List of CIDR ranges that are treated as internal. If unset, then RFC1918 / RFC4193
+  // IP addresses will be considered internal.
+  repeated string internal_addresses = 64;


Not for 'best practice' - it won't work otherwise... But I think if you add the ranges above it'll cover all the mesh.

ramaraochavali · 2022-04-08T08:06:21Z

I think we need more discussion if we want to add this here, or use the existing 'networks', which define the ranges of pods in each internal network.

If we go down this path - we need to change https://github.com/istio/api/blob/master/mesh/v1alpha1/network.proto#L53 to repeated. If you are OK with that, I can change that as well

howardjohn · 2022-04-08T20:05:18Z

mesh/v1alpha1/config.proto

@@ -1088,8 +1088,18 @@ message MeshConfig {
  // Configuration of mTLS for traffic between workloads within the mesh.
  TLSConfig mesh_mTLS = 63;

+  // List of CIDR ranges that are treated as internal. If unset, then RFC1918 / RFC4193


can you explain in the comments what the implications of "internal" are?

howardjohn · 2022-04-08T20:05:26Z

mesh/v1alpha1/config.proto

+  // List of CIDR ranges that are treated as internal. If unset, then RFC1918 / RFC4193
+  // IP addresses will be considered internal.
+  // RFC 1918 defines the following address ranges as private,
+  // 10.0. 0.0/8 (addresses 10.0. 0.0 through 10.255. 255.255 inclusive)


nit: weird spacing in the IPs?

yeah. Copied from RFC. Will correct

costinm · 2022-04-08T20:35:49Z

BTW - if you do make changes to the Networks proto, I would strongly recommend ``` from: - ipBlock: cidr: 172.17.0.0/16 except: - 172.17.1.0/24 - ipBlock: cidr: 1.1.1.0/24 -.... ``` ( copied from NetworkPolicy ) - instead of another variation. Will also make it easier to write NetworkPolicies - I expect users will want to do this as well for 'in-depth' defence.

…

On Fri, Apr 8, 2022 at 7:07 AM Costin Manolache ***@***.***> wrote: Endpoints are already repeated. - fromCidr: 1.1.1.0/24 - fromCidr: ..... I don't think making a fromCidr repeated is backward compatible, but we could add a new one endpoints: fromCidrs: - .... - .... How about getting the implementation with the current API, and sync in next NetworkingWG - I think having a good way to describe the IP ranges and networks is pretty important but may be time to move it to a proper CRD and figure out how it can be used with K8S Gateway attachment policy, etc. Meanwhile - I don't think the feature you need is blocked by API change if you use networks, just not ideal UX. On Fri, Apr 8, 2022 at 1:06 AM Rama Chavali ***@***.***> wrote: > I think we need more discussion if we want to add this here, or use the > existing 'networks', which define the ranges of pods in each internal > network. > > If we go down this path - we need to change > https://github.com/istio/api/blob/master/mesh/v1alpha1/network.proto#L53 > to repeated. If you are OK with that, I can change that as well > > — > Reply to this email directly, view it on GitHub > <#2312 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2R2QZV27A4VNGWHPKTVD7SITANCNFSM5SSRZ7HQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

ramaraochavali · 2022-04-09T04:15:56Z

@costinm My preference would be to leave it here as it s mesh wide config and people running with single network do not have to worry about additional network config + even if we network it needs an API change. But I see your point about redundancy, but I expect people who use it for multi network cases, do not have to worry about it. And I am not sure where/how that from_cidr is used.
@howardjohn WDYT?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

costinm · 2022-04-09T15:31:25Z

Networks are also 'mesh wide', and it is fine to define a single network ( default ). In reality "mesh config" is not 'mesh wide' - but specific to one revision in one cluster - promoting the network description to CRD could make it mesh wide, and seems a generally useful feature. If multi-cluster code starts watching them - we won't have to copy the same thing in all clusters and keep it in sync. There are other related use cases - for example non-Istio gateways that use a dedicated IP range ( separate node pool, etc) and we 'trust' them using L3 (assuming network security).

…

On Fri, Apr 8, 2022 at 9:16 PM Rama Chavali ***@***.***> wrote: @costinm <https://github.com/costinm> My preference would be to leave it here as it s mesh wide config and people running with single network do not have to worry about additional network config. But I see your point about redundancy, but I expect people who use it for multi network cases, do not have to worry about it. And I am not sure where/how that from_cidr is used. @howardjohn <https://github.com/howardjohn> WDYT? — Reply to this email directly, view it on GitHub <#2312 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SPMUTQSSCLKOZHI5TVEEAAPANCNFSM5SSRZ7HQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

nrjpoddar · 2022-04-11T09:39:06Z

Are we still adding fields to MeshConfig or have frozen it to provide a way to migrate to the CRD?

ramaraochavali · 2022-04-11T10:48:17Z

This is not for proxyConfig - Are you thinking it is for proxy config?

costinm · 2022-04-11T17:21:29Z

We are trying to minimize additions to MeshConfig, and migrate to CRDs - but still pragmatic. Sometimes it is ok to add an env variable or mesh config and get feedback, without pressure to get a CRD out fast.

…

On Mon, Apr 11, 2022 at 2:39 AM Neeraj Poddar ***@***.***> wrote: Are we still adding fields to MeshConfig or have frozen it to provide a way to migrate to the CRD? — Reply to this email directly, view it on GitHub <#2312 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2RRH2AXQ7NG6MNX3FLVEPXMLANCNFSM5SSRZ7HQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ramaraochavali · 2022-04-22T06:24:49Z

@costinm @howardjohn Please let me know your preference - I want to move forward with one way or other. I am not fully familiar with network use cases so I am good with what you think is best place to put this. But I would like to add API support for this

costinm · 2022-04-26T22:01:53Z

My preference remains to use networks. It may not even require any API change - just a flag/env variable in Istiod to indicate that networks will be used to trigger this behavior.

User will have to configure a network ('default') with all the ranges. They may also configured additional networks. All defined networks will be treated as internal, if this flag is set.

ramaraochavali · 2022-04-27T21:24:39Z

I am fine with that.

@howardjohn Are you good with that as well?

ramaraochavali · 2022-05-10T09:36:04Z

Closing this in-favour of istio/istio#38801

costinm · 2022-10-11T09:07:18Z

Endpoints are already repeated. - fromCidr: 1.1.1.0/24 - fromCidr: ..... I don't think making a fromCidr repeated is backward compatible, but we could add a new one endpoints: fromCidrs: - .... - .... How about getting the implementation with the current API, and sync in next NetworkingWG - I think having a good way to describe the IP ranges and networks is pretty important but may be time to move it to a proper CRD and figure out how it can be used with K8S Gateway attachment policy, etc. Meanwhile - I don't think the feature you need is blocked by API change if you use networks, just not ideal UX.

…

On Fri, Apr 8, 2022 at 1:06 AM Rama Chavali ***@***.***> wrote: I think we need more discussion if we want to add this here, or use the existing 'networks', which define the ranges of pods in each internal network. If we go down this path - we need to change https://github.com/istio/api/blob/master/mesh/v1alpha1/network.proto#L53 to repeated. If you are OK with that, I can change that as well — Reply to this email directly, view it on GitHub <#2312 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2R2QZV27A4VNGWHPKTVD7SITANCNFSM5SSRZ7HQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

add support for configuring internal addresses

7ab9c8f

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested review from linsun, louiscryan, nrjpoddar, howardjohn and ericvn as code owners April 5, 2022 12:15

istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 5, 2022

ramaraochavali added release-notes-none Indicates a PR that does not require release notes. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 5, 2022

costinm reviewed Apr 5, 2022

View reviewed changes

update docs

b8adad2

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 6, 2022

costinm reviewed Apr 7, 2022

View reviewed changes

howardjohn reviewed Apr 8, 2022

View reviewed changes

fix ip format and add more docs

15797f0

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali mentioned this pull request May 9, 2022

make use of networks to determine internal address config istio/istio#38801

Merged

10 tasks

ramaraochavali closed this May 10, 2022

ramaraochavali deleted the fix/internal_addresses branch May 10, 2022 09:36

add support for configuring internal addresses #2312

add support for configuring internal addresses #2312

Uh oh!

Conversation

ramaraochavali commented Apr 5, 2022

Uh oh!

istio-policy-bot commented Apr 5, 2022

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zirain Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm commented Apr 6, 2022 via email

Uh oh!

hzxuzhonghu commented Apr 6, 2022

Uh oh!

ramaraochavali commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented Apr 6, 2022

Uh oh!

hzxuzhonghu commented Apr 6, 2022

Uh oh!

ramaraochavali commented Apr 6, 2022

Uh oh!

hzxuzhonghu commented Apr 6, 2022

Uh oh!

ramaraochavali commented Apr 6, 2022

Uh oh!

hzxuzhonghu commented Apr 6, 2022

Uh oh!

ramaraochavali commented Apr 6, 2022

Uh oh!

ramaraochavali commented Apr 7, 2022

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Apr 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm commented Apr 8, 2022 via email

Uh oh!

ramaraochavali commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

costinm commented Apr 9, 2022 via email

Uh oh!

nrjpoddar commented Apr 11, 2022

Uh oh!

ramaraochavali commented Apr 11, 2022

Uh oh!

costinm commented Apr 11, 2022 via email

Uh oh!

ramaraochavali commented Apr 22, 2022

Uh oh!

costinm commented Apr 26, 2022

Uh oh!

ramaraochavali commented Apr 27, 2022

Uh oh!

zirain Apr 7, 2022 •

edited

Loading

ramaraochavali commented Apr 6, 2022 •

edited

Loading

ramaraochavali commented Apr 9, 2022 •

edited

Loading