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

Deprecate additional sidecar config options #4279

Closed
johnharris85 opened this issue May 12, 2022 · 8 comments
Closed

Deprecate additional sidecar config options #4279

johnharris85 opened this issue May 12, 2022 · 8 comments
Labels
area/k8s good first issue Good for newcomers kind/cleanup Cleanup/refactor an existing component/code triage/rotten closed due to lack of information for too long, rejected feature...

Comments

@johnharris85
Copy link
Contributor

Description

Once the ability to customize the sidecar and init containers is merged, there will be some existing 'band aid' configuration settings that can be deprecated, this is to track the high-level list (and maybe decide if / how / when to do those deprecations).

Annotations:

  • kuma.io/sidecar-env-vars
  • kuma.io/service-account-token-volume
@johnharris85 johnharris85 added kind/feature New feature triage/pending This issue will be looked at on the next triage meeting labels May 12, 2022
@lahabana lahabana added kind/cleanup Cleanup/refactor an existing component/code and removed kind/feature New feature labels May 13, 2022
@lahabana
Copy link
Contributor

Sidecar injection will be released in 1.7 so can't do this before 1.9

@lahabana lahabana added triage/accepted The issue was reviewed and is complete enough to start working on it area/k8s and removed triage/pending This issue will be looked at on the next triage meeting labels May 16, 2022
@lahabana lahabana changed the title [WIP] Deprecate additional sidecar config options Deprecate additional sidecar config options May 16, 2022
@lahabana lahabana added the good first issue Good for newcomers label May 16, 2022
@afzal442
Copy link

hi @lahabana, wanna contribute here.

Any help?

@lahabana
Copy link
Contributor

Hey unfortunately it's a little too early to contribute this as our deprecation policy is to keep things around for 2 releases before we remove them. This is to enable users to migrate to the new way of doing things.

If you are looking for other simpler changes to do look at: good first issue Good for newcomers

@github-actions
Copy link
Contributor

This issue was inactive for 30 days it will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant please comment on it promptly or attend the next triage meeting.

@github-actions github-actions bot added the triage/stale Inactive for some time. It will be triaged again label Jun 19, 2022
@lahabana lahabana removed the triage/stale Inactive for some time. It will be triaged again label Jun 27, 2022
@rainest
Copy link

rainest commented Jun 29, 2022

I'd actually encourage keeping this as a minimal fuss method of handling Pods with automountServiceAccountToken: false.

It's possible to handle these with ContainerPatch, but managing the annotation is easier than managing and attaching the separate resource. We can also add the annotation blindly on behalf of users in the Kong chart--doing so without Kuma in place is benign--but require an additional toggle to create ContainerPatch, since that CRD won't be available without Kuma and some installers lack permission to check for CRD availability. ContainerPatch as-is isn't viable for this either; it requires the json-patch EnsurePathExistsOnAdd option for safe use.

Edit: keeping this, or at least waiting longer than initially planned to drop it, is also good for cross-version compatibility. Dropping it quickly will mean we have a narrow range of chart versions compatible with still recent versions of Kuma, with later chart versions only compatible with Kuma releases after #4279. I don't think we can include both the annotation and patch, since including the patch with older versions will actually break them (it'll try, but fail to apply).

Another alternative is having Kuma request its own tokens, similar to what Istio uses. The TokenRequest API that leverages should be available by default on 1.20+ clusters. Using that negates the need to create a separate token and reference it in the patch/annotation, since Kuma can just create its own.

@github-actions
Copy link
Contributor

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

@lahabana lahabana removed the triage/stale Inactive for some time. It will be triaged again label Oct 17, 2022
@rainest
Copy link

rainest commented Dec 2, 2022

Have there been any further thoughts on whether to keep or deprecate these fields? Releases are now up 2.0 there hasn't been any further discussion of plans or additional features to deprecate found.

@lahabana lahabana added triage/pending This issue will be looked at on the next triage meeting and removed triage/accepted The issue was reviewed and is complete enough to start working on it labels Dec 5, 2022
@jakubdyszkiewicz
Copy link
Contributor

Triage: We decided to close this issue and not deprecate the kuma.io/service-account-token-volume annotations as keeping them does not cost us much from the maintenance point of view.

@jakubdyszkiewicz jakubdyszkiewicz closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2022
@jakubdyszkiewicz jakubdyszkiewicz added triage/rotten closed due to lack of information for too long, rejected feature... and removed triage/pending This issue will be looked at on the next triage meeting labels Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/k8s good first issue Good for newcomers kind/cleanup Cleanup/refactor an existing component/code triage/rotten closed due to lack of information for too long, rejected feature...
Projects
None yet
Development

No branches or pull requests

5 participants