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.

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

Ingress resource V0.2 #14459

Merged

Conversation

bprashanth
Copy link
Contributor

Eg:

apiVersion: v1
kind: Ingress
metadata:
  name: lbconfig
spec:
  backend:
    serviceName: "foo"
    servicePort: 80
  rules:
  - host: foo.bar.com
    http:
      paths:
      - path: "/foo"
        backend:
          serviceName: "foo"
          servicePort: 80

One can create an Ingress with either a default backend, ingress rules, or both but not neither.
Kubectl get ing:

NAME      RULE          BACKEND   ADDRESS
echomap   -             foo:80    0.0.0.0
          foo.bar.com   
          /foo          foo:80
          /bar          bar:8080
          bar.baz.com   
          /baz[.*]      foo:80
          /b?ar         bar:8080
lbmap     -             foo:80    0.0.0.1
          foo.bar.com   
          /foo          foo:80
          /bar          bar:8080
          bar.baz.com   
          /baz[.*]      foo:80
          /b?ar         bar:8080

#13947 #12827 #561

@bprashanth bprashanth changed the title Ingress resource Ingress resource V0.2 Sep 24, 2015
@k8s-bot
Copy link

k8s-bot commented Sep 24, 2015

GCE e2e build/test passed for commit 692dad070cd04054feebd5266347df82b8a63745.

@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 24, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@k8s-bot
Copy link

k8s-bot commented Sep 24, 2015

GCE e2e build/test passed for commit 0f9812b26a3f6a27eac2aea6593607e7ea1920eb.

@k8s-bot
Copy link

k8s-bot commented Sep 24, 2015

GCE e2e build/test passed for commit 3c2c0d246abaaaddc25eb4863078d3f1dd9503e3.

Backend IngressBackend `json:"backend"`
}

