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

Watch kapp-controller-config configmap and apply updated config #179

Closed
ewrenn8 opened this issue Apr 28, 2021 · 8 comments · Fixed by #591
Closed

Watch kapp-controller-config configmap and apply updated config #179

ewrenn8 opened this issue Apr 28, 2021 · 8 comments · Fixed by #591
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.

Comments

@ewrenn8
Copy link
Contributor

ewrenn8 commented Apr 28, 2021

Describe the problem/challenge you have
Currently, kapp-controller loads and applies the kapp-controller-config configmap at startup. This means if the configmap is updated, the kapp-controller pod will need to be restarted in order to pick up that changes.

Describe the solution you'd like
kapp-controller should dynamically reconfigure itself if there are updates to the configmap.

Anything else you would like to add:
N/A


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@ewrenn8 ewrenn8 added enhancement This issue is a feature request carvel-triage This issue has not yet been reviewed for validity labels Apr 28, 2021
@ewrenn8 ewrenn8 changed the title Watch kapp-controller-config configmap and apply updated Watch kapp-controller-config configmap and apply updated config Apr 28, 2021
@jessehu
Copy link
Contributor

jessehu commented Apr 29, 2021

👍

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Jun 9, 2021
@danielhelfand danielhelfand removed the stale This issue has had no activity for a while and will be closed soon label Jun 9, 2021
@ewrenn8 ewrenn8 added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Jun 9, 2021
@aaronshurley
Copy link
Contributor

@ewrenn8 @jessehu

Just trying to gauge the impact of this feature request. How disruptive is the current process that requires a kapp-controller restart? How often would you expect the kapp-controller-config configmap to change (when kapp-controller would not be restarting)?

@aaronshurley
Copy link
Contributor

@ewrenn8 @jessehu any thoughts on the above?

@aaronshurley aaronshurley added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jul 30, 2021
@aaronshurley
Copy link
Contributor

aaronshurley commented Jul 30, 2021

For now, prioritizing this as priority/important-longterm. It would be great to hear more about the impact of this feature request, though.
Oops, wrong issue. Marking this as awaiting-more-evidence to learn more before we prioritize this.

@aaronshurley aaronshurley added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jul 30, 2021
@vibhas
Copy link

vibhas commented Aug 10, 2021

Hey @shyaamsn, wanted to see if you might be able to provide more information to Aaron's comment above.

Just trying to gauge the impact of this feature request. How disruptive is the current process that requires a kapp-controller restart? How often would you expect the kapp-controller-config configmap to change (when kapp-controller would not be restarting)?

@shreyassreenivas
Copy link

Kapp controller should detect the change in the kapp-controller-config and dynamically should reload its runtime config/certs.
Bouncing the pod on every such change in the secret is not a good user experience and difficult to write operators managing kapp controllers.

This issue is essential because kapp controller is installed first and then the packageRespository is added.
So every time a new repository that has cert-based auth is added to the cluster we should bounce the pod so that the kapp controller can pick up the latest config.

The solution for this would be to implement a reconciler which would periodically fetch the config and dynamically reloads the certs in runtime.

@neil-hickey
Copy link
Contributor

neil-hickey commented Mar 24, 2022

Development / implementation notes

  1. We current have two calls to GetConfig()
    • main process - here
    • init process - here
  2. We have an init process which runs before the main reconcilers / process are run in KC. This init process ensures that the proxy settings and relevant ca_cert appending is done before anything else.
  3. The certs are located at /etc/pki/tls/certs/ca-bundle.crt which is the system's trusted certs. We add the cert at startup which prevents any race conditions on editing that cert file during runtime of KC. This is one of the points we should validate - if we append a cert to the cert bundle file only processes called by KC will get the new updated certs - KC itself would not because this requires a restart (we believe) on POSIX based systems. There's a suspicion that maybe something like glibc does hackery [suspected] to update processes that use this - but this may not be available to us on this OS
  4. If we create a tmp file for changing certs (copy and rename approach) do we have the write permissions within the KC container to do this? (currently is +w)
  5. Each proxy entry is read using an ENV_VAR which may not play nicely with goroutines [suspected] - if we write to using os.SetEnv() - does this propagate to other goroutines? Or is this is a local copy of the env_var that when read by the another thread is not the updated value.
    • each call to os.SetEnv() and os.GetEnv()usemutexes` - see here. Therefore only one goroutine should be able to access this global system variable at any one time, and so we should be good on concurrent read/writes
  6. We chatted and agreed that using volumeMount and relying on K8s to bounce the KC pod isn't gonna play nicely with this use case - we want to introduce this dynamic nature but the tradeoff might be that proxy'ied ips are not applied on the KC container itself until a restart happens. Invoked binaries (such as ytt, kapp etc) should get this updated proxy value though. Sounds good enough to us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants