search

cloudfoundry / cf-acceptance-tests

CF Acceptance tests
Apache License 2.0
69 stars 171 forks source link

security_groups/dynamic_asgs.go fails due hardcoded ASG private network ranges #1151

Open renelehmann opened 1 month ago

renelehmann commented 1 month ago

Issue While executing the test for enabled dynamic ASG it creates an ASG with fixed private network ranges and checks the connection to cc via https://cloud-controller-ng.service.cf.internal:9024/v2/info. It does not cover foundations using other IP ranges than these hardcoded private network ranges.

Context cats version: 16.2.0, 16.3.0, latest 16.4.0 With commit https://github.com/cloudfoundry/cf-acceptance-tests/commit/7f50d0b86824dd69ed565c6bf98e03fc0165eac5 the ASG was redefined and the destination 10.0.0.0/0 (which covered our used IPs for cc) has been replaced with 10.0.0./8.

security_groups/dynamic_asgs.go (ASG covers private network ranges only): https://github.com/cloudfoundry/cf-acceptance-tests/blob/v16.4.0/security_groups/dynamic_asgs.go#L153-L166

Possible Fix Please revert this ASG definition to the destination 10.0.0.0/0 like it was before or even more open with 0.0.0.0 without any CIDR. But a better approach would be either:

  1. get the used IPs of all the cc endpoints and define this specific IP destinations on the ASG (e.g. with net.LookupIP and loop trough the range).

or

  1. Introducing a cats-config.json property to define or overwrite the ASG destination range.
ctlong commented 1 month ago

@geofffranks can you please take a look at this issue when you get a chance, thanks.

geofffranks commented 4 weeks ago

10.0.0.0/0 is equivalent to 0.0.0.0/0. Both allow egress to any IPv4 addr on the internet. There are situations where using 10.0.0.0/0 is an invalid cidr when given to the CNI, which is what triggered this change.

Are you running these tests as part of developing CF, or testing an installation? Generally these tests are not good for exercising the health of a cloudfoundry environment. They're very long and in-depth, to validate code changes to CF before CF components + cf-deployment are released. The preferred method for validating deployments is to run the smoke-tests errand.

renelehmann commented 4 weeks ago

Hi @geofffranks , I understood the trigger for the change. The 0.0.0.0 without cidr was only an idea. The best approach would be really to lookup the used IPs for cc and then define the ASG for this test accordingly. As further input we use e.g. 100.x.x.x on our management components. Of course this is then nated from external.

We run the CATS test only on development/integration environments and to test against the packaged cloudfoundry component releases before it will be used as a platform release package in production. Beside that we run further test frameworks (rspecs), also on production environments to ensure all works fine and like expected after an update.

geofffranks commented 4 weeks ago

Oh - are you rolling your own cloudfoundry deployment outside of cf-deployment? Feel free to PR a change to update those ASG blocks to use a variable for a list of network ranges to open on the ASG.

renelehmann commented 3 weeks ago

@geofffranks Don't get me wrong. We use and align to cf-deployment as much and as close as possible. But to have the confidence that all combinations of components and also the specific configurations works fine together the CATS run supports a lot in this manner.

I just saw the comma_delmited_asgs.go would need the same list also.

At the moment I feel uncomfortable to PR the needed changes for the approach with the config input variables and to respect the conventions. Do you see a chance to enhance/add the code with this request? Otherwise I will try, but it probably takes me some time since I don't use to code on a daily base ;-)

geofffranks commented 3 weeks ago

Honestly it wouldn't be a high priority for our engineering team at the moment, but it might be something that we'd get to eventually. If you're looking for a speedy fix, a PR is probably the way to go.