Add port conflict detection to the service validation logic.

[brendandburns]

Part of #974

[smarterclayton]

LGTM

[lavalamp]

Validating this here is racy; it needs to be atomic with the etcd write, or you need to get a lock on an etcd key named according to the port.

I also don't like the precedent of pushing a cluster-wide constraint check into the validation stuff. Validation should (IMO) check formatting, syntax, etc--local constraints, not cluster-wide constraints.

[smarterclayton]

If the validation is mostly to prevent user error, is even the race a net win?

I kind of agree with the sentiment of doing the check above validation.

On Sep 10, 2014, at 7:30 PM, Daniel Smith notifications@github.com wrote:

In pkg/api/validation/validation.go:

}
if labels.Set(service.Selector).AsSelector().Empty() {
    allErrs = append(allErrs, errs.NewFieldRequired("selector", service.Selector))
}

• services, err := lister.ListServices()
  Validating this here is racy; it needs to be atomic with the etcd write, or you need to get a lock on an etcd key named according to the port.

I also don't like the precedent of pushing a cluster-wide constraint check into the validation stuff. Validation should (IMO) check formatting, syntax, etc--local constraints, not cluster-wide constraints.

—
Reply to this email directly or view it on GitHub.

[brendandburns]

I'd like to check this in, as I think this is a net win, as it presents a better experience than the status quo.

We can add things to the etcd registry too, but then we are offloading a bunch of the work into each registry provider.

This way we will have a decent chance of catching most problems no matter what, and if a registry provider wants to make it even better, that's great.

[brendandburns]

also @smarterclayton I don't know how to parse "... doing the check above validation." are you supportive of doing this validation here, or are you supportive of @lavalamp's perspective?

[smarterclayton]

On Sep 10, 2014, at 7:54 PM, brendandburns notifications@github.com wrote:

In pkg/api/validation/validation.go:

}
if labels.Set(service.Selector).AsSelector().Empty() {
    allErrs = append(allErrs, errs.NewFieldRequired("selector", service.Selector))
}

• services, err := lister.ListServices()
  also @smarterclayton I don't know how to parse "... doing the check above validation." are you supportive of doing this validation here, or are you supportive of @lavalamp's perspective?

Leaving validation as inherent validation, and doing the constraint check in the registry before or after his call. The benefit there is that if someone had a registry with transactional secondary indexes they could do a uniqueness check on the index and wouldn't have to pass I a fake service lister to validation.

—
Reply to this email directly or view it on GitHub.

[brendandburns]

Ok, so is the conclusion of both reviewers to not submit this, and to do it in the registry instead?

[lavalamp]

I think it should be done in the registry. I don't think putting it in the validation stuff is a net win, I think it will give people a false sense of security and set a bad precedent. I'm OK if it's still racy while placed in the registry, because that can theoretically be fixed. I think it can never be made race free while located in the validation code; it's just the wrong place for checking cluster-wide constraints.