Skip to content

helm: Adds 'aws', 'gcp', 'standalone' and ‘custom’ modes to teleport-cluster chart#6344

Merged
webvictim merged 29 commits intomasterfrom
gus/helm/improvements-2
May 17, 2021
Merged

helm: Adds 'aws', 'gcp', 'standalone' and ‘custom’ modes to teleport-cluster chart#6344
webvictim merged 29 commits intomasterfrom
gus/helm/improvements-2

Conversation

@webvictim
Copy link
Copy Markdown
Contributor

@webvictim webvictim commented Apr 7, 2021

Updates #5582

Adds new modes to the teleport-cluster Helm chart to allow deployments as discussed in #5582.

The Helm guides and reference accompanying these changes are hosted here if it helps with review: https://teleport-docs-next-git-gus-helmdocs-gravitational.vercel.app/docs/ver/7.0/kubernetes-access/helm/guides/

Fixes #6681

@webvictim webvictim added the helm label Apr 7, 2021
@webvictim webvictim self-assigned this Apr 7, 2021
@webvictim webvictim force-pushed the gus/helm/improvements-2 branch 3 times, most recently from 550836f to a827609 Compare April 8, 2021 17:33
@webvictim webvictim force-pushed the gus/helm/improvements-2 branch 3 times, most recently from edb8c5b to 1e4baaa Compare April 26, 2021 17:17
@webvictim webvictim requested review from awly and klizhentas April 26, 2021 17:18
@webvictim webvictim marked this pull request as ready for review April 26, 2021 17:19
@russjones russjones assigned r0mant, klizhentas and awly and unassigned r0mant, awly and webvictim Apr 28, 2021
@webvictim webvictim force-pushed the gus/helm/improvements-2 branch from 2c7668f to d3eb086 Compare April 29, 2021 17:08
Comment thread version.mk
Comment on lines +26 to +28
# If the version contains '-dev' (as it does on the master branch, or for development builds) then we get the latest
# published major version number by parsing a sorted list of git tags instead, to make deploying the chart from master
# work as expected. Version numbers are quoted as a string because Helm otherwise treats dotted decimals as floats.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this part.
If a branch is 7.0.0-dev, shouldn't the helm chart use 7.0.0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

7.0.0-dev is what master is currently set to (when our most recent release is 6.1.3)

When we release 7.0.0, master will get updated to 8.0.0-dev, so this always needs to calculate n-1.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But don't we publish helm charts from release branches, where the version is correct?
Publishing master chart as the previous version could cause issues, e.g. overwriting the released version of the chart

Copy link
Copy Markdown
Contributor Author

@webvictim webvictim May 6, 2021

Choose a reason for hiding this comment

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

No, the Helm charts are published nightly from master by a cron job in Drone.

You make a good point though, I think that if we want to tie Helm chart version to Teleport version (as per #5582) then we need to change Drone to make Helm chart releases part of the current tag/promote process instead, so that we publish a correctly versioned copy on the correct tag/branch whenever a release is made. We're trying to push people to install from https://charts.releases.teleport.dev rather than cloning master (or a tag/branch) so this makes some sense.

I've been resistant to this because it means that pushing a simple Helm chart bug fix is going to require releasing a whole new version of Teleport, which is a relatively heavyweight operation. This is really what the appVersion field is supposed to be for - i.e. you version your Helm chart completely separately to your application and then you can interchangeably use different versions of each. I think @klizhentas wanted to version them the same to reduce confusion, but it will make our release process a bit worse. I don't see a way around this if we want to inextricably tie chart version to Teleport version, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use the "Promote" function of Drone to trigger a Helm chart deployment, without triggering a full release?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could probably do that. The only question is what we'd be promoting - i.e. normally with Teleport releases, we promote a tag build which has DRONE_TAG set. If we want to allow promotion of other arbitrary commits/pushes then we'd have to work based on commit hash. We'd likely have to add some logic which refuses to release if the commit hash being promoted is on the master branch rather than a release branch, for example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Raised #6887 to track this

Comment thread version.mk
Comment thread Makefile
Comment thread Makefile
Comment thread examples/chart/teleport-auto-trustedcluster/Chart.yaml
Comment thread examples/chart/teleport-cluster/values.yaml Outdated
Comment thread examples/chart/teleport-kube-agent/.lint/initcontainers.yaml
Comment thread examples/chart/teleport-kube-agent/values.schema.json
Comment thread examples/chart/teleport-kube-agent/values.schema.json Outdated
Comment thread examples/chart/teleport-cluster/values.schema.json
@webvictim
Copy link
Copy Markdown
Contributor Author

@klizhentas Ping

Copy link
Copy Markdown
Contributor

@awly awly left a comment

Choose a reason for hiding this comment

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

LGTM for now, keeping the existing release model from master.
But please consider moving helm releases to the same model as regular teleport releases, with some escape hatch to publish new changes without a full Teleport release.

Comment thread examples/chart/teleport-cluster/templates/deployment.yaml Outdated
@webvictim webvictim changed the title helm: Adds 'aws', 'gcp' and 'standalone' modes to teleport-cluster chart helm: Adds 'aws', 'gcp', 'standalone' and ‘custom’ modes to teleport-cluster chart May 13, 2021
@webvictim webvictim mentioned this pull request May 13, 2021
@webvictim
Copy link
Copy Markdown
Contributor Author

Needs a backport to branch/v6 to be published along with 6.2 (not strictly necessary from a Helm repo perspective, but some customers like to check out the release tag from Git and deploy the corresponding chart version)

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.

Allow for arbitrary pod annotations in teleport-cluster chart

4 participants