Split implementations from generic code #15

aledbf · 2016-11-11T21:29:02Z

No description provided.

bprashanth

I'm confused about the interface. I thought we wanted to define update/reload methods to which we pass a list of structs that contain all the ep/service/nodeport information required to deserialize into valid nginx/haproxy/gce conf? but this interface only has start/stop etc. Is this just an intermediate step?

bprashanth · 2016-11-11T23:06:34Z

controllers/nginx/README.md

@@ -325,6 +337,12 @@ Amongst others [ELBs in AWS](http://docs.aws.amazon.com/ElasticLoadBalancing/lat
 Please check the [proxy-protocol](examples/proxy-protocol/) example


+## Service Integration


Didn't we decide to delete READMEs for now to avoid drift and confusion?

bprashanth · 2016-11-11T23:09:50Z

controllers/nginx/rootfs/etc/nginx/lua/vendor/lua-resty-http/lua-resty-http-0.09-0.rockspec

@@ -0,0 +1,22 @@
+package = "lua-resty-http"


can you clarify why we need any of the lua stuff?

sure. The lua dependencies are required when you enable custom errors (is not possible to create a request in nginx)

bprashanth · 2016-11-11T23:11:57Z

core/pkg/ingress/controller/controller.go

+
+// Interface holds the methods to handle an Ingress backend
+type Interface interface {
+	Start()


please add godocs. Comments from the other pr still valid, it's weird that this interface doesn't take any arguments or return errors.

Don't we need some sort of Update/Sync() method so the backend can write out a config with new endpoints before reload?

this is the correct interface https://github.com/kubernetes/ingress/pull/15/files#diff-9cef6a58a4eaf42843bd0db5b4a2f018R40

aledbf · 2016-11-15T12:16:42Z

@bprashanth ping

bprashanth

I'd like to iterate on the interface first, left some initial comments. Would be nice if just types.go was in a different commit.

bprashanth · 2016-11-16T16:53:28Z

core/pkg/ingress/types.go

+	DefaultSSLDirectory = "/ingress-controller/ssl"
+)
+
+// Controller holds the methods to handle an Ingress backend


Naming this controller is confusing because it isn't the controller right? it's the backend.
Suggest a comment like:

Backend is the interface between the generic controller and various ingress backends.

bprashanth · 2016-11-16T16:54:01Z

core/pkg/ingress/types.go

+type Controller interface {
+	// Start executes a command to start the backend.
+	// The command must run in foreground.
+	Start()


What does foreground mean in this context? I mean, if I'm a cloudprovider, is Start supposed to just hang?

Do we need both a start and a reload?

Do we need both a start and a reload?

yes because we need to track the status of the backend command execution (usually a reload command in nginx,haproxy or caddy signal the master process to spawn new child and terminate the old ones after the existing connections end)

What does foreground mean in this context? I mean, if I'm a cloudprovider, is Start supposed to just hang?

yes? I'm not sure how to deal in a generic way. Please advice how this should be handled

bprashanth · 2016-11-16T16:54:54Z

core/pkg/ingress/types.go

+	// The command must run in foreground.
+	Start()
+	// Stop stops the backend
+	Stop() error


Please clarify that this is a terminal state, i.e after a stop invocation, you will only get killed not restarted (right? becuase we only call it on sigterm and then have terminationGracePeriodSeconds to clean up after which we get a sigkill).

bprashanth · 2016-11-16T16:59:27Z

core/pkg/ingress/types.go

+	Stop() error
+	// Restart signal the backend with a new configuration file returning
+	// the combined output of Stdout and Stderr
+	Restart(data []byte) ([]byte, error)


You need to be more specific about what the byte arrays are, because byte arrays are confusing in general. Also this method is central to the integration so I'd rather over document it than under document it.

Restart takes a byte array representing the new loadbalancer configuration, and returns a byte array containing any output/errors from the backend. Before returning, the backend must load the configuration in the given array into the loadbalancer and restart it, or fail with an error and message string. If reloading fails, the config should be reverted to the last known good configuration, NOT the given byte array.

2 more questions:

why return a byte array instead of a string?

how do we differentiate failure to reload vs something else like failure to write config to disk or cloudprovider?

why return a byte array instead of a string?

no particular reason.

how do we differentiate failure to reload vs something else like failure to write config to disk or cloudprovider?

I think the reason is irrelevant. The important part is to NOT modify the running configuration file in case of any error.

bprashanth · 2016-11-16T17:00:03Z

core/pkg/ingress/types.go

+	Restart(data []byte) ([]byte, error)
+	// Tests returns the commands to execute that verifies if the configuration file is valid
+	// Example: nginx -t -c <file>
+	Test(file string) *exec.Cmd


This interface is weird, why not just give it the same byte array we give to Restart and have it return an error?

good point. The reason is because the backend (nginx,haproxy,caddy) execute a command to test the proposed new configuration without replacing the existing one (this creates a temporal file)

bprashanth · 2016-11-16T18:10:31Z

core/pkg/ingress/types.go

+
+var (
+	// DefaultSSLDirectory defines the location where the SSL certificates will be generated
+	DefaultSSLDirectory = "/ingress-controller/ssl"


Can we make this a flag (a TODO is fine)? Can you also clarify what certs are generated here exactly? are your user secrets stored here? or is this the default snakeoil that the controller uses?

bprashanth · 2016-11-16T18:10:52Z

core/pkg/ingress/types.go

+	Info() string
+}
+
+// Configuration contains the pieces


Please elaborate

bprashanth · 2016-11-16T18:12:17Z

core/pkg/ingress/types.go

+// Configuration contains the pieces
+type Configuration struct {
+	HealthzURL           string
+	Upstreams            []*Upstream


We either need to:

Be crystal clear about what an upstream is in docs and examples

Replace upstream with something else

Maybe we can just use backends/endpoints/server etc (something more natural in this context)?

bprashanth · 2016-11-16T18:12:57Z

core/pkg/ingress/types.go

+// Upstream describes one or more remote server/s (endpoints) associated with a service
+type Upstream struct {
+	// Secure indicates if the communication with the endpoint must use TLS
+	Secure bool


Why not just call this HSTS?

This indicates the protocol to be used between nginx and the endpoint, not from the user.
I will clarify this

bprashanth · 2016-11-16T18:13:56Z

core/pkg/ingress/types.go

+	// Name represents an unique api.Service name formatted as <namespace>-<name>-<port>
+	Name string
+	// Backends contains the list of endpoints
+	Backends []UpstreamServer


Maybe we can call this endpoints and use backens for upstream?

aledbf · 2016-11-16T19:23:57Z

I'd like to iterate on the interface first, left some initial comments. Would be nice if just types.go was in a different commit.

done. Last commit contains types.go

Edit: I'll wait to finish the interface before refactoring the code :)
(meaning this will not pass the tests)

bprashanth · 2016-11-18T18:00:53Z

core/pkg/ingress/types.go

+
+// BackendInfo returns information about the backend.
+// This fields contains information that helps to track issues or to
+// map the running ingress controller to source code


for all fields in types.go please add a json tag like https://github.com/kubernetes/kubernetes/blob/master/pkg/api/types.go#L80 and mark the ones that are optional as omitempty. Name for example, is that just informational? i.e requires omitempty

Please also add a godoc for every public field or method in this file.

bprashanth · 2016-11-18T18:09:09Z

core/pkg/ingress/types.go

+	FailTimeout int
+}
+
+// Server describes a virtual server


please clarify what a "virtual" server exactly is, or if there's docs somewhere please point to that

bprashanth · 2016-11-18T18:09:23Z

core/pkg/ingress/types.go

+// Server describes a virtual server
+type Server struct {
+	// Name contains the FQDN
+	Name string


why not just call this hostname to avoid confusion with other Name fields?

bprashanth · 2016-11-18T18:09:40Z

core/pkg/ingress/types.go

+	// SSLPassthrough indicates if the TLS termination is realized in the server or in the remote endpoint
+	SSLPassthrough bool
+
+	SSLCertificate string


Is this the entier cert as a string, or a path?

bprashanth · 2016-11-18T18:09:56Z

core/pkg/ingress/types.go

+
+	SSLCertificate string
+	//SSLCertificateKey string
+	SSLPemChecksum string


please clarify how this checksum is computed

bprashanth · 2016-11-18T18:12:45Z

core/pkg/ingress/types.go

+	Backends           []*Backend
+	Servers            []*Server
+	TCPEndpoints       []*Location
+	UPDEndpoints       []*Location


Nit: can you reflow the structs so they're

Configuration Server Location Backend

So people reading the code see a natural progression?

bprashanth · 2016-11-18T18:14:15Z

core/pkg/ingress/types.go

+	HealthzURL         string
+	Backends           []*Backend
+	Servers            []*Server
+	TCPEndpoints       []*Location


can we somehow get rid of these, and use Servers as the one contruct that leads us to endpoitns ? I mean, a server without any virtual host information + protocol=TCP is assumed to contain a localtion for a tcp endpoint, and the same for udp?

What I'd consider clear is if this struct boiled down to a list of servers, each of which contained a list of locations, each of which had a set of endpoints.

bprashanth · 2016-11-18T18:46:37Z

core/pkg/ingress/types.go

+	SecureBackend   bool
+	EnableCORS      bool
+	Path            string
+	Backend         Backend


it's confusing that we have a list of backensd in Configuration as well as a Backend in here.

bprashanth · 2016-11-18T18:56:03Z

core/pkg/ingress/types.go

+	DefaultSSLDirectory = "/ingress-controller/ssl"
+)
+
+// Controller holds the methods to handle an Ingress backend


please add

// TODO (#18): Make sure this is sufficiently supportive of other backends.

Also please add a doc.go in this package with some text description of how to use the interface

bprashanth

Almost there, just a bunch of comment and clarity updates. I think the api needs to be crystal clear, and even though we're iterating I'd like to get a decent first cut so please bear with me :)

bprashanth · 2016-11-19T00:13:44Z

core/pkg/ingress/types.go

+	Repository string `json:"repository"`
+}
+
+// Configuration holds the definition of all the parts required to describe


please finish the sentence, describe a single Ingress, or all ingresses in the cluster?

bprashanth · 2016-11-19T00:15:36Z

core/pkg/ingress/types.go

+	// Setting 0 indicates that the check is performed by a Kubernetes probe
+	MaxFails int `json:"maxFails"`
+
+	FailTimeout int `json:"failTimeout"`


bprashanth · 2016-11-19T00:16:12Z

core/pkg/ingress/types.go

+// SSLPassthroughBackend describes a SSL upstream server configured
+// as passthrough (no TLS termination in the ingress controller)
+type SSLPassthroughBackend struct {
+	Backend `json:"namespace,omitempty"`


bprashanth · 2016-11-19T00:16:40Z

core/pkg/ingress/types.go

+
+// SSLCert describes a SSL certificate to be used in a server
+type SSLCert struct {
+	api.ObjectMeta `json:"metadata,omitempty"`


why embed an api object into a custom type?

bprashanth · 2016-11-19T00:17:33Z

core/pkg/ingress/types.go

+// This fields contains information that helps to track issues or to
+// map the running ingress controller to source code
+type BackendInfo struct {
+	// Name returns the name of the backend


This comment could use some clarification since it's required not optional, what does name imply? does it follow some format, or can it be a random hash/alphanumerics etc

bprashanth · 2016-11-19T00:28:02Z

core/pkg/ingress/doc.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


please add some comments about what this package contains, eg: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/doc.go#L19

bprashanth · 2016-11-19T00:29:53Z

core/pkg/ingress/types.go

+	Path string `json:"path"`
+	// IsDefBackend indicates if service specified in the Ingress
+	// contains active endpoints or not.
+	IsDefBackend bool `json:"isDefBackend"`


Can you explain how the name matches this comment?
IsDefBackend makes me think of default backned, but the comment makes me think of unready endpoints? What should the backend implementation do if IsDefBackend=false?

bprashanth · 2016-11-19T00:30:52Z

core/pkg/ingress/types.go

+	// authentication using an external provider
+	// +optional
+	ExternalAuth authreq.External `json:"externalAuth,omitempty"`
+	// RateLimit describes


please finish the sentence, I guess it describes a rate limit for this specific path?

bprashanth · 2016-11-19T00:31:39Z

core/pkg/ingress/types.go

+	// +optional
+	RateLimit ratelimit.RateLimit `json:"rateLimit,omitempty"`
+	// Redirect describes the redirection this location
+	// +optional


please clarify, if set, the path should redirect to the set location? is this also subject to the ratelimit field?

bprashanth · 2016-11-19T00:33:25Z

core/pkg/ingress/types.go

+	// Name represents an unique api.Service name formatted as <namespace>-<name>-<port>
+	Name string `json:"name"`
+	// This indicates if the communication protocol between the backend and the endpoint is HTTP or HTTPS
+	// Allowing the use of HTTPS


If set to true, the ingress backend uses https for all communication to the endpoints (is this true?) including health checks. The endpoint needs to provide certificates with a valid hostname/ip (right? or is any self signed cert ok? if so please elaborate).

aledbf · 2016-11-19T02:55:35Z

@bprashanth ~~I give up.~~

bprashanth · 2016-11-23T00:18:22Z

Assume you're going to change as part of this pr, let me know if you'd rather send a follow up

aledbf · 2016-11-23T02:05:12Z

Assume you're going to change as part of this pr

Yes and fix the test. This must provide a working controller

(please do not merge)

bprashanth · 2016-11-23T23:58:14Z

Re: your previous comment, tests passed, good to merge?
Please squash the "fix x" commits so we don't end up with a partial revert by mistake

aledbf · 2016-11-24T00:02:41Z

@bprashanth not yet :)
Fixing last issue.

Please squash the "fix x" commits so we don't end up with a partial revert by mistake

ok

porridge · 2016-11-25T11:10:53Z

core/pkg/ingress/controller/controller.go

+
+// Check returns if the nginx healthz endpoint is returning ok (status code 200)
+func (ic GenericController) Check(_ *http.Request) error {
+	res, err := http.Get("http://127.0.0.1:18080/healthz")


@aledbf this seems to be very specific to the nginx-based ingress controller, yet is in core/pkg, which I assume is supposed to be generic?
Could you please explain why this is so?
I don't mean this to be criticism, I'm new to this, and just trying to understand this codebase. Is this an example implementation - to be overriden elsewhere, or are all ingress controller running an nginx internally, or is this meant to go away in the future, or am I completely misunderstanding what "generic" means in this context?

@porridge you are right. This should use an URL defined by the backend

Is this an example implementation

GenericController provides the boilerplate and should not contain specific information about the backend (like nginx). The idea is that if you want to write an ingress controller for XX it should be similar to https://github.com/kubernetes/ingress/blob/master/controllers/nginx/pkg/cmd/controller/nginx.go (only the template part)

I don't mean this to be criticism, I'm new to this, and just trying to understand this codebase.

Please keep doing this

tyranron · 2017-01-10T18:59:39Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

-        proxy_responses        1;
-        proxy_pass             udp-{{ $udpServer.Upstream.Name }};
-    }
-{{ end }}


@aledbf sorry for bumping up old stuff, but I can't figure out why TCP services and UDP services were removed in this commit.
In current master the TCP & UDP services are present in template config and are passed to template rendering, but missed in nginx.tmpl at all.

@tyranron yes, I removed the stream section because it requires changes.
This is the PR I need to open to fix that. https://github.com/kubernetes/ingress/compare/master...aledbf:fix-tcp-stream?expand=1
I will do this after #95 and #123

remove lic from dockerfile

This was referenced Nov 11, 2016

Split implementations from generic code #12

Closed

Update godeps and gce controller #14

Merged

aledbf force-pushed the merge-aledbf branch 2 times, most recently from d263e52 to 969bab6 Compare November 11, 2016 23:14

bprashanth reviewed Nov 11, 2016

View reviewed changes

aledbf force-pushed the merge-aledbf branch 4 times, most recently from 2c6ccda to 1f57a94 Compare November 12, 2016 18:45

aledbf changed the title ~~WIP: Split implementations from generic code~~ Split implementations from generic code Nov 15, 2016

bprashanth reviewed Nov 16, 2016

View reviewed changes

aledbf force-pushed the merge-aledbf branch 2 times, most recently from 2e19988 to 7576f10 Compare November 16, 2016 19:18

aledbf force-pushed the merge-aledbf branch from 7576f10 to f908fb9 Compare November 17, 2016 20:36

bprashanth mentioned this pull request Nov 18, 2016

SSL-enabled Nginx Ingress Controller integrated Hashicorp Vault kubernetes-retired/contrib#2060

Closed

bprashanth reviewed Nov 18, 2016

View reviewed changes

bprashanth mentioned this pull request Nov 18, 2016

Proposal: Replace the IngressResource with something generic kubernetes/kubernetes#31364

Closed

aledbf force-pushed the merge-aledbf branch 4 times, most recently from 5f126ec to 08d4ba4 Compare November 18, 2016 20:00

bprashanth reviewed Nov 19, 2016

View reviewed changes

aledbf closed this Nov 19, 2016

aledbf reopened this Nov 22, 2016

aledbf force-pushed the merge-aledbf branch from 08d4ba4 to 9627d2e Compare November 22, 2016 18:21

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2016

aledbf force-pushed the merge-aledbf branch 5 times, most recently from 049da97 to 807afde Compare November 23, 2016 23:20

aledbf added 4 commits November 23, 2016 21:17

Split implementations from generic code

ed9a416

Remove interface

f2b6274

Add Generic interface

5a8e090

Avoid nginx reloads

42b58e9

aledbf force-pushed the merge-aledbf branch from 81fd98f to df0e3a5 Compare November 24, 2016 00:19

Add e2e boilerplate

16c5800

aledbf force-pushed the merge-aledbf branch from df0e3a5 to 16c5800 Compare November 24, 2016 00:34

aledbf merged commit 850af93 into kubernetes:master Nov 24, 2016

porridge reviewed Nov 25, 2016

View reviewed changes

euank mentioned this pull request Nov 28, 2016

Tag 0.11 not found in repository gcr.io/google_containers/nginx-slim #35

Closed

aledbf deleted the merge-aledbf branch December 21, 2016 00:15

tyranron reviewed Jan 10, 2017

View reviewed changes

EcaterinaGr mentioned this pull request Nov 22, 2018

Regular crash of ngnix ingress controller pods after upgrade to 0.20.0 image #3457

Closed

haoqing0110 referenced this pull request in stolostron/management-ingress Mar 5, 2021

Migrate to IBM Travis (#15)

65ad4fd

haoqing0110 referenced this pull request in stolostron/management-ingress Mar 5, 2021

Merge pull request #15 from open-cluster-management/remove-lic

3f58bb0

remove lic from dockerfile

derek-burdick mentioned this pull request Sep 25, 2021

Worker Segmentation Fault (v0.44.0) #6896

Closed

rikatz mentioned this pull request Dec 23, 2021

fix custom-error-pages file not exist #7728

Merged

9 tasks

Alvaro-Campesino added a commit to Alvaro-Campesino/ingress-nginx-k8s that referenced this pull request Jan 19, 2023

Update CHANGELOG.md (kubernetes#15)

009f44d

Alvaro-Campesino added a commit to Alvaro-Campesino/ingress-nginx-k8s that referenced this pull request Jan 24, 2023

Update CHANGELOG.md (kubernetes#15)

264c9b7

domdomegg mentioned this pull request Jan 28, 2024

DevEx: make live-docs script does not work #10927

Closed

sajith-madhusanka mentioned this pull request Feb 2, 2024

Issues with Ingress Configuration and Client Certificate Transmission #10967

Closed

		@@ -325,6 +337,12 @@ Amongst others [ELBs in AWS](http://docs.aws.amazon.com/ElasticLoadBalancing/lat
		Please check the [proxy-protocol](examples/proxy-protocol/) example


		## Service Integration

Split implementations from generic code #15

Split implementations from generic code #15

Conversation

aledbf commented Nov 11, 2016

bprashanth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Nov 15, 2016

bprashanth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bprashanth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Nov 19, 2016 • edited Loading

bprashanth commented Nov 23, 2016

aledbf commented Nov 23, 2016 • edited Loading

bprashanth commented Nov 23, 2016

aledbf commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Nov 16, 2016 •

edited

Loading

aledbf commented Nov 19, 2016 •

edited

Loading

aledbf commented Nov 23, 2016 •

edited

Loading