Navigation Menu

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

Added support for service.spec.loadBalancerIP #8774

Closed
wants to merge 8 commits into from
Closed

Added support for service.spec.loadBalancerIP #8774

wants to merge 8 commits into from

Conversation

macevil
Copy link
Contributor

@macevil macevil commented Oct 28, 2021

Hello all, the possibility to configure the Service loadBalancerIP in the Helm Chart was missing. I have added it and tested it.

Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

@macevil Thanks for the PR. Looks good overall, but there are a few requests before we can merge:

  1. Please add service.spec at the appropriate location in the teleport-cluster chart reference: https://github.com/gravitational/teleport/blob/master/docs/pages/kubernetes-access/helm/reference.mdx

  2. Please add service.spec to https://github.com/gravitational/teleport/blob/master/examples/chart/teleport-cluster/values.schema.json

  3. Please add an example which sets service.spec so it can be validated by the linter - see other examples in https://github.com/gravitational/teleport/tree/master/examples/chart/teleport-cluster/.lint

Thank you!

examples/chart/teleport-cluster/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

@macevil Thanks! Looking better - just one more issue with the schema JSON.

examples/chart/teleport-cluster/values.schema.json Outdated Show resolved Hide resolved
examples/chart/teleport-cluster/templates/service.yaml Outdated Show resolved Hide resolved
@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

@russjones This one looks good to go - please approve/merge/backport as appropriate.

@russjones
Copy link
Contributor

Will buddy merge in #9484.

@russjones russjones closed this Dec 18, 2021
@russjones russjones mentioned this pull request Dec 18, 2021
type: LoadBalancer
type: {{- default "LoadBalancer" .Values.service.type }}
{{- with .Values.service.spec }}
{{- toYaml . | nindent 4 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The nindent should be 2 instead of 4 here as it produces invalid yaml otherwise that looks like below

# Source: teleport-cluster/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: teleport
  namespace: cert-manager
  labels:
    app: teleport
spec:
  type: LoadBalancer
    loadBalancerIP: 1.1.1.1

and produces the following error

Error: YAML parse error on teleport-cluster/templates/service.yaml: error converting YAML to JSON: yaml: line 10: mapping values are not allowed in this context

for the following values

clusterName: teleport.example.com
service:
  spec:
    loadBalancerIP: "1.1.1.1"

Copy link
Contributor

Choose a reason for hiding this comment

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

PR to fix #9772

Copy link
Contributor

Choose a reason for hiding this comment

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

Handled in #9645, thanks

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

Successfully merging this pull request may close these issues.

None yet

4 participants