Add auth header to logcache API requests for cf-on-k8s #2267
Conversation
Authored-by: Matt Royal <[email protected]>
- Extract "Authorization" header logic for CFonK8s into api/shared/WrapForCFOnK8sAuth and refactor existing code to use this - Move LogCache client into api/logcache to avoid a cyclic import - Bubble out error from logcache.NewClient - New selfcontained tests for logcache request changes Co-authored-by: Matt Royal <[email protected]> Co-authored-by: Dave Walter <[email protected]> Co-authored-by: Ashwin Krishna <[email protected]>
| logCacheClient := command.NewLogCacheClient(logCacheURL, configV3, nil) | ||
| logCacheClient, err := logcache.NewClient(logCacheURL, configV3, nil, v7action2.NewDefaultKubernetesConfigGetter()) | ||
| if err != nil { | ||
| panic("handle this error!") |
There was a problem hiding this comment.
Now that creating the logcache client can yield an error, we need to handle that. The existing code also panics on error a few lines up, so we copied that pattern. We could also change this function and the caller function to return errors, and then bubble that out into the main func if you prefer.
|
This PR is built on top of #2266, so that PR should be accepted first. |
| if transportConfig.WrapTransport == nil { | ||
| // i.e. not auth-provider or exec plugin | ||
| return transport.HTTPWrappersForConfig(transportConfig, roundTripper) | ||
| } | ||
|
|
||
| // using auth provider to generate token | ||
| return transportConfig.WrapTransport(roundTripper), nil |
There was a problem hiding this comment.
We have recently found our that this check is redundant.
There was a problem hiding this comment.
should that check be removed before we merge the PR?
There was a problem hiding this comment.
Yes, we might get away with just return transportConfig.HTTPWrappersForConfig(...) but I'd try it first, just in case.
Add auth header to logcache API requests for cf-on-k8s
It's hard for me to imagine anything in v8 that does not belong to the |
Add auth header to logcache API requests for cf-on-k8s
This reverts commit 891592c.
This reverts commit 891592c.
Does this PR modify CLI v6, CLI v7, or CLI v8?
This PR modifies v8. This same behavior will need to exist in v9. How should we handle that?
Description of the Change
Set the correct
Authorizationheader when requesting logs on a cf-on-k8s deployment. This uses the same logic for generating the header as exists in the current CLI, but refactored for reuse. The behavior is unchanged for cf-on-vmsWhy Is This PR Valuable?
This PR enables future work for cf-on-k8s to support logging during push, etc.
Why Should This Be In Core?
Logging is part of core behavior and we've already made changes to support auth for cf-on-k8s in core.
Applicable Issues
cloudfoundry/korifi#370
How Urgent Is The Change?
This change is necessary to unblock logging work for cf-on-k8s.
Other Relevant Parties
@cloudfoundry/cf-k8s