Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Missing validation for Service.Spec.Ports.TargetPort #9734

Closed
sdminonne opened this issue Jun 12, 2015 · 3 comments
Closed

Missing validation for Service.Spec.Ports.TargetPort #9734

sdminonne opened this issue Jun 12, 2015 · 3 comments
Assignees
@thockin
Labels
priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Milestone
v1.0

Comments

@sdminonne
Copy link
Contributor

Original openshift issue openshift/origin#3143
If Service.Spec.Ports.TargetPort is defined as string there's no validation on this.
The user could write something like ( targetPort with "")

{
   "kind":"Service",
   "apiVersion":"v1",
   "metadata":{
      "name":"redis-master",
      "labels":{
         "name":"redis-master"
      }
   },
   "spec":{
      "ports": [
        {
          "port":6379,
          "targetPort": "6379"
        }
      ],
      "selector":{
         "name":"redis-master"
      }
   }
}

and the final results could be no endpoints for the service.

Some kind of validation with some syntax check should be introduced (for example https://tools.ietf.org/html/rfc6335#section-5.1)

I'm submitting a PR to add some validation.
@sdminonne sdminonne mentioned this issue Jun 12, 2015
Merged
@ArtfulCoder ArtfulCoder added this to the v1.0-candidate milestone Jun 12, 2015
@thockin
Copy link
Member

thockin commented Jun 14, 2015

I see this in validation:

        if sp.TargetPort != util.NewIntOrStringFromInt(0) && sp.TargetPort != util.NewIntOrStringFromString("") {
                if sp.TargetPort.Kind == util.IntstrInt && !util.IsValidPortNum(sp.TargetPort.IntVal) {
                        allErrs = append(allErrs, errs.NewFieldInvalid("targetPort", sp.TargetPort, portRangeErrorMsg))
                }
        }

I guess that validation happens afetr defaulting, so we should never see that case, wo I guess it should be an error.

@roberthbailey roberthbailey added team/master sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 15, 2015
@brendandburns brendandburns added priority/backlog Higher priority than priority/awaiting-more-evidence. kind/friction labels Jun 16, 2015
@brendandburns brendandburns modified the milestones: v1.0, v1.0-candidate Jun 16, 2015
@bgrant0607 bgrant0607 removed their assignment Jun 16, 2015
@thockin thockin self-assigned this Jun 17, 2015
sdminonne added a commit to sdminonne/kubernetes that referenced this issue Jun 22, 2015
@sdminonne
To add validation for service ports when defined as string (fixing is…
4b13faa 
…sue kubernetes#9734)
@bgrant0607
Copy link
Member

@thockin are you working on this?

cc @krousey

@thockin
Copy link
Member

thockin commented Jun 24, 2015

I just tagged you as the 2nd LGTM for #9736

@sdminonne sdminonne mentioned this issue Jun 26, 2015
Merged
@thockin thockin closed this as completed Jun 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thockin thockin

Labels
priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
v1.0
Development

No branches or pull requests
7 participants
@ArtfulCoder @thockin @brendandburns @bgrant0607 @roberthbailey @davidopp @sdminonne