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

KEP 18 - Simplify installer - do not install istio nor nginx-ingress #18

Merged
merged 8 commits into from Jun 30, 2020
Merged

KEP 18 - Simplify installer - do not install istio nor nginx-ingress #18

merged 8 commits into from Jun 30, 2020

Conversation

christian-kreuzberger-dtx
Copy link
Member

This Draft provides some information on why we should simplify the Keptn installer to no longer install istio nor nginx-ingress.

FYI, I believe that implementing this KEP will make the following Keptn Issues obsolete:

Please consider the open questions on the bottom of the markdown file

@christian-kreuzberger-dtx christian-kreuzberger-dtx changed the title Simplify installer - no more istio and nginx Simplify installer - do not install istio and nginx Jun 18, 2020
@christian-kreuzberger-dtx christian-kreuzberger-dtx changed the title Simplify installer - do not install istio and nginx Simplify installer - do not install istio nor nginx-ingress Jun 18, 2020
@christian-kreuzberger-dtx christian-kreuzberger-dtx changed the title Simplify installer - do not install istio nor nginx-ingress KEP 18 -ö Simplify installer - do not install istio nor nginx-ingress Jun 18, 2020
@christian-kreuzberger-dtx christian-kreuzberger-dtx changed the title KEP 18 -ö Simplify installer - do not install istio nor nginx-ingress KEP 18 - Simplify installer - do not install istio nor nginx-ingress Jun 18, 2020
@jetzlstorfer
Copy link
Member

Copy link
Member

@agrimmer agrimmer left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm! I only have two minor comments.

text/0018-simplify-installer.md Outdated Show resolved Hide resolved
* Installer: Do not set `KEPTN_DOMAIN`
* Installer: Do not generate SSL certificates

* CLI/API: Change `keptn configure bridge` no longer provide the flag `action=[expose | lockdown]`, but a flag to configure the service-type [user and password should stay as they are, they are still required]
Copy link
Member

Choose a reason for hiding this comment

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

I would propose that neither the CLI nor the API can change the service type because this should be handled by an administrator.
The administrator has then the rights to execute e.g.
kubectl patch svc api-gateway-nginx -n keptn -p '{"spec": {"type": "LoadBalancer"}}'
However, configure bridge should allow to set the basic auth password.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would propose that neither the CLI nor the API can change the service type because this should be handled by an administrator.

I agree with that proposal. We should document the command, and remove the funcionality from our API.

text/0018-simplify-installer.md Outdated Show resolved Hide resolved
@christian-kreuzberger-dtx christian-kreuzberger-dtx added the next-sprint Items that should be discussed and implemented in the next sprint label Jun 23, 2020
* Helm-Service will break without `KEPTN_DOMAIN` - instead we should ask the user to supply this configuration using a new environment variable called `INGRESS_HOSTNAME_SUFFIX` which we will default to `svc.cluster.local`
* Helm-Service will need to check if istio is available before applying any virtualservices
* Not sure, but OpenShift support might break and we need to fix certain things
* Dynatrace-service will break as it also relies on `KEPTN_DOMAIN` - especially for problem notifications
Copy link
Member

Choose a reason for hiding this comment

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

In addition to Problem notifications, the KEPTN_COMAIN is currently used during the dashboard setup (to provide links to the onboarded services, and to the bridge), as well as to provide deep links to the bridge in dynatrace events.

For the first two cases, we could pass the URL of the API/Bridge as parameter(s) for the configure monitoring command.
For adding the deep links to the bridge in deployment events, we will have to rely on the INGRESS_HOSTNAME_SUFFIX env var (as we also do in the helm-service)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for bringing this up, I've added it to the KEP.
Basically, with the proposed change in this KEP deep links to Keptn Bridge will not work anymore, as Keptn does not expose Bridge via a VirtualService or alike.

bacherfl
bacherfl previously approved these changes Jun 23, 2020
Copy link
Member

@bacherfl bacherfl left a comment

Choose a reason for hiding this comment

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

lgtm

@grabnerandi
Copy link
Member

Hi. I would like to know what it takes for a user to get Keptn running on e.g: GKE, Microk8s and k3s.
I think the goal must be to make this as simple as possible - and - describing this in the doc and showing the commands that need to be executed would help understand how the "end user experience" would look like

Here is an example. Assume we have a GKE cluster, an nginx-ingress and I am using Istio that is offered with GKE. What are the recommended installation commands to install Keptn? is it somethign like this?

keptn install
kubectl apply -f keptn_bridge_ingress.yaml
kubectl apply -f keptn_api_inrgress.yaml

So - for me to accept this proposal I would like to know what the user needs to execute in order to cover the most common use cases we have seen. Maybe this is already explained in the doc - but - it would be helpful to add a section with "How to install Keptn on these common deployment scenarios"

@christian-kreuzberger-dtx
Copy link
Member Author

Thanks Andi, I'll add a section that describes what the installation feels like for the user in terms of a full installation.

@christian-kreuzberger-dtx
Copy link
Member Author

I've added instructions for GKE, OpenShift and K3s.

@checkelmann
Copy link

Hi,

I understand the need to simplify the installer and that not every flavor of different ingress and virtual service types could be covered.

As @christian-kreuzberger-dtx also mentioned within the KEP is that different cloud providers got different types of LoadBalancer Annotations. This is the same for ingresses.

Perhaps it would be an idea instead of creating the ingress or virtual service to print out a default ingress yaml with preconfigured services hosts and ports, which could be then adjusted by the administrator or by a CI/CD Tool when installing keptn automatically.

Something like

keptn describe ingress
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
  name: kubernetes-api-ingress
spec:
  rules:
  - host: api.kubernetes.keptn.dev.ert.com
    http:
      paths:
      - backend:
          serviceName: kubernetes
          servicePort: 443
        path: /
  tls:
  - hosts:
    - api.kubernetes.keptn.dev.ert.com

@christian-kreuzberger-dtx
Copy link
Member Author

Perhaps it would be an idea instead of creating the ingress or virtual service to print out a default ingress yaml with preconfigured services hosts and ports, which could be then adjusted by the administrator or by a CI/CD Tool when installing keptn automatically.

We will be trying to cover this with tutorials and docs with examples for now, which we believe is more than sufficient. Users should know their way around Kubernetes services and ingress if they want to use it. In fact, we have plenty of users that know what they are doing and overwrite the ingress gateway anyway, and we have plenty of users that struggle in determining the correct hostname for the ingress gateway on their Kubernetes setup.

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

7 participants