// IngressBackend describes all endpoints for a given Service, port and protocol.
// IngressBackend describes all endpoints for a given Service and port.
type IngressBackend struct {
// Specifies the referenced service.
ServiceRef api.LocalObjectReference `json:"serviceRef"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalObjectReference has one field - Name. We have mixed conventions on this. Some places that reference another object in the same namespace say "FooName string" and others say "FooRef LocalObjectreference".

I prefer FooName for terseness.

@bgrant0607 I don't want you do to a full API review, just answer this one point where I argue with myself. ServiceRef or ServiceName?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for Ref > Name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServiceName is fine if you don't expect to extend this to refer to services in other namespaces or to other kinds of resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref = another object in api (regardless of typeof(ref)), that seems clear to me from a usability perspective. I initially had it as ServiceName string, but in theory we could grow the local object ref, or create another object ref to contain more information (like object meta?) and that would be useful here.

Don't feel strongly eitherway, so defer back to @thockin for final call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided on just ServiceName for now, to reduce yaml chatter

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2015
@karlkfi
Copy link
Contributor

karlkfi commented Sep 25, 2015

I'm not clear why we need multiple hosts per ingress. Why not just make a new ingress? The only thing they share is a name, and they could easily share a label instead.

Given that the rules are ordered, you don't need an explicit default service ref. Just put one at the end of the rule list with a nil or "/" path so that it catches everything not caught by previous rules.

It seems like we could flatten and simplify the API significantly to improve UX. How about:

type IngressSpec struct {
    Host string // hostname[.domain][:port]
    Rules []IngressRule
}

type IngressRule struct  {
    Path *string // optional. if nil, catches all paths (or isn't http and doesn't have a path)
    Service IngressServiceFacet
}

type IngressServiceFacet struct {
    api.LocalObjectReference // anonymous field for transparent composition
    Port util.IntOrString
}

Example usage:

apiVersion: v1
kind: Ingress
metadata:
  name: foobar
spec:
  host: foo.bar.com
  rules:
  - path: "/foo"
    service:
      name: "foo"
      port: 80
  - path: "/bar"
    service:
      name: "bar"
      port: 80
  - path: "/"
    service:
      name: "baz"
      port: 80

@bprashanth
Copy link
Contributor Author

I'm not clear why we need multiple hosts per ingress. Why not just make a new ingress? The only thing they share is a name, and they could easily share a label instead.

Atomic updates for all urls associated with a site, blue green deployment type thing for maps.google.com, apis.google.com and so on (some discussion on #561).

Given that the rules are ordered, you don't need an explicit default service ref. Just put one at the end of the rule list with a nil or "/" path so that it catches everything not caught by previous rules.

I mean yeah, you can say everyone should have a catchall rule, but:

  • tcp services shouldn't need that
  • you still want a way to say hit this backend, even if all my certs are messed up (we haven't introduced certs yet). With TLS you can't get the "/"

It seems like we could flatten and simplify the API significantly to improve UX. How about:

Yeah we discussed something like this in #13947 (comment), tldr being we want a resource that addresses Ingress, not just HTTP L7 Ingress, so it needs to be usable for defining L4, L7 and any other usecase that comes up that might or might not need paths (hence the IngressRule -> HTTPIngressRule bit). Things like SNI at L4 get a lot harder to think about if you have a flat url based structure. The eventual goal (hopefully eventual == 1.2) is to dovetail the TCP loadbalancer stuff into this. In theory you can have just one resource that has a mix of tcp and http rules, just like a off the shelf loadbalancer.

@k8s-bot
Copy link

k8s-bot commented Sep 25, 2015

GCE e2e build/test passed for commit 1c842524234c42f2cc5d31688ec3d6776c91cdad.

@bprashanth
Copy link
Contributor Author

@karlkfi We essentially have your resource, the default backend is optional. The reason we don't have rules going straight to a list of paths is #14459 (comment) (i.e we might, before this leaves experimental).

@karlkfi
Copy link
Contributor

karlkfi commented Sep 25, 2015

Working through the alternate API proposal above, I got the feeling that the naming doesn't match the structure.

An API resource called "Ingress" sounds like it should be from the perspective of a single Service. It's so much easier to talk about "Ingress routes to a Service", rather than the way the PR proposes, which is a many-to-many relationship or one-to-many relationship from Host(s) to Service Ports.

I feel like the proposed structure (both mine and the PRs) is more closely describing a set of edge router rules where the router has a Host (like a static IP or dns) and can route/proxy port/paths permutations to different service ports.

It seems like either the name should change to match the structure (e.g. Route or Router) or the structure should change to match the name (many-to-one Hosts to Service).

func deleteIngress(expClient client.ExperimentalInterface, ns string) error {
items, err := expClient.Ingress(ns).List(labels.Everything(), fields.Everything())
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't yet have a flag, do not return an error if you get a 404

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's either all or nothing, i mean the apiserver either has all experimental resources (exp=true) or doesn't. We're only in trouble if the controller manager is started with one of the exp flags and the apiserver isn't, in which case the autoscaler itself will fail. We have disjoint sets of flags that denote the same thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is fine. I couldn't tell easily on phone if this was in the
experimental if block. Thanks!

On Monday, September 28, 2015, Prashanth B notifications@github.com wrote:

In pkg/controller/namespace/namespace_controller.go
#14459 (comment)
:

@@ -478,3 +482,17 @@ func deleteDeployments(expClient client.ExperimentalInterface, ns string) error
}
return nil
}
+
+func deleteIngress(expClient client.ExperimentalInterface, ns string) error {

  • items, err := expClient.Ingress(ns).List(labels.Everything(), fields.Everything())
  • if err != nil {
  •   return err
    

Hmm, it's either all or nothing, i mean the apiserver either has all
experimental resources (exp=true) or doesn't. We're only in trouble if the
controller manager is started with one of the exp flags and the apiserver
isn't, in which case the autoscaler itself will fail. We have disjoint sets
of flags that denote the same thing.


Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/14459/files#r40623670.

@bprashanth
Copy link
Contributor Author

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Sep 29, 2015

Unit, integration and GCE e2e test build/test passed for commit be094eaa976c454c63ba0525c16aa3f1827f69db.

@bprashanth
Copy link
Contributor Author

Tim, trap is green

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2015
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2015
@k8s-bot
Copy link

k8s-bot commented Sep 30, 2015

Unit, integration and GCE e2e test build/test passed for commit c148332.

@bprashanth bprashanth removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2015
@k8s-github-robot
Copy link

LGTM was before last commit, removing LGTM

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2015
@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 2, 2015

Unit, integration and GCE e2e test build/test passed for commit c148332.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 2, 2015
@k8s-github-robot k8s-github-robot merged commit e330b11 into kubernetes:master Oct 2, 2015
a-robinson added a commit that referenced this pull request Oct 5, 2015
…4459-upstream-release-1.1

Automated cherry pick of #14459
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…pick-of-#14459-upstream-release-1.1

Automated cherry pick of kubernetes#14459
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…pick-of-#14459-upstream-release-1.1

Automated cherry pick of kubernetes#14459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants