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

Make cfdot actual-lrps consistent #64

Conversation

MarcPaquette
Copy link
Contributor

What is this change about?

cfdot is not reporting empty host_tls_proxy_port when C2c is utilized

  1. Deploy an app on CF with c2c networking enabled
  2. ssh into the diego cell that the app is running on
  3. Run cfdot actual-lrps

In the port section, each array entry should have 4 attributes. If there's no correpsonding port, it should list 0 as the port.

 "ports": [
    {
      "container_port": 8080,
      "host_port": 61004,
      "container_tls_proxy_port": 61001,
      "host_tls_proxy_port": 61006
    },
    {
      "container_port": 8080,
      "host_port": 61004,
      "container_tls_proxy_port": 61443,
      "host_tls_proxy_port": 0
    },
    {
      "container_port": 2222,
      "host_port": 61005,
      "container_tls_proxy_port": 61002,
      "host_tls_proxy_port": 61007
    }
  ],

For the C2CTLS port (61443), there is no entry for host_tls_proxy_port. This should be set to 0 in all cases (regardless of whether the values of the containers.proxy.enable_unproxied_port_mappings and containers.proxy.require_and_verify_client_certificates properties are set)

{
  "process_guid": "4a295375-84d5-4017-8e14-97cc08fef7e6-f257d4b3-e810-4ad9-bfbe-f493dcddddb0",
  "index": 0,
  "domain": "cf-apps",
  "instance_guid": "6f042458-693c-456a-6fa6-ed96",
  "cell_id": "e0c33815-b521-4e83-bc43-a8fb41d29ea7",
  "address": "10.0.1.14",
  "ports": [
    {
      "container_port": 8080,
      "host_port": 61000,
      "container_tls_proxy_port": 61001,
      "host_tls_proxy_port": 61002
    },
    {
      "container_port": 8080,
      "host_port": 61000,
      "container_tls_proxy_port": 61443
    },
    {
      "container_port": 2222,
      "host_port": 61001,
      "container_tls_proxy_port": 61002,
      "host_tls_proxy_port": 61003
    }
  ],

What problem it is trying to solve?

This change will report host_tls_proxy_port when it is empty as well as populated

How should this change be described in diego-release release notes?

The bbs now correctly reports empty port assignments when using C2C networking

Please provide any contextual information.

Tracker story for private repo183842469

1.  Deploy an app on CF
2.  ssh into the diego cell that the app is running on
3. Run `cfdot actual-lrps`

In the port section, each array entry should have 4 attributes. If there's no correpsonding port, it should list `0` as the port.
```
 "ports": [
    {
      "container_port": 8080,
      "host_port": 61004,
      "container_tls_proxy_port": 61001,
      "host_tls_proxy_port": 61006
    },
    {
      "container_port": 8080,
      "host_port": 61004,
      "container_tls_proxy_port": 61443,
      "host_tls_proxy_port": 0
    },
    {
      "container_port": 2222,
      "host_port": 61005,
      "container_tls_proxy_port": 61002,
      "host_tls_proxy_port": 61007
    }
  ],
```

For the C2CTLS port (61443), there is no entry for `host_tls_proxy_port`. This should be set to 0 in all cases (regardless of whether the values of the `containers.proxy.enable_unproxied_port_mappings` and `containers.proxy.require_and_verify_client_certificates` properties are set)
```
{
  "process_guid": "4a295375-84d5-4017-8e14-97cc08fef7e6-f257d4b3-e810-4ad9-bfbe-f493dcddddb0",
  "index": 0,
  "domain": "cf-apps",
  "instance_guid": "6f042458-693c-456a-6fa6-ed96",
  "cell_id": "e0c33815-b521-4e83-bc43-a8fb41d29ea7",
  "address": "10.0.1.14",
  "ports": [
    {
      "container_port": 8080,
      "host_port": 61000,
      "container_tls_proxy_port": 61001,
      "host_tls_proxy_port": 61002
    },
    {
      "container_port": 8080,
      "host_port": 61000,
      "container_tls_proxy_port": 61443
    },
    {
      "container_port": 2222,
      "host_port": 61001,
      "container_tls_proxy_port": 61002,
      "host_tls_proxy_port": 61003
    }
  ],
```
Copy link
Contributor

@notrepo05 notrepo05 left a comment

Choose a reason for hiding this comment

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

I'm not an expert on this, but the removal of omitempty does seem to satisfy the requirements

Copy link
Contributor

@dsabeti dsabeti left a comment

Choose a reason for hiding this comment

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

LGTM

@geofffranks geofffranks merged commit e9001b4 into cloudfoundry:main Apr 18, 2023
winkingturtle-vmw added a commit to cloudfoundry/rep that referenced this pull request Apr 20, 2023
winkingturtle-vmw added a commit to cloudfoundry/rep that referenced this pull request Apr 20, 2023
winkingturtle-vmw added a commit to cloudfoundry/rep that referenced this pull request Apr 20, 2023
* Fix tests

Context: cloudfoundry/bbs#64

* Add distributed tracing.

Signed-off-by: Brandon Roberson <broberson@vmware.com>
Signed-off-by: Nick Rohn <nrohn@vmware.com>

* Remove logger from EvacuationHandler

there seems to be no code-path that would do any logging for
evacuation_handler

---------

Signed-off-by: Brandon Roberson <broberson@vmware.com>
Signed-off-by: Nick Rohn <nrohn@vmware.com>
winkingturtle-vmw added a commit to cloudfoundry/route-emitter that referenced this pull request Apr 20, 2023
geofffranks pushed a commit to cloudfoundry/route-emitter that referenced this pull request Apr 20, 2023
dsabeti pushed a commit that referenced this pull request Aug 9, 2023
This is a follow up commit to:
- #64
- e9001b4

In the original PR we accidentally modified a file that is automatically
generated from the proto definition instead of modifying the proto
defintion itself.

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

Signed-off-by: David Sabeti <sabetid@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants