Allow dynamic KC config updates

[neil-hickey]

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #179

Does this PR introduce a user-facing change?

Additional Notes for your reviewer:

See this comment for some concerns and questions before I implemented this feature

• I had a really hard time trying to test this. The reconciler is fairly dumb, so that felt unnecessary. And the config change I wanted an e2e test, but kubectl exec was failing from the test suite - always with errors like

Running 'kubectl exec -n kapp-controller deployment/kapp-controller -- bash -c ls'... OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "bash -c ls": executable file not found in $PATH: unknown

Review Checklist:

• Follows the developer guidelines
• Relevant tests are added or updated
• Relevant docs in this repo added or updated
• Relevant carvel.dev docs added or updated in a separate PR and there's
  a link to that PR
• Code is at least as readable and maintainable as it was before this
  change

Additional documentation e.g., Proposal, usage docs, etc.:

[neil-hickey]

Implementation Notes:

What happens if I update my ca bundle whilst KC is running?

• I added the PEM for https://self-signed.badssl.com/ to the caCerts field of our KC configmap. Verified I got an error with curl https://self-signed.badssl.com/ before updating the configmap and then after updating I ran curl https://self-signed.badssl.com/ again whilst kubectl exec'd on the container and it worked.

But what about KC process? Does it pick up the change?

It does not. I added in a http.Get("https://self-signed.badssl.com/ " into one of the controllers. After applying the secret and updating the config - the curl still fails with a bad cert error. Upon running kubectl rollout restart deployment/kapp-controller -n kapp-controller that http.Get() no longer fails. So any http calls within KC will not get the new certs dynamically until the next restart. This feels like a niche case though, as KC is mostly just a fancy exec() for the fetch / template / deploy tools.

NOTE: If we do ever move to using the go library version of these tools instead of invoking binaries, we will likely need to revisit this implementation

What about GoRoutine race conditions whilst os.Setenv'ing?

I validated that the os.Setenv func and os.Getenv are guarded by a r/w lock in golang. So only one goroutine can access it at once. I also printed out the environment variables in each controller to validate that they were picking up the new values dynamically.

Is os.Getenv() global / shared amongst go routines?

Yep - checkout my playground example https://go.dev/play/p/mJoS_uwD6yC

Do we need to have a lock around the /etc/pki/tls/certs/ca-bundle.crt file?

Probably not. Again golang has a lock around the write function. So we should be good here also (given we are not expecting any other process to update the system file.) - which is the assumption the previous code here held

[neil-hickey]

Hmm, I need to give some more thought on our append to /etc/pki/tls/certs/ca-bundle.crt approach. As every update of the config appends the certs to that file. So we get lots of duplication.

[benmoss]

This seems like a bug:

• Create a config (I just copy/pasted the one from https://carvel.dev/kapp-controller/docs/v0.34.0/controller-config/#controller-configuration-spec)
• See that it gets applied
• Delete that secret
• Configuration doesn't change back

I think it's because we noop if we don't find a ConfigMap or Secret, when instead we need to somehow revert to defaults

[neil-hickey]

This seems like a bug:

* Create a config (I just copy/pasted the one from https://carvel.dev/kapp-controller/docs/v0.34.0/controller-config/#controller-configuration-spec)

* See that it gets applied

* Delete that secret

* Configuration doesn't change back

I think it's because we noop if we don't find a ConfigMap or Secret, when instead we need to somehow revert to defaults

Good point @benmoss ! I updated the code a bit to accommodate that case. Basically always apply the config (even if there doesn't exist one) -- therefore just set the default (empty string) values.

[benmoss]

Nice, I was confused by that populated bool anyway, not needing it makes the code easier to understand