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

Gslb CRD and Controller #4

Closed
wants to merge 6 commits into from
Closed

Gslb CRD and Controller #4

wants to merge 6 commits into from

Conversation

ytsarev
Copy link
Member

@ytsarev ytsarev commented Dec 5, 2019

First iteration of Gslb CRD and Controller.

Important points

  • Gslb instantiates GslbResolver(abstracted coredns deployment)
  • Gslb does not instantiate Ingress objects, instead it gets the list of Ingresses with gslb=true label attached. This way we will decouple Gslb from standard Ingress creation. It will give us freedom to e.g. use helm charts with minimum modifications( if we stick to original plan then we would have to replace Ingresses with Gslb objects in charts which might be painful from maintenance standpoint)
  • Currently we just reflect collected gslb labeled Ingresses in Gslb.Status. Later on we will use this information for a real action, e.g update resolver and external dns

First iteration - Gslb instantiates GslbResolver
Get the ingress list with gslb=true label and put them in Status
Later on we will use hosts from Ingress struct to populate Resolver
zone db
Copy link
Contributor

@donovanmuller donovanmuller left a comment

Choose a reason for hiding this comment

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

Think we just need to discuss how Ingress resources relate to Gslb resources.
The reason the initial design had two distinct Ingress concepts:

  • one for the usual ingress for a service within a single cluster (cluster specific ingress). I.e. the Ingress resource you would create under normal circumstances
  • and another separate/independant Ingress resource that will be created by the GSLB controller specifically to handle ingress for the GSLB host

The reason for the separation is:

  • it is a common in some environments to have a cluster specific host (e.g. servicea.dc-1.host.com) that allows clients to deliberately connect to a service in a specific cluster (e.g. dc-1). This would be the usual Ingress resource created
  • exposing a service via Gslb should be a conscious/explicit decision, where the team has taken into account that their services in fact can support running in a multi datacenter scenario. I.e. not all services would be exposed via GSLB
  • the GSLB service's host (e.g. servicea.cloud.host.com) will typically differ from the cluster specific host (e.g. servicea.dc-1.host.com). Expressing this as annotations etc. becomes complex if using the cluster specific Ingress vs. using the details in the Gslb resource to generate a Ingress specifically for GSLB traffic
  • given that you can specify multiple hosts's and service backend's in an Ingress, does that mean that all hosts's in an Ingress should be exposed via GSLB? If not how would you specify only certain hosts's? Also, in the case of multiple service backend's, does the health of the GSLB service depend on the Pod health of all the Pods for all backend services? It becomes quite difficult where to draw the line in how much you support from a GSLB perspective vs. what is available for a Ingress resource
  • it gives a nice separation between ingress for local traffic and GSLB traffic and deleting/editing the local Ingress does not necessarily affect GSLB traffic
  • it also guards against other ingress solutions (who do not use Ingress resource to describe an ingress endpoint) not being compatible with Oh My GSLB. Like Istio Ingress Gateway, Ambassador API Gateway, Treafik IngressRoute etc.

The GSLB Ingress resource being created specifically, with a one to one relationship with a Gslb resource, by the controller seemed to solve many of the issues above and was seen as almost an implementation detail.

We also initially considered using annotation's to upgrade a Ingress to support GSLB but with the above considerations and the potential of the annotations becoming more and more to add specific Gslb meaning etc. it seemed to make more sense to just use a Gslb resource as the only resource that a user would interact with to expose a service.

Happy to debate though 👍

@ytsarev
Copy link
Member Author

ytsarev commented Dec 6, 2019

@donovanmuller thanks a lot for extensive comment! Now I get it, Gslb is not going to replace standard Ingresses but sit next to them. Makes total sense! I will update the controller logic and api schema.
Regarding the problem of which pod health to monitor I think we need to introduce something like NodeLabelSelector in Gslb resource to explicitly being able to control the health check targets

@donovanmuller
Copy link
Contributor

Regarding the label selector, it could be a option but if we agree that the service backends in the Gslb resource are explicitly added for the GSLB exposed service, then by extension we can assume that all Pods for the referenced backend servers should be used when considering the health of the GSLB service.

Ie. because it's now a choice and by adding the backend services/paths, we are basically agreeing that all Pods Endpoints backed by the services should all be included as part of the health checks.

Make sense? Agree?

@ytsarev
Copy link
Member Author

ytsarev commented Dec 6, 2019

@donovanmuller ok, so instead of LabelSelector we about to rely on Gslb Ingress ServiceName and make the recursive pod checks. It makes sense but this way we will health check exclusively frontend /exposed services only. It is more simple and straightforward meanwhile Readiness check of the frontend pods should be good enough to detect potential backend problem.

@ytsarev
Copy link
Member Author

ytsarev commented Dec 9, 2019

Closing in favour of #5 . Not force pushing to #4 to keep and log useful contextual discussion

@ytsarev ytsarev closed this Dec 9, 2019
@ytsarev ytsarev deleted the gslb branch August 2, 2020 10:21
kuritka added a commit that referenced this pull request Nov 6, 2020
kuritka added a commit that referenced this pull request Nov 6, 2020
kuritka added a commit that referenced this pull request Nov 11, 2020
deploy-full-local-setup

fix localtargets name in documentation

local playground passed

wait

deploy-gslb-operator-14

k8gb terratest, moving to makefile

terratest-pipeline fix #1

terratest-pipeline fix #2

terratest-pipeline fix #3

terratest-pipeline fix #4

terratest-pipeline fix #5

move stable repo location from 	https://kubernetes-charts.storage.googleapis.com to https://charts.helm.sh/stable
see: https://helm.sh/blog/new-location-stable-incubator-charts/

highlight large script sections

fix version

fix version #2

fix version #3

fix version #4

simplify release pipeline, move logic to Makefile

fix terratest by adding image-repo between deploy-local-cluster args

registry.sh #1

registry.sh; installing docker registry always in deploy-full-terratest-setup; remove registry.sh

remove full.sh, remove registry.sh

test; functionality, hide generate, controller gent, manifest

remove GOPATH guardian as GO terraform-test runs clean ubuntu image, no GOLANG image

refactor generate and manifest

move kustomize

run

manager

install

infoblox-secret

retreive version from shell , deploy

fix deploy, remove  kustomize udf

bundle

build-bundle, debug-local

docker-test-build-push

change git-last-commit-hash into shell COMMIT_HASH

lowercase comments

alphabetically sort targets

remove deploy, bundle, bundle-build

remove debug-local and create debug-local-idea instead

refactor debug-local-idea to debug-idea

refactor debug-local-idea to debug-idea
kuritka added a commit that referenced this pull request Nov 11, 2020
deploy-full-local-setup

fix localtargets name in documentation

local playground passed

wait

deploy-gslb-operator-14

k8gb terratest, moving to makefile

terratest-pipeline fix #1

terratest-pipeline fix #2

terratest-pipeline fix #3

terratest-pipeline fix #4

terratest-pipeline fix #5

move stable repo location from 	https://kubernetes-charts.storage.googleapis.com to https://charts.helm.sh/stable
see: https://helm.sh/blog/new-location-stable-incubator-charts/

highlight large script sections

fix version

fix version #2

fix version #3

fix version #4

simplify release pipeline, move logic to Makefile

fix terratest by adding image-repo between deploy-local-cluster args

registry.sh #1

registry.sh; installing docker registry always in deploy-full-terratest-setup; remove registry.sh

remove full.sh, remove registry.sh

test; functionality, hide generate, controller gent, manifest

remove GOPATH guardian as GO terraform-test runs clean ubuntu image, no GOLANG image

refactor generate and manifest

move kustomize

run

manager

install

infoblox-secret

retreive version from shell , deploy

fix deploy, remove  kustomize udf

bundle

build-bundle, debug-local

docker-test-build-push

change git-last-commit-hash into shell COMMIT_HASH

lowercase comments

alphabetically sort targets

remove deploy, bundle, bundle-build

remove debug-local and create debug-local-idea instead

refactor debug-local-idea to debug-idea

refactor debug-local-idea to debug-idea
kuritka added a commit that referenced this pull request Nov 12, 2020
deploy-full-local-setup

fix localtargets name in documentation

local playground passed

wait

deploy-gslb-operator-14

k8gb terratest, moving to makefile

terratest-pipeline fix #1

terratest-pipeline fix #2

terratest-pipeline fix #3

terratest-pipeline fix #4

terratest-pipeline fix #5

move stable repo location from 	https://kubernetes-charts.storage.googleapis.com to https://charts.helm.sh/stable
see: https://helm.sh/blog/new-location-stable-incubator-charts/

highlight large script sections

fix version

fix version #2

fix version #3

fix version #4

simplify release pipeline, move logic to Makefile

fix terratest by adding image-repo between deploy-local-cluster args

registry.sh #1

registry.sh; installing docker registry always in deploy-full-terratest-setup; remove registry.sh

remove full.sh, remove registry.sh

test; functionality, hide generate, controller gent, manifest

remove GOPATH guardian as GO terraform-test runs clean ubuntu image, no GOLANG image

refactor generate and manifest

move kustomize

run

manager

install

infoblox-secret

retreive version from shell , deploy

fix deploy, remove  kustomize udf

bundle

build-bundle, debug-local

docker-test-build-push

change git-last-commit-hash into shell COMMIT_HASH

lowercase comments

alphabetically sort targets

remove deploy, bundle, bundle-build

remove debug-local and create debug-local-idea instead

refactor debug-local-idea to debug-idea

refactor debug-local-idea to debug-idea
kuritka added a commit that referenced this pull request Nov 12, 2020
deploy-full-local-setup

fix localtargets name in documentation

local playground passed

wait

deploy-gslb-operator-14

k8gb terratest, moving to makefile

terratest-pipeline fix #1

terratest-pipeline fix #2

terratest-pipeline fix #3

terratest-pipeline fix #4

terratest-pipeline fix #5

move stable repo location from 	https://kubernetes-charts.storage.googleapis.com to https://charts.helm.sh/stable
see: https://helm.sh/blog/new-location-stable-incubator-charts/

highlight large script sections

fix version

fix version #2

fix version #3

fix version #4

simplify release pipeline, move logic to Makefile

fix terratest by adding image-repo between deploy-local-cluster args

registry.sh #1

registry.sh; installing docker registry always in deploy-full-terratest-setup; remove registry.sh

remove full.sh, remove registry.sh

test; functionality, hide generate, controller gent, manifest

remove GOPATH guardian as GO terraform-test runs clean ubuntu image, no GOLANG image

refactor generate and manifest

move kustomize

run

manager

install

infoblox-secret

retreive version from shell , deploy

fix deploy, remove  kustomize udf

bundle

build-bundle, debug-local

docker-test-build-push

change git-last-commit-hash into shell COMMIT_HASH

lowercase comments

alphabetically sort targets

remove deploy, bundle, bundle-build

remove debug-local and create debug-local-idea instead

refactor debug-local-idea to debug-idea

refactor debug-local-idea to debug-idea
kuritka added a commit that referenced this pull request Nov 13, 2020
deploy-full-local-setup

fix localtargets name in documentation

local playground passed

wait

deploy-gslb-operator-14

k8gb terratest, moving to makefile

terratest-pipeline fix #1

terratest-pipeline fix #2

terratest-pipeline fix #3

terratest-pipeline fix #4

terratest-pipeline fix #5

move stable repo location from 	https://kubernetes-charts.storage.googleapis.com to https://charts.helm.sh/stable
see: https://helm.sh/blog/new-location-stable-incubator-charts/

highlight large script sections

fix version

fix version #2

fix version #3

fix version #4

simplify release pipeline, move logic to Makefile

fix terratest by adding image-repo between deploy-local-cluster args

registry.sh #1

registry.sh; installing docker registry always in deploy-full-terratest-setup; remove registry.sh

remove full.sh, remove registry.sh

test; functionality, hide generate, controller gent, manifest

remove GOPATH guardian as GO terraform-test runs clean ubuntu image, no GOLANG image

refactor generate and manifest

move kustomize

run

manager

install

infoblox-secret

retreive version from shell , deploy

fix deploy, remove  kustomize udf

bundle

build-bundle, debug-local

docker-test-build-push

change git-last-commit-hash into shell COMMIT_HASH

lowercase comments

alphabetically sort targets

remove deploy, bundle, bundle-build

remove debug-local and create debug-local-idea instead

refactor debug-local-idea to debug-idea

refactor debug-local-idea to debug-idea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants