Remove the check for the clone cluster name. #270
Conversation
Clone cluster name may not adhere to the same rules as the names of the clusters managed by the operator, as it might be just a name inside the S3 bucket.
|
👍 |
|
👎 tests are still failing |
|
This means anyone able to deploy a manifest can clone from any cluster's backups, and thus read all the data in other clusters. Just checking: does this break any currently existing privilege boundaries? |
|
It pretty much depends on how different teams are separated on the infrastructure level; if all of them share the backup location there is no way this PR makes things worse (as one could, for instance, just restore the backup of other team to your own cluster manually w/o any assistance from the operator) |
|
👍 |
- check that the normal and clone cluster name complies with the valid service name. For clone cluster, only do it if clone timestamp is not set; with a clone timestamp set, the clone name points to the S3 bucket - add tests and improve existing ones, making sure we don't call Error() method for an empty error, as well as that we don't miss cases where expected error is not empty, but actual call to be tested does not return an error.
|
👍 |
| if err == nil { | ||
| tmp2.Spec.ClusterName = clusterName | ||
| } else { | ||
| if clusterName, err := extractClusterName(tmp2.ObjectMeta.Name, tmp2.Spec.TeamID); err != nil { |
There was a problem hiding this comment.
will this code now support restoring from S3 where the cluster name is basically: "foobar" (spilo ec2 deployments)
There was a problem hiding this comment.
That line doesn't validate the clone cluster name, it happens at https://github.com/zalando-incubator/postgres-operator/pull/270/files/04d6a7607508fef669faacfb57ef9ef443b587da#diff-44fe837275fc3b64cc6c85eb90ce74c9R247
There, the name for the cluster clone that comes from S3 is not validated.
| @@ -43,14 +43,29 @@ var clusterNames = []struct { | |||
| {"acid-test", "acid", "test", nil}, | |||
There was a problem hiding this comment.
seems like "foobar" is not covered but should work as this is team "foo" and cluster "bar" in ec2 spilo way
There was a problem hiding this comment.
@Jan-M which test do you have in mind? We do test with exactly 'foobar' as a clone cluster name for cloning from basebackup in line 66 of this patch.
pkg/spec/postgresql.go
Outdated
| return "", fmt.Errorf("name cannot be longer than %d characters", clusterNameMaxLength) | ||
| } | ||
| if !serviceNameRegex.MatchString(clusterName) { | ||
| return "", fmt.Errorf("name must confirm to a valid service name (DNS-1035)") |
There was a problem hiding this comment.
RFC 1035 is a 50+ pages document. Can you please specify the section that provides overview of the naming conventions and thus the regexp ?
| func validateCloneClusterDescription(clone *CloneDescription) error { | ||
| // when cloning from the basebackup (no end timestamp) check that the cluster name is a valid service name | ||
| if clone.ClusterName != "" && clone.EndTimestamp == "" { | ||
| if !serviceNameRegex.MatchString(clone.ClusterName) { |
There was a problem hiding this comment.
Do existing cluster clones always have names adhering to this format ? that should be enforced by k8s, right ?
| } else if err := validateCloneClusterDescription(&tmp2.Spec.Clone); err != nil { | ||
| tmp2.Error = err | ||
| tmp2.Status = ClusterStatusInvalid | ||
| } else { |
There was a problem hiding this comment.
i feel the summary of this discussion is worth a comment here:
anyone able to deploy a manifest can clone from any cluster's backups, and thus read all the data in other clusters.
It pretty much depends on how different teams are separated on the infrastructure level;
i.e. it is outside of the operator scope to check this
pkg/spec/postgresql.go
Outdated
| @@ -76,6 +77,11 @@ const ( | |||
| ClusterStatusInvalid PostgresStatus = "Invalid" | |||
| ) | |||
|
|
|||
| const ( | |||
| serviceNameMaxLength = 63 | |||
| continue | ||
| } else if tt.err != nil { | ||
| t.Errorf("Expected error: %v", tt.err) |
|
👍 |
1 similar comment
|
👍 |
Clone cluster name may not adhere to the same rules as the names of the
clusters managed by the operator, as it might be just a name inside the
S3 bucket.