Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`cf ssh` does not work for Windows cells when PAS is deployed with NSX-T Container Plugin (NCP) tile #427

Closed
cunnie opened this issue Apr 26, 2019 · 8 comments
Labels

Comments

@cunnie
Copy link
Member

@cunnie cunnie commented Apr 26, 2019

Summary

cf ssh does not work for Windows cells when PAS is deployed with NSX-T Container Plug-in (NCP) tile.

Expected Result

cf ssh drops me, the operator, into a "container" on the Windows cell.

Actual Result

It hangs. We didn't wait for the timeout.

cf ssh win-test

Context

There is at least one user who would like to use PAS, PASW, and NSX-T concurrently. It's important that they be able to cf ssh to both their Windows and Linux apps.

Possible Cause

cf ssh is enabled on NSX-T-backed deployments by setting the ssh-proxy property, connect_to_instance_address, a boolean, which defaults to false.

When an external networking plugin (such as NSX-T's plugin) is deployed, this property is flipped to true.

Problem: the Linux apps are reachable from their instance address, but the Windows apps aren't. The NSX-T container plugin currently only applies to the Linux apps (the NSX-T is co-located on the Linux Diego cell via a BOSH runtime config, but not colocated on the Windows cell).

Steps to Reproduce

[Provide a minimal sequence of detailed steps to reproduce the issue, including how to observe the effects of the issue.]

  • Deploy PAS Tile
  • Deploy PASW Tile
  • Deploy NSX-T Container Plugin Tile
  • push app to Windows cell

Possible Fix

One approach would be to plumb an additional parameter, prefer_instance_address, which would signal to the ssh-proxy job to which IP address & port to forward the ssh traffic, e.g. (redacted output from cfdot actual-lrp-groups):

{
  "instance": {
    "address": "192.168.2.28",
    "ports": [
      {
        "container_port": 8080,
        "host_port": 61032,
        "container_tls_proxy_port": 61001,
        "host_tls_proxy_port": 61034
      },
      {
        "container_port": 2222,
        "host_port": 61033,
        "container_tls_proxy_port": 61002,
        "host_tls_proxy_port": 61035
      }
    ],
    "instance_address": "10.12.29.15",
+   "prefer_instance_address": True
  }
}

This parameter would be a BOSH property of the rep job, set at deployment. This BOSH property is then provided to the process that composes LRPs. Finally, the bool on the LRP can be read by the SSH Proxy, which handles forwarding logic. This property would default to False, and container plugins may optionally enable it. In the case of NSX-T, the prefer_instance_address property would be enabled on the Linux Diego cells but not the Windows Diego cells.

Additional Text Output or Screenshots (optional)

IMG_20190426_113813793

@cf-gitbot

This comment has been minimized.

Copy link
Collaborator

@cf-gitbot cf-gitbot commented Apr 26, 2019

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/165647777

The labels on this github issue will be updated when the story is started.

@cunnie

This comment has been minimized.

Copy link
Member Author

@cunnie cunnie commented Apr 26, 2019

mhoran pushed a commit to greenhouse-org/diego-release that referenced this issue Apr 26, 2019
[finishes #163037208](https://www.pivotaltracker.com/story/show/163037208)

Submodule src/code.cloudfoundry.org/executor 325be3662..b67be5e6e:
  > Merge branch 'pr/41'
Submodule src/github.com/envoyproxy/go-control-plane 000000000...8a91fb26f (new submodule)
Submodule src/github.com/gogo/googleapis 000000000...8558fb44d (new submodule)
Submodule src/github.com/gogo/protobuf f9114dace..4cbf7e384:
  > Merge pull request #528 from jmarais/mergeaa810b6
  > merged in golang/protobuf commit 7d1b268556d691919f2262240737157830eab632 - jsonpb: avoid unexported fields in hand-crafted message (#526)
  > merged in golang/protobuf commit f5983d50c82d70eaa88c17080245cc871558081f - proto: make invalid UTF-8 errors non-fatal (#525)
  > merged in golang/protobuf commit 560bdb64431cc123098c2db67f16053a923a0688 - jsonpb: strictly document JSONPBMarshaler and JSONPBUnmarshaler behavior (#524)
  > merged in golang/protobuf commit 93b26e6a70e37abb14f2f88194949312b0592a84 - protoc-gen-go: refactor generator by splitting up generateMessage
  > Merge pull request #522 from jmarais/somemerges
  > use only one write in the varint writer when possible (#504)
  > fix typo independant to independent (#512)
  > Add godocs link to Readme.md (#506)
  > Fix text unmarshal for (u)int(8/16) fields (#498)
  > Codegen for well-known types (#489)
  > reorder some of the protoc paths in order to prefer our protobuf/google/protobuf/*.proto files. This is just to avoid using the wrong protos if you have the same protos in you gopath/src dir. (#502)
  > fix error: bad Go source code was generated, illegal hexadecimal number (#488)
  > Jsonpb custom type - cloudfoundry#411 (#491)
  > Customtype  Warnings and issues  update (#479)
  > Exact slice allocation for repeated packed varints (#480)
  > Adding missing func to CustomType documentation (#483)
  > bumped the go version (#475)
  > added nil check in Proto/Size methods fix #444 (#451)
  > fix for letmegrpc (#474)
  > options to not generate xxx fields (#467)
  > updated to go1.11 and removed go1.9 (#473)
  > merged in golang/protobuf commit 70c277a8a150a8e069492e6600926300405c2884 - Fix unmarshaling JSON object with escaped string into Struct type. (#464)
  > merge in golang/protobuf commit 3a3da3a4e26776cc22a79ef46d5d58477532dede - proto: mention field name in error message (#616) (#465)
  > added license details to Readme.md (#469)
  > Update Readme.md (#468)
  > Set defaults on nonnullable fields (cloudfoundry#435) (#459)
  > removed the GOPATH env dep in the makefile protopath (#461)
  > Merge pull request #455 from donaldgraham/master
  > Fix nullable extension issues for non generated code (#453)
  > Fix wrong build tags (#445)
  > merged commit 32a84b27e28ab9f681f0df16160c4ef1f6587094 from golang/pr… (#446)
  >  Added ProtoSize wrapper functions for the well known types  cloudfoundry#438 (#443)
  > exact slice allocation for fixed size packed fields (cloudfoundry#437)
  > Added gRPC Course on Udemy (cloudfoundry#434)
  > Update Readme.md
  > Fix typo (cloudfoundry#441)
  > fix cloudfoundry#427 consistent import naming between the import declaration and the vars in grpc
  > fix build by regenerating everything
  > fix git diff for travis
  > merged 7c4add53b497798e7fd7b204f28e41ab409bdbb7 from golang/protobuf. protoc-gen-go: remove deprecated function in grpc (cloudfoundry#426)
  > merged 3fac2a27c94f99f4379551928df388fcb0ad37ce from golang/protobuf. ptypes: optimize Is to avoid prefix scan (cloudfoundry#425)
  > Merge pull request cloudfoundry#424 from gogo/dev
  > gofmt
  > travis: opt into apt get
  > messagename
  > grpc error usage article
  > add mentions from Johan Brandhorst
  > merge bbd03ef6da3a115852eaf24c8a1c46aeb39aa175 from golang/protobuf
  > upgrade to go1.10
  > fix build for gopherjs that now requires go 1.10
  > fix build
  > fix for issue 389: importing of customtypes that are messages should not cause another import for the original message
  > Update Readme.md
  > add new user : go-spacemesh
  > protoc-min-version don't suppress protoc stdout and stderr out on success. (cloudfoundry#381)
  > Update Readme.md
  > More well known types (cloudfoundry#378)
  > Added link to new blog post in README (cloudfoundry#375)
  > merge 925541529c1fa6821df4e44ce2723319eb2be768 from golang/protobuf
  > Added instructions for using proto files from google/protobuf (cloudfoundry#371)
  > another user: zero stor
  > Update to protobuf 3.5.1 and minor cleanups for Golang 1.10 (cloudfoundry#363)
  > Test with latest Go and protobuf patch versions. (cloudfoundry#352)
  > Merge pull request cloudfoundry#361 from temoto/lint-equal-return
  > Merge pull request cloudfoundry#357 from benesch/populate-recursion
  > example
  > Merge pull request cloudfoundry#355 from agnivade/agnivade-protopath
  > Merge pull request cloudfoundry#341 from meling/master
  > fix for testdata/my_test
  > Merge pull request cloudfoundry#350 from tomwilkie/go_package
  > Merge pull request cloudfoundry#345 from Starnop/master
  > min version 3 for my_test/test.proto
  > Update extensions.md
  > merged 130e6b02ab059e7b717a096f397c5b60111cae74 from golang/protobuf
  > updated descriptor
  > merged 11b8df160996e00fd4b55cbaafb3d84ec6d50fa8 from golang/protobuf
  > merged 17ce1425424ab154092bbb43af630bd647f3bb0d from golang/protobuf
  > merged 5afd06f9d81a86d6e3bb7dc702d6bd148ea3ff23 from golang/protobuf
  > Merge pull request cloudfoundry#343 from gogo/nounsafe
  > Merge branch 'master' of https://github.com/gogo/protobuf
  > fix mixbench
  > fix for issue 330
  > remove unset GOROOT
  > different unset method
  > unset GOROOT
  > update gopherjs
  > go1.9 for gopherjs
  > upgrade descriptor and other includes
  > protoc3.4
  > Merge branch 'master' of https://github.com/gogo/protobuf
  > less go versions
  > upgrade to go1.9
  > nullable = false for defaults as well
  > merged 1909bc2f63dc92bb931deace8b8312c4db72d12f from golang/protobuf
  > merged 748d386b5c1ea99658fd69fe9f03991ce86a90c1 from golang/protobuf
  > merged 0a4f71a498b7c4812f64969510bcb4eca251e33a from golang/protobuf
  > merged 4f95b0d3eab845462f9b0ca5ad21dea8093be211 from golang/protobuf
  > merged 5a0f697c9ed9d68fef0116532c6e05cfeae00e55 from golang/protobuf
  > merged 6e4cc92cc905d5f4a73041c1b8228ea08f4c6147 from golang/protobuf
  > merged 9f174c986221c608fb5143bd623b6076a71feae3 from golang/protobuf
  > merge 7b8002443fd4a3ce5f25ef93087c524546799a56 from golang/protobuf
  > merge 63bfc7087234061252151bf2276a9db446645bb9 from golang/protobuf
  > Fix unmarshal to correctly handle default value in maps (cloudfoundry#318)
  > Fix a race when using TextMarshal on a StdDuration (cloudfoundry#290)
  > another gostring fix for non nullable repeated types
  > fix for issue 312
  > hacks to reproduce a failing GoString test
  > trying to reproduce issue 312
  > Update Readme.md
  > more consistent marshal/unmarshal behavior for types that implement json Marshaler and Unmarshaler interfaces (cloudfoundry#308)
  > Update Readme.md
  > Update Readme.md
  > Update Readme.md
  > remove tags of typedecl=false and add JsonPBMarshalers and Unmarshaler implementations
  > fix:305 only serialize protofields (cloudfoundry#307)
  > Update Readme.md
  > Update Readme.md
  > Merge branch 'master' of https://github.com/gogo/protobuf
  > doc.go file in sizerconflict
  > merged 7a211bcf3bce0e3f1d74f9894916e6f116ae83b4 from golang/protobuf
  > merged a4e8f93323fcf2bc1ae5c036dc4d973872057936 from golang/protobuf
  > upgrade to proto3.3 and merge 157d9c53be5810dd5a0fac4a467f7d5f400042ea and fec3b39b059c0f88fa6b20f5ed012b1aa203a8b4 from golang/protobuf
  > merged 47eb67eaf5cab63c58956d4d4ce86b03ad5eaa03 from golang/protobuf follow the new code generation comment convention
  > merged b50ceb1fa9818fa4d78b016c2d4ae025593a7ce3 from golang/protobuf
  > disable Any with Message for go1.7
  > merged 18c9bb3261723cd5401db4d0c9fbc5c3b6c70fe8 from golang/protobuf
  > Update extensions.md
  > removed go1.6.3 from continious testing
  > Merge branch 'master' of https://github.com/gogo/protobuf
  > install protoc script modified to rather download precompiled protoc for versions which has this available
  > protoc 3.2.0 (cloudfoundry#280)
  > skip unsafe tests for bigendian architectures. (cloudfoundry#272)
  > Compile time warning when both Sizer and Protosizer are used (cloudfoundry#271)
  > Add dual registration plugin (cloudfoundry#270)
  > better mesos-go link
  > fixed mesos-go sample link
  > a real fix for issue261
  > Merge branch 'master' of https://github.com/gogo/protobuf
  > fixes issue 261
  > fix broken build
  > fix for issue 262
  > Remove unnecessary statement (cloudfoundry#263)
  > Add references to some custom imports (cloudfoundry#258)
  > new user protoactor-go
  > Unrendered markdown (cloudfoundry#256)
  > Drop types declaration option (cloudfoundry#250)
  > fix for issue 253 gostring for stdtime and stdduration when nullable=false
Submodule src/github.com/lyft/protoc-gen-validate 000000000...a11cd25ca (new submodule)

Co-authored-by: Sunjay Bhatia <sbhatia@pivotal.io>
@cf-gitbot cf-gitbot added scheduled and removed unscheduled labels Apr 29, 2019
@cf-gitbot cf-gitbot added in progress and removed scheduled labels May 9, 2019
@aminjam

This comment has been minimized.

Copy link
Member

@aminjam aminjam commented May 9, 2019

@cunnie We just looked at this issue and the suggested solution seems reasonable. Would y'all feel comfortable making a PR with the above changes? If not, let's schedule a X-team pair to implement this.

@cf-gitbot cf-gitbot added scheduled and removed in progress labels May 9, 2019
@cunnie

This comment has been minimized.

Copy link
Member Author

@cunnie cunnie commented May 13, 2019

@aminjam : yes, we'll try the PR approach first, and if it gets outside our comfort zone we'll call you for reinforcements.

@cunnie

This comment has been minimized.

Copy link
Member Author

@cunnie cunnie commented May 21, 2019

Success!

We've done a successful spike in which one can cf ssh to both Windows apps and Linux apps with a 3rd-party networking plugin that only works for Linux.

Next Steps

@heyjcollins / @sunjayBhatia Would you like to cross-team pair or would you prefer a PR?

From the included diffs, one can see the the potential changes to the diego repos should not be large. We can potentially take the work of actually implementing this feature with TDD and making sure everything looks good.

What we didn't do:

  • Write any integration / unit tests
  • Modify existing tests

What we did:

Repos that we've touched:

Changes:

diego-release

Note: we'd like to revisit the property name (currently prefer_instance_address); we feel that something along the lines of advertise_preference_for_instance_address is more suitable and more aptly conveys the intention of the property.

diff --git a/jobs/rep/spec b/jobs/rep/spec
index 4ce258dfb..be182fde8 100644
--- a/jobs/rep/spec
+++ b/jobs/rep/spec
@@ -91,6 +91,9 @@ properties:
   diego.rep.listen_addr_securable:
     description: "address where rep listens for LRP and task start auction requests"
     default: "0.0.0.0:1801"
+  diego.rep.prefer_instance_address:
+    description: "boolean to signal the ssh-proxy should attempt to connect directly to the container IP; may be used by third-party container networking plugins"
+    default: false
 
   diego.ssl.skip_cert_verify:
     description: "when connecting over https, ignore bad ssl certificates"
diff --git a/jobs/rep/templates/rep.json.erb b/jobs/rep/templates/rep.json.erb
index e2d5f14ac..8e5dc9133 100644
--- a/jobs/rep/templates/rep.json.erb
+++ b/jobs/rep/templates/rep.json.erb
@@ -45,6 +45,7 @@
     container_proxy_trusted_ca_certs: p("containers.proxy.trusted_ca_certificates"),
     container_proxy_verify_subject_alt_name: p("containers.proxy.verify_subject_alt_name"),
     container_proxy_ads_addresses: p("containers.proxy.ads_addresses"),
+    prefer_instance_address: p("diego.rep.prefer_instance_address"),
     enable_unproxied_port_mappings: p("containers.proxy.enable_unproxied_port_mappings"),
     proxy_memory_allocation_mb: p("containers.proxy.additional_memory_allocation_mb"),
     container_proxy_path: "/var/vcap/packages/proxy",

bbs

Note: due to the nature of protobuf, we only included the relevant changes to the source code, not all of the regenerated files.

diff --git a/models/actual_lrp.proto b/models/actual_lrp.proto
index 09e61a1..7ddd793 100644
--- a/models/actual_lrp.proto
+++ b/models/actual_lrp.proto
@@ -35,6 +35,7 @@ message ActualLRPNetInfo {
   string address = 1 [(gogoproto.jsontag) = "address"];
   repeated PortMapping ports = 2 [(gogoproto.jsontag) = "ports"];
   string instance_address = 3;
+  bool prefer_instance_address = 4;
 }
 
 message ActualLRP {

diego-ssh

diff --git a/authenticators/permissions_builder.go b/authenticators/permissions_builder.go
index 6b06d77..6c822b7 100644
--- a/authenticators/permissions_builder.go
+++ b/authenticators/permissions_builder.go
@@ -62,7 +62,8 @@ func (pb *permissionsBuilder) createPermissions(
 		if mapping.ContainerPort == sshRoute.ContainerPort {
 			address := actual.Address
 			port := mapping.HostPort
-			if pb.useDirectInstanceAddr {
+			//if actual.PreferInstanceAddress {
+			if pb.useDirectInstanceAddr || actual.PreferInstanceAddress {
 				address = actual.InstanceAddress
 				port = mapping.ContainerPort
 			}

executor

diff --git a/depot/containerstore/containerstore.go b/depot/containerstore/containerstore.go
index bc476a7..95115f2 100644
--- a/depot/containerstore/containerstore.go
+++ b/depot/containerstore/containerstore.go
@@ -83,6 +83,7 @@ type containerStore struct {
 	cellID string
 
 	enableUnproxiedPortMappings bool
+	preferInstanceAddress       bool
 }
 
 func New(
@@ -102,6 +103,7 @@ func New(
 	proxyConfigHandler ProxyManager,
 	cellID string,
 	enableUnproxiedPortMappings bool,
+	preferInstanceAddress bool,
 ) ContainerStore {
 	return &containerStore{
 		containerConfig:               containerConfig,
@@ -122,6 +124,7 @@ func New(
 		cellID: cellID,
 
 		enableUnproxiedPortMappings: enableUnproxiedPortMappings,
+		preferInstanceAddress:       preferInstanceAddress,
 	}
 }
 
@@ -153,6 +156,7 @@ func (cs *containerStore) Reserve(logger lager.Logger, req *executor.AllocationR
 			cs.proxyConfigHandler,
 			cs.cellID,
 			cs.enableUnproxiedPortMappings,
+			cs.preferInstanceAddress,
 		))
 
 	if err != nil {
diff --git a/depot/containerstore/storenode.go b/depot/containerstore/storenode.go
index 4acc9b8..185afef 100644
--- a/depot/containerstore/storenode.go
+++ b/depot/containerstore/storenode.go
@@ -78,6 +78,7 @@ type storeNode struct {
 	bindMounts                  []garden.BindMount
 	cellID                      string
 	enableUnproxiedPortMappings bool
+	preferInstanceAddress       bool
 
 	destroying, stopping int32
 
@@ -101,6 +102,7 @@ func newStoreNode(
 	proxyConfigHandler ProxyManager,
 	cellID string,
 	enableUnproxiedPortMappings bool,
+	preferInstanceAddress bool,
 ) *storeNode {
 	return &storeNode{
 		config:                      config,
@@ -122,6 +124,7 @@ func newStoreNode(
 		proxyConfigHandler:          proxyConfigHandler,
 		cellID:                      cellID,
 		enableUnproxiedPortMappings: enableUnproxiedPortMappings,
+		preferInstanceAddress:       preferInstanceAddress,
 	}
 }
 
@@ -391,6 +394,7 @@ func (n *storeNode) createGardenContainer(logger lager.Logger, info *executor.Co
 	info.Ports = n.portMappingFromContainerInfo(containerInfo, info.Ports, proxyPortMapping)
 	info.ExternalIP = containerInfo.ExternalIP
 	info.InternalIP = containerInfo.ContainerIP
+	info.PreferInstanceAddress = n.preferInstanceAddress
 
 	info.MemoryLimit = containerSpec.Limits.Memory.LimitInBytes
 	info.DiskLimit = containerSpec.Limits.Disk.ByteHard
diff --git a/initializer/initializer.go b/initializer/initializer.go
index 8a162cf..8e2803d 100644
--- a/initializer/initializer.go
+++ b/initializer/initializer.go
@@ -142,6 +142,7 @@ type ExecutorConfig struct {
 	VolmanDriverPaths                  string                `json:"volman_driver_paths"`
 	CSIPaths                           []string              `json:"csi_paths"`
 	CSIMountRootDir                    string                `json:"csi_mount_root_dir"`
+	PreferInstanceAddress              bool                  `json:"prefer_instance_address"`
 }
 
 var (
@@ -305,6 +306,7 @@ func Initialize(logger lager.Logger, config ExecutorConfig, cellID string,
 		proxyConfigHandler,
 		cellID,
 		config.EnableUnproxiedPortMappings,
+		config.PreferInstanceAddress,
 	)
 
 	depotClient := depot.NewClient(
diff --git a/resources.go b/resources.go
index 14bae35..e94ff89 100644
--- a/resources.go
+++ b/resources.go
@@ -38,14 +38,15 @@ type Container struct {
 	Guid string `json:"guid"`
 	Resource
 	RunInfo
-	Tags        Tags
-	State       State              `json:"state"`
-	AllocatedAt int64              `json:"allocated_at"`
-	ExternalIP  string             `json:"external_ip"`
-	InternalIP  string             `json:"internal_ip"`
-	RunResult   ContainerRunResult `json:"run_result"`
-	MemoryLimit uint64             `json:"memory_limit"`
-	DiskLimit   uint64             `json:"disk_limit"`
+	Tags                  Tags
+	State                 State              `json:"state"`
+	AllocatedAt           int64              `json:"allocated_at"`
+	ExternalIP            string             `json:"external_ip"`
+	InternalIP            string             `json:"internal_ip"`
+	RunResult             ContainerRunResult `json:"run_result"`
+	MemoryLimit           uint64             `json:"memory_limit"`
+	DiskLimit             uint64             `json:"disk_limit"`
+	PreferInstanceAddress bool               `json:"prefer_instance_address"`
 }
 
 func NewContainerFromResource(guid string, resource *Resource, tags Tags) Container {

rep

diff --git a/conversion_helpers.go b/conversion_helpers.go
index 5dc35c2..78a24de 100644
--- a/conversion_helpers.go
+++ b/conversion_helpers.go
@@ -88,7 +88,7 @@ func ActualLRPNetInfoFromContainer(container executor.Container) (*models.Actual
 		))
 	}
 
-	actualLRPNetInfo := models.NewActualLRPNetInfo(container.ExternalIP, container.InternalIP, ports...)
+	actualLRPNetInfo := models.NewActualLRPNetInfo(container.ExternalIP, container.InternalIP, container.PreferInstanceAddress, ports...)
 
 	err := actualLRPNetInfo.Validate()
 	if err != nil {
@heyjcollins

This comment has been minimized.

Copy link
Member

@heyjcollins heyjcollins commented May 21, 2019

Feel free to comment and reopen if there's more content to capture/share about this but as per our zoom conversation regarding the proposed solution and implementation the Diego team will accept and PR from y'all.

So I'm closing this out! Thanks!

@cf-gitbot cf-gitbot added delivered and removed scheduled labels May 21, 2019
@rosenhouse

This comment has been minimized.

Copy link
Member

@rosenhouse rosenhouse commented May 21, 2019

Possible alternate language for the bosh property description....

advertise that containers managed by this rep are directly accessible on the infrastructure network at their instance address. Components like ssh-proxy or routers may use this when determining how to connect to a container. Set this flag only when using a third-party container-networking solution that provides direct connectivity between containers and VMs.

@cf-gitbot cf-gitbot added accepted and removed delivered labels May 21, 2019
cunnie added a commit to jrussett/diego-release that referenced this issue May 21, 2019
Silk prefers `cf ssh` to Diego cell IP, and that's the default.

Third party network plugins will now have the option to advertise the
instance (container) IP address when they are placed on a separate
overlay network.

Set the `rep` BOSH job property
`advertise_preference_for_instance_address` to true to direct `cf ssh`
to use instance IP address.

This is a more fine-grained version of the `ssh-proxy` BOSH job
property, `connect_to_instance_address`, which, when set to `true`,
directs _all_ `cf ssh` connections to the instance's IP address, which
is the desired behavior when _all_ instances are on the overlay network.

Fine-grain is required for networks where not all instances are on the
overlay networks, for example, networks with a mixture of Linux and
Windows Diego cells.

By setting the preference for instance addresses on each `rep` job,
heterogenous deployments can specify the the best/accessible address on
the BOSH instance group level, allowing Linux cells to elect connecting to
instance addresses and vice versa.

[fixes cloudfoundry#427]

[#166166339](https://www.pivotaltracker.com/story/show/166166339)

Signed-off-by: Brian Cunnie <bcunnie@pivotal.io>
@rosenhouse

This comment has been minimized.

Copy link
Member

@rosenhouse rosenhouse commented Jun 4, 2019

Just for reference, here are code changes made to implement this:

Cara-Sciorilli pushed a commit to greenhouse-org/diego-release that referenced this issue Oct 11, 2019
Submodule src/code.cloudfoundry.org/csishim 409becd0..bb675c6d:
  > Regenerate fakes for CSI v0.3.0
  > switch connection to interface in NewControllerClient
Submodule src/code.cloudfoundry.org/volman bb06b986..be448675:
  > Check for accessibility constraints when discovering CSI plugins
Submodule src/github.com/container-storage-interface/spec 35d9f9d7..2178fdee:
  > Introduce concept of topology to CSI spec.
  > spec: Fix grammatical error
  > spec: Remove trailing spaces.
  > spec: Volume from snapshot with different size
  > spec: Fix SnapshotSource commment
  > spec: Add units for GetCapacityResponse
  > spec: clarify field requirements * fixes cloudfoundry#185
  > Merge pull request cloudfoundry#221 from gnufied/add-node-capacity
  > spec: Probe, s/readiness/ready/
  > spec: replace Readiness with WKT BoolValue
  > lib/go: initial support for protobuf "well known types"
  > spec: ProbeResponse reports readiness
  > spec: fix spelling
  > spec: consistently document intentionally empty messages
  > Merge pull request cloudfoundry#224 from xing-yang/snapshot2
  > Merge pull request cloudfoundry#234 from jdef/pin_golang_protobuf_1.1.0
  > lib/go: regenerate bindings w/ latest golang/protobuf generator
  > make: transitively clean/clobber generated go bindings
  > Merge pull request cloudfoundry#223 from jdef/jdef_clarify_authoring_process
  > address review feedback
  > spec: use MUST when describing CapacityRange field reqs
  > lib: Fix the generated code for CI
  > spec: Some consistency fix on map declarations
  > spec: Remove a reference to VolumeInfo
  > travis: bump golang to 1.9.5
  > Merge pull request cloudfoundry#203 from jieyu/csi_adoption
  > gRPC error code corrections
  > Merge pull request cloudfoundry#210 from saad-ali/bump020release
Submodule src/github.com/golang/protobuf c3cefd437..b4deda097:
  > Merge pull request #591 from golang/master-merge
  > Fix godoc examples for Any (#569)
  > proto, protoc-gen-go: fix vet and gofmt errors for Go 1.10 (#508)
  > proto: add DiscardUnknown function (#498)
  > Revert "fix unnecessary rounding to float64 precision when JSON-marshaling durations (#453)" (#493)
  > fix unnecessary rounding to float64 precision when JSON-marshaling durations (#453)
  > Add godoc badge (#444)
  > jsonpb: fix unmarshal null to value that implements JSONPBUnmarshaler (cloudfoundry#429)
  > update protos to 3.4.1 tag of google/protobuf repo (cloudfoundry#428)
  > Fix interpretation of default bytes literal values (cloudfoundry#427)
  > README: specify syntax in proto example. (cloudfoundry#417)
  > jsonpb: add support for custom resolution of Any messages to/from JSON (cloudfoundry#410)
  > Add tests for Go 1.9 (cloudfoundry#421)
  > changed registered paths for WKT to use proto import path google/protobuf/... (cloudfoundry#412)
  > jsonpb: unmarshal JSON null to nil map/slice instead of empty instance (cloudfoundry#404)
  > jsonpb: unmarshal JSON "null" for WKT (cloudfoundry#394)
  > Remove unnecessary conversions (cloudfoundry#315)
  > Fix Travis config and add status (cloudfoundry#381)
  > Add travis-ci configuration. (cloudfoundry#379)
  > Fix protoc-gen-go tests (cloudfoundry#371)
  > ptypes: sync internal development (cloudfoundry#374)
  > Deserialize JSON NaN, Infinity and -Infinity to corresponding Go values (cloudfoundry#363)
  > Fix JSON marshaling of Any containing object that implements JSONPBMarshaler (cloudfoundry#361)
  > Fix jsonpb to serialize NaN and Infinity as special strings (cloudfoundry#242)
  > Fix doc re. getters for proto3, i.e. getters are now generated on all fields (cloudfoundry#354)
  > Fixed broken tests on pre-go1.8 cloudfoundry#356 (cloudfoundry#359)
  > Fix jsonpb to emit zero values for Map, Slice, Ptr when EmitDefaults=true
  > Support custom JSON (de)serialization in jsonpb
  > protoc-gen-go: update Makefiles for protobuf 3.3.0 (cloudfoundry#348)
  > regenerate pb.go files with protobuf 3.3.0
  > protoc-gen-go: follow the new code generation comment convention (cloudfoundry#329)
  > Simplify the installation step (cloudfoundry#345)
  > jsonpb: unmarshalling of Struct, ListValue, Value, Any and proto2 extensions; and marshalling of ListValue.
  > Merge pull request cloudfoundry#326 from golang/revert-275-go-context-stdlib
  > Update to standard library context package. (cloudfoundry#275)
  > protobuf: Regenerate golden files.
  > Makefile: Make "make test" work on OS X.
  > Merge pull request cloudfoundry#290 from natefinch/fix-import
  > descriptor: rename generated protobuf package on import.
  > jsonpb: merge golang/protobuf#255
  > proto: Add a descriptor subpackage.
  > proto: Fix a Marshal race on messages with extensions.
  > jsonpb: treat `null` JSON values for `Timestamp` and `Duration` as defaults (cloudfoundry#255)
  > Generate pb.go files after golang/protobuf@cb9b777
  > Add a benchmark demonstrating lock contention when Marshaling/Unmarshaling maps.
  > Regenerate pb.go files after golang/protobuf@24f28ae
  > protoc-gen-go: Add Filename to ExtensionDesc.
  > Run 'make generate-test-pbs' in proto/ to regenerate proto3.pb.go testdata
  > Generate new pb.go files.
  > proto: generate Get methods for fields of basic type in proto3
  > Merge pull request cloudfoundry#249 from Huawei-PTLab/master
  > improve error message for duplicate oneof in text parsing PiperOrigin-RevId: 138128520
  > grpc: update generated code version
  > Produce an error when unmarshaling text protos if a oneof field is set more than once. PiperOrigin-RevId: 137859856
  > Fix tests.
  > Add a decoding benchmark for multiple small repeated ints PiperOrigin-RevId: 135693188
  > Fix jsonpb for Go 1.8
  > Unroll DecodeVarint to speed up int32/int64/uint64 slice decoding.
  > Note that Buffer.Unmarshal does NOT reset the destination protobuf.
  > Add Varint decoding benchmarks PiperOrigin-RevId: 135181742
  > Add protobuf benchmarks for varint encoding.
  > proto: In Size, don't double-count the tagcode for structs that implement Marshaler.
  > Merge pull request cloudfoundry#235 from lstoll/lstoll-mod-nanos-correctly
  > Allow t/f and True/False while parsing text protos
  > Change MessageName to check if Message has an XXX_MessageName() method, and use the name returned by that method instead of internal registry when present. PiperOrigin-RevId: 131111087
  > proto.Equal: document map equality and clarify the rule for repeated fields. PiperOrigin-RevId: 130971987
  > Remove conformance binary from repository
  > _conformance: generate proto
  > third_party/golang/protobuf: open source conformance test runner PiperOrigin-RevId: 130532724
  > Fix comment for Buffer.index field
  > Update Go tests for C++ JSON name change
Submodule src/github.com/jeffpak/local-node-plugin 1406f1a0e..3e0ef63d1:
  > Update to CSI v0.3.0

Signed-off-by: Dave Walter <dwalter@pivotal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.