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

[buddy] fix(helm-chart): adjust to entrypoint within dockerfile #44607

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

fheinecke
Copy link
Contributor

@fheinecke fheinecke commented Jul 24, 2024

Signed-off-by: Fred Heinecke fred.heinecke@goteleport.com

changelog: Fixed event-handler Helm charts using the wrong command when starting the event-handler container

Signed-off-by: Fred Heinecke <fred.heinecke@goteleport.com>
@fheinecke fheinecke self-assigned this Jul 24, 2024
@fheinecke fheinecke marked this pull request as ready for review July 25, 2024 18:29
@fheinecke fheinecke enabled auto-merge July 25, 2024 18:29
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@fheinecke fheinecke added this pull request to the merge queue Jul 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 25, 2024
@hugoShaka hugoShaka changed the title fix(helm-chart): adjust to entrypoint within dockerfile [buddy] fix(helm-chart): adjust to entrypoint within dockerfile Jul 25, 2024
@hugoShaka
Copy link
Contributor

Can we also add in the v16 changelog that we did a breaking change by renaming the binary?

@fheinecke
Copy link
Contributor Author

a breaking change by renaming the binary

IMO this is not a breaking change as binary names in container images aren't considered an intentional part of our user-facing interface.

@fheinecke fheinecke added this pull request to the merge queue Jul 25, 2024
@hugoShaka
Copy link
Contributor

hugoShaka commented Jul 25, 2024

IMO this is not a breaking change

We even broke our own chart. This is a breaking change as this will cause user's deployment to break and make the v15 chart not able to deploy a v16 handler.

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

Many deployments rely on custom entrypoints and init scripts to obtain credentials for the plugin (mostly pre-machineID setups). I would rather revert the change or add a symlink as there was no need to break by renaming binaries in the container. This will generate support load.

Merged via the queue into master with commit 1b29bc9 Jul 25, 2024
38 of 39 checks passed
@fheinecke fheinecke deleted the fred/pr-buddy-43956 branch July 25, 2024 20:10
@fheinecke
Copy link
Contributor Author

fheinecke commented Jul 25, 2024

We even broke our own chart.

Yes - but again, this is not intended as a public, customer-facing interface. This is equivocal to changing a variable name in our Teleport Go code and then claiming that it's a breaking customer change for customers when we miss a reference to it somewhere in our codebase. A customer could be building their own Teleport builds with their own patches that depend on this variable (just as they could be building their own helm charts/deployment manifests), and a change such as this would break their workflow, but that doesn't mean that we support it in the first place.

This is a breaking change as this will cause user's deployment to break

Will this cause deployments based on our Helm chart to break (outside of the bug that this PR fixes), or just custom deployment manifests that users' may be using?

make the v15 chart not able to deploy a v16 handler.

What is the use case for this? AFAIK the auto updater doesn't support plugins, and we version our Helm charts with Teleport.


Given that this is the first time we've heard of the issue in the month+ that it's been released, and that it makes our build process easier and simpler, I am not inclined to revert this change unless there is significant customer impact via a workflow that we support.

@hugoShaka
Copy link
Contributor

hugoShaka commented Jul 25, 2024

Yes - but again, this is not intended as a public, customer-facing interface.

Regardless of the intent; customers, and our own solution engineers are potentially invoking the binary by its name in their setups. This change is disruptive for all those setups and I don't understand what is the motivation to do a change which will generate support load in this case.

IMO there is no benefit in breaking, and the fact it's not part of the official interface (an assertion I don't agree with, but this is another topic and not relevant here) is not a justification for creating toil and more work for users and our own support.

@fheinecke
Copy link
Contributor Author

our own solution engineers are invoking the binary by its name in their setups

Why? What is the use case? To be honest even with our Helm charts I don't understand why we're invoking it via command rather than just passing args.

what is the motivation to do a change which will generate support load in this case

Quite literally every change to our repos which customers can see or use could affect customers in some way. The questions we should ask are:

  • How many customers will this affect?
  • How impactful is the change on our customers?
  • Does this change affect a customer-facing interface that we provide support for?
  • How impactful is the change on us (on things like support load, or our internal processes)?

To answer these questions for this change:

  • How many customers will this affect? - All customers using the event handler container image that also explicitly invoke the binary inside the container image by name/path. Given that this is the first report across v15 and v16 that we've seen, there is likely either not many customers impacted, or other impact customers have fixed the issue themselves.
  • How impactful is the change on our affected customers? Not terribly large - customers simply need to change <plugin name> e.g. event-handler to teleport-plugin.
  • Does this change affect a customer-facing interface that we provide support for? No - the paths of binaries inside container images is not something that we intend on being stable.
  • How impactful is the change on us (on things like support load, or our internal processes)? Given that this is the first report of the issue across all plugins and two major versions over the past 6 weeks, and that this means less complexity and maintenance for our build process, the gain so far has been a net positive for us as a company.

In summary, this change to an internal interface has a small impact on a very small number of customers, and it brings us a net positive gain. As such, I believe the benefits to the company as a whole outweigh the cost.

@public-teleport-github-review-bot

@fheinecke See the table below for backport results.

Branch Result
branch/v15 Create PR
branch/v16 Create PR

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.

5 participants