Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

implement ingress and ServiceSpec.Ports[].Endpoint #31

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

concaf
Copy link
Collaborator

@concaf concaf commented Jun 19, 2017

This commit allows specifying a root level "ingress" field which
allows specifying an IngressSpec resource, plus a Name field for
the name of the ingress.

This also adds a shortcut "endpoint" in
"services[].ports[].endpoint" which is essentially an extension of
"ServiceSpec.Ports[]" which allows specifying the Ingress' Host
and Path in the form "ingress_host/ingress_path", and an Ingress
resource is automatically generated based on the Service Port.

The name of the resulting Ingress is
"ServiceName-ServicePort.Port"

Fixes #22

ports:
- port: 8080
targetPort: 80
endpoint: minikube.local/foo
ingress:
- name: root-ingress
Copy link
Member

Choose a reason for hiding this comment

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

this is creating two ingresses?

Copy link
Member

Choose a reason for hiding this comment

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

One defined here and one defined that gets auto-generated from the service with endpoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, just for this example, to use the spec.

@surajssd
Copy link
Member

right now running

✘-1 ~/go/src/github.com/surajssd/kapp/examples/wordpress [pr_31 L|✔] 
19:07 $ kapp convert -f db.yaml -f web.yaml | oc create -f -
deployment "database" created
deployment "web" created
Error from server (Invalid): error when creating "STDIN": Service "database" is invalid: spec.ports: Required value
Error from server (Invalid): error when creating "STDIN": Service "wordpress" is invalid: spec.ports: Required value

seems like services are not getting created properly, seems like they are missing ports:

apiVersion: v1
kind: Service
metadata:
  creationTimestamp: null
  labels:
    app: database
  name: database
spec:
  ports: null
  selector:
    app: database
status:
  loadBalancer: {}

@surajssd
Copy link
Member

before sending PR I test this like this right now:

cd configmap
kproject.sh configmap
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..


cd customVol
kproject.sh customVol
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..

cd health
kproject.sh health
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..

cd healthchecks
kproject.sh healthchecks
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..

cd ingress
kproject.sh ingress
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..

cd wordpress
kproject.sh wordpress
kapp convert -f web.yaml -f db.yaml | oc create -f -
cd ..

where kproject.sh is:

$ cat `which kproject.sh `
#!/bin/bash
set -x

kubectl create ns $1
kubectl config set-context minikube --namespace $1

}
ing := &ext_v1beta1.Ingress{

ingressName := s.Name + "-" + strconv.FormatInt(int64(port.Port), 10)
Copy link
Member

Choose a reason for hiding this comment

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

since name is compulsory field do we need to create this name?

Copy link
Member

Choose a reason for hiding this comment

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

now reading code more closely i find out this is necessary!

@surajssd
Copy link
Member

@containscafeine added following diff and the output sevice has ports:

$ git diff
diff --git a/pkg/transform/kubernetes/kubernetes.go b/pkg/transform/kubernetes/kubernetes.go
index 3c820ab..940f69a 100644
--- a/pkg/transform/kubernetes/kubernetes.go
+++ b/pkg/transform/kubernetes/kubernetes.go
@@ -18,9 +18,10 @@ import (
        // install api
        "strings"
 
+       "strconv"
+
        _ "k8s.io/client-go/pkg/api/install"
        _ "k8s.io/client-go/pkg/apis/extensions/install"
-       "strconv"
 )
 
 func getLabels(app *spec.App) map[string]string {
@@ -47,6 +48,10 @@ func createIngresses(app *spec.App) ([]runtime.Object, error) {
 func createServices(app *spec.App) ([]runtime.Object, error) {
        var svcs []runtime.Object
        for _, s := range app.Services {
+               for _, p := range s.Ports {
+                       s.ServiceSpec.Ports = append(s.ServiceSpec.Ports, p.ServicePort)
+               }
+
                svc := &api_v1.Service{
                        ObjectMeta: api_v1.ObjectMeta{
                                Name:   s.Name,

@concaf
Copy link
Collaborator Author

concaf commented Jun 19, 2017

@surajssd fixed, I did the same thing once the svc was generated instead of modifying app.Services[]

               for _, servicePortMod := range s.Ports {
                       svc.Spec.Ports = append(svc.Spec.Ports, servicePortMod.ServicePort)
               }

@surajssd
Copy link
Member

@containscafeine
right now this works for me:

$ curl minikube.external -L | head
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  2578    0  2578    0     0   2571      0 --:--:--  0:00:01 --:--:--  2571<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="en-US" xml:lang="en-US">
<head>
        <meta name="viewport" content="width=device-width" />
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
        <meta name="robots" content="noindex,nofollow" />
        <title>WordPress &rsaquo; Installation</title>
        <link rel='stylesheet' id='buttons-css'  href='http://minikube.external/wp-includes/css/buttons.min.css?ver=4.8' type='text/css' media='all' />
<link rel='stylesheet' id='install-css'  href='http://minikube.external/wp-admin/css/install.min.css?ver=4.8' type='text/css' media='all' />
<link rel='stylesheet' id='dashicons-css'  href='http://minikube.external/wp-includes/css/dashicons.min.css?ver=4.8' type='text/css' media='all' />
100  8069    0  8069    0     0   7988      0 --:--:--  0:00:01 --:--:--  766k
curl: (23) Failed writing body (123 != 3055)

but this does not:

$ curl minikube.local/foo -L
default backend - 404

while both are pointing to the same service?

pkg/spec/spec.go Outdated
api_v1.ServiceSpec `json:",inline"`
type ServicePortMod struct {
api_v1.ServicePort `json:",inline"`
Endpoint string `json:"endpoint"`
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice to have comments in new fields that we are adding describing what they are for

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

There are is a lot that we will have to improve in the way we generate Ingress, but this PR is a good start.
LGTM

@surajssd
Copy link
Member

code LGTM but not sure what is wrong in here!

@kadel
Copy link
Member

kadel commented Jun 20, 2017

I don't understand the problem :-(
can you share ingress that was generated?
Maybe something wrong with your ingress router?

@concaf
Copy link
Collaborator Author

concaf commented Jun 20, 2017

@surajssd - Sigh, after an hour of creating ingresses and namespaces, I think this is what is happening.
minikube.local/foo sends traffic to the wordpress pod, which then redirects to minikube.local/wp-admin/install.php, and the nginx ingress throws a 💩 because it does not have rule for that path, and hence redirects to the default backend.

This is a configuration problem, not related to this PR.

@kadel @surajssd I'm hitting the green button!

This commit allows specifying a root level "ingress" field which
allows specifying an IngressSpec resource, plus a Name field for
the name of the ingress.

This also adds a _shortcut_ "endpoint" in
"services[].ports[].endpoint" which is essentially an extension of
"ServiceSpec.Ports[]" which allows specifying the Ingress' Host
and Path in the form "ingress_host/ingress_path", and an Ingress
resource is automatically generated based on the Service Port.

The name of the resulting Ingress is
"ServiceName-ServicePort.Port"

Fixes kedgeproject#22
@concaf concaf merged commit 9917534 into kedgeproject:master Jun 20, 2017
@surajssd
Copy link
Member

cool

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants