pkg/asset/installconfig/aws: public DNS validation #5189

jhixson74 · 2021-09-02T21:29:03Z

Verify no DNS records exist in the public DNS zone. This helps to prevent
installing into an existing cluster. The goal here is to not leak public DNS
records. This should help make the race condition less likely.

https://issues.redhat.com/browse/CORS-1195

wking · 2021-09-12T23:14:58Z

Previously, an existing public A record would cause an install failure when Terraform tried to create the public A record and failed on the collision. With this PR's precheck, you'll fail a bit faster. But the overall result will be similar. If you want to avoid the leak entirely, you'll need to reverse ae9cbaf (#1508), and start claiming the private record before you claim the public record. That way, the deletion logic will find the private record and know it can safely remove the public record. It will be a bit racy, although your new precheck will limit the exposure. You'll be vulnerable to:

Install A's precheck looks for the public record, confirms it doesn't exist.
Install B creates the public record.
Install A creates the private record.
Install A tries to create the public record, but fails because B won the race.
Attempting to remove cluster A breaks cluster B by removing its public record (which A thought it owned, because of the private record from step 3).

It's possible you could move the precheck into a Terraform data request or some such, to reduce the racy gap between step 1 and step 4. But even with your Go-side precheck, the gap may be small enough that we can say "probably won't happen too often, and we can manually restore B's public A record if it does" and decide that that risk is less annoying than the current A record leak. I personally don't have much opinion on the annoyance level of leaking public records on deletion vs. removing public records that really belong to one cluster when deleting a separate cluster. Parallel installs using the same name seems like something that will probably never happen for production clusters.

jhixson74 · 2021-10-20T16:10:25Z

/test e2e-aws

staebler

I am good with this change, on its merits. This aligns with the small window of vulnerability that we have with BYO hosted zone. And both those vulnerabilities seem acceptably rare and shoot-yourself-in-the-foot enough to me.

pkg/asset/installconfig/aws/validation.go

jhixson74 · 2021-10-28T00:13:21Z

/test e2e-aws

pkg/asset/installconfig/aws/validation.go

data/data/aws/cluster/route53/base.tf

Verify no DNS records exist in the public DNS zone. This helps to prevent installing into an existing cluster. The goal here is to not leak public DNS records. This should help make the race condition less likely. https://issues.redhat.com/browse/CORS-1195

staebler

/lgtm
/approve

openshift-ci · 2021-12-15T02:58:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~data/data/aws/OWNERS~~ [staebler]
~~pkg/asset/installconfig/aws/OWNERS~~ [staebler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-12-15T03:08:52Z

/retest-required