enable private service connect #476

kon-angelo · 2022-07-29T21:35:31Z

How to categorize this PR?

/area control-plane
/kind enhancement
/platform gcp

What this PR does / why we need it:
Allows user to configure PrivateServiceConnect for their VPC. The user needs to specify an IP for the endpoint and to enable the service.

Important notes:

This allows configuration only for accessing GCP services from the endpoint and it is not possible to consume other services from other VPCs (which technically is also a feature under the umbrella of PSC).
The endpoint can be any IP but it a private address makes more sense. In order for the shoot to not be broken this endpoint IP must not collide with (gcp ref):
- Shoot CIDRS (nodes, services, pods)
- Seed service CIDR (because of the default/kubernetes service routing)
- Reverse VPN CIDR ranges

ToDos:

infrastucture unit tests
infrastructure integration tests
Validation unit tests
runtime config validator check

Gotchas:
The endpoint and fwd-rule created for PSC have very strict requirements for their options. Due to an unknown condition (likely bug in Terraform provider-gcp) the fwd-rule will be deleted and recreated on infrastructure reconciliation. This may cause issues with open connections to services. The reason for that is that the labels field of fwd-rule has to be empty (specifically for PSC rules) which TF interprets as nil. However the GCP API stores it as "empty" and when TF reads it back during reconciliation it marks it as a different value that the nil stored in state. Hence it recreates the resource.

Because of the seed restriction that this feature imposes we need to wary of transitions during shoot scheduling, which is why we introduce the shoots/binding subresource in the admission controller. We what that:

For shoots with no spec.seedName on creation we can validate the endpointIP against only the shoot ranges. We also need to prevent scheduler from assigning it to a seed that it may have collisions during update.
For shoots created with spec.seedName, check that it can host this endpoint otherwise prevent creation.
Our landscapes usually have seeds using the same ranges, so an unlucky shoot may not find compatible seeds. The user should then check shoot events via kubectl to see why the scheduling fails.

We can also think about leveraging configValidator (aka the runtime check before infrastructure reconciliation). However because this is too late in the process, the shoot may already have been scheduled in an incompatible seed.

Introduce private service connect
Which issue(s) this PR fixes:
Fixes #465

Special notes for your reviewer:

Release note:

dkistner

As discussed in a private conversation.
At validation time we might not know the Seed (only if the .spec.seedName is set manually in the Shoot resource and not by the gardener-scheduler) which means we cannot reliable validate if cidrs are not clashing.
So I suggest to add an additional check in the configvalidator of the Infrastructure controller to ensure that cidrs are ok. At this point in time the Seed is known and we can early abort (before creating any infrastructure) if the cidrs are clashing.

dkistner · 2022-08-04T12:46:01Z

pkg/apis/gcp/types_infrastructure.go

+
+// PrivateServiceConnect holds the configuration for Private Service Connect endpoints.
+type PrivateServiceConnect struct {
+	Enabled    bool


Doc strings.

dkistner · 2022-08-04T12:46:40Z

pkg/apis/gcp/v1alpha1/types_infrastructure.go

+
+// PrivateServiceConnect holds the configuration for Private Service Connect endpoints.
+type PrivateServiceConnect struct {
+	Enabled    bool   `json:"enabled"`


Doc strings.

dkistner · 2022-08-04T12:50:21Z

pkg/internal/infrastructure/terraform.go

@@ -145,6 +148,19 @@ func ComputeTerraformerTemplateValues(
 		"outputKeys": outputKeys,
 	}

+	if config.Networks.PrivateServiceConnect != nil && config.Networks.PrivateServiceConnect.Enabled {


Why do we need the config.Networks.PrivateServiceConnect.Enabled flag at all? Would it not sufficient to enable a private service connect if the endpoint ip is given?

It would be sufficient but I went for a more explicit enablement. How about this check: if PSC struct it not nil, then both are true:

we consider PSC enabled

endpointIP must not be empty (enforced via validation).

dkistner · 2022-08-04T12:53:22Z

pkg/internal/infrastructure/templates/main.tpl.tf

+// There is an issue currently that TF will always delete and recreate the address during infrastructure reconciliation
+// because it cannot handle the labels properly.


What does this mean for us? Will it always be recreated?

Yes it is recreated in every infra reconciliation. The reason is that between the terraform and the GCP API the labels value is oscillating between an empty map and a nil value. This makes TF believe that the resource is changed and it recreates it which may interrupt open connections.

I tried all the combinations e.g. specifying the labels as empty map, not specifying them at all and none of them stopped this behavior. Also it is not possible to initialise the map with any kind of value because it is prohibited by the GCP API for endpoints used in PSC

dkistner · 2022-08-04T12:54:34Z

docs/usage-as-end-user.md

@@ -62,6 +66,9 @@ networks:
 #   natIPNames:
 #   - name: manualnat1
 #   - name: manualnat2
+#  privateServiceConnect:
+#    enabled: true
+#    endpointIP: 10.252.0.0


General question will be multiple google services accessible via the same endpoint ip? Or do we need for each service we want to connect privately an own endpoint?

All services should be accessible by the endpoint. For some you may need to create a private DNS zone so that clients can be configured to use them properly

dkistner · 2022-08-04T12:59:16Z

pkg/apis/gcp/validation/infrastructure.go

+// - Seed's service CIDRs
+// - The VPC CIDR (the sub of the subnet CIDRs used by workers)
+// - The static ranges used by	reverse VPN (192.168.123.0/24)
+func ValidatePrivateServiceConnect(fldPath *field.Path, infra *apisgcp.InfrastructureConfig, ranges []cidrvalidation.CIDR) field.ErrorList {


I guess it is sufficient to pass the PrivateServiceConnect instead of the entire infra. Also the is enabled check is not necessary as we already checked before if private service connect shall be used.

kon-angelo · 2022-08-23T18:25:08Z

/test

testmachinery · 2022-08-23T18:25:12Z

Testrun: e2e-xwznd
Workflow: e2e-xwznd-wf
Phase: Failed

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| bastion-test        | bastion-test        | Succeeded | 9m9s     |
| infrastructure-test | infrastructure-test | Failed    | 13m5s    |
+---------------------+---------------------+-----------+----------+

kon-angelo · 2022-09-07T16:24:47Z

After discussion we decided to close the PR for now as we are not sure it will have the expected results. We will try to bring #489 first and then consider again about PSC.

asifhasnaincer · 2022-11-07T11:19:50Z

Do we have any timeline to completion of the PSC feature? A ticket is already opened for this feature from our side.

dkistner reviewed Aug 4, 2022

View reviewed changes

kon-angelo added 2 commits August 19, 2022 12:37

enable private service connect

aab8fdd

tests

6117caf

kon-angelo force-pushed the enh/private-service-connect branch from a378d33 to d5d13bf Compare August 23, 2022 15:31

gardener-robot added the size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) label Aug 23, 2022

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 23, 2022

gardener-robot removed the size/l Size of pull request is large (see gardener-robot robot/bots/size.py) label Aug 23, 2022

integration tests

d58dad3

kon-angelo force-pushed the enh/private-service-connect branch from d5d13bf to d58dad3 Compare August 23, 2022 18:22

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 23, 2022

kon-angelo closed this Sep 7, 2022

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 7, 2022

kon-angelo deleted the enh/private-service-connect branch February 29, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable private service connect #476

enable private service connect #476

kon-angelo commented Jul 29, 2022

dkistner left a comment

dkistner Aug 4, 2022

dkistner Aug 4, 2022

dkistner Aug 4, 2022

kon-angelo Aug 9, 2022

dkistner Aug 4, 2022

kon-angelo Aug 9, 2022

dkistner Aug 4, 2022

kon-angelo Aug 23, 2022

dkistner Aug 4, 2022

kon-angelo commented Aug 23, 2022

testmachinery bot commented Aug 23, 2022 •

edited

Loading

kon-angelo commented Sep 7, 2022

asifhasnaincer commented Nov 7, 2022

		// There is an issue currently that TF will always delete and recreate the address during infrastructure reconciliation
		// because it cannot handle the labels properly.

enable private service connect #476

enable private service connect #476

Conversation

kon-angelo commented Jul 29, 2022

dkistner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kon-angelo commented Aug 23, 2022

testmachinery bot commented Aug 23, 2022 • edited Loading

kon-angelo commented Sep 7, 2022

asifhasnaincer commented Nov 7, 2022

testmachinery bot commented Aug 23, 2022 •

edited

Loading