search

cyberark / conjur-oss-helm-chart

Helm chart for deploying Conjur OSS to Kubernetes
Apache License 2.0
27 stars 23 forks source link

Eliminate redundant Kubernetes service for Conjur #131

Open diverdane opened 3 years ago

diverdane commented 3 years ago

Is your feature request related to a problem? Please describe.

With the current Conjur OSS helm chart, there are 2 services created, which by default are an NodePort type service, and a LoadBalancer type service:

dane@dane-vbox:~/diverdane/conjur-demo-helm-chart/templates$ kubectl get svc | grep -v postgres
NAME                  TYPE           CLUSTER-IP   EXTERNAL-IP    PORT(S)         AGE
conjur-oss            NodePort       10.0.0.213   <none>         443:32413/TCP   137d
conjur-oss-ingress    LoadBalancer   10.0.4.165   34.74.122.54   443:30279/TCP   137d
dane@dane-vbox:~/diverdane/conjur-demo-helm-chart/templates$

The manifest templates for these services are found in conjur-oss/templates/service.yaml and conjur-oss/templates/load-balancer.yaml, respectively.

These two services can be combined into one service (with one YAML manifest for that combined service), where the type of service can be one of the following (mutually exclusive) types:

Note that providing a ClusterIP and NodePort options is helpful in situations where the platform does not natively support Load balancers (e.g. Minikube and Kubernetes-in-Docker), or where load balancers are not necessary (all access to Conjur is from clients that are inside the Kubernetes cluster).

Note that for LoadBalancer type services, Kubernetes provides gratuitous ClusterIP and NodePort service access by default, so there's no need for separate internal services to be defined.

The support of 2 services will need to be deprecated for a time period before this support is removed, since there may be some folks in the community expecting these 2 services to be available.

Deprecation should be possible by adding additive fields in values.yaml, e.g.:

service:
  singleService: false
  # annotations: {}
  # port: 443
  # type: LoadBalancer

Where the singleService setting and all internal and external settings will disappear for the next major version when dual-service is no longer supported. The type field can be set to ClusterIP, NodePort, or LoadBalancer (when singleService is set to true. When singleService is set to true, then the internal and external values settings are ignored.

Deprecation and Ignored Values Warnings Required in NOTES.txt

The implementation for this Issue must include updates to the conjur-oss/templates/NOTES.txt to include the following:

Here is a useful reference: https://github.com/helm/helm/issues/8766

Describe the solution you would like

See description above.

Describe alternatives you have considered

Alternative is keeping both services for Conjur.

Additional context

rafis3 commented 3 years ago

Thanks for this clear and elaborate design @diverdane. Question regarding this:

When singleService is set to true, then the internal and external values settings are ignored.

what do you think about failing in that case? I am more concerned with ignoring because the user might not notice they did something wrong. If not a failure, then at least a warning message that these field will be ignored.

diverdane commented 3 years ago

@rafis3 thank you for your input on this! You raise a very good point... there should be some warning that some fields are being ignored.

Beyond the issue with ignored values, your comment brings to mind the more general problem that we're not displaying any deprecation warnings. We definitely need to add deprecation notes.

I think the way that this can be implemented is to update the NOTES that are displayed at the end of a Helm install/update. (As far as I know, there's no way to emit warnings while the chart is being installed/updated). I'll update this Issue with the requirement that we include a warning that some values are ignored if/when the single Conjur service mode is enabled, and open a more general Issue to add deprecation warnings.

diverdane commented 3 years ago

@rafis3 , I updated the description with a Deprecation and Ignored Values Warnings Required in NOTES.txt section. Thanks for bringing this up!