Skip to content

Commit

Permalink
Stop sending base info via X-Juju-Metadata.
Browse files Browse the repository at this point in the history 
This is a duplication of data which can be collected from the context
and action themselves.  Nor is it being collected upstream.
  • Loading branch information
@hmlanigan
hmlanigan committed Oct 8, 2021
1 parent 92db34e commit dbd1167
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 91 deletions.
12 changes: 6 additions & 6 deletions apiserver/facades/client/charms/charmhubrepo_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ func (m RefreshConfgMatcher) Matches(x interface{}) bool {
return false
}

cb, _, err := m.Config.Build()
cb, err := m.Config.Build()
m.c.Assert(err, jc.ErrorIsNil)

rcb, _, err := rc.Build()
rcb, err := rc.Build()
m.c.Assert(err, jc.ErrorIsNil)
m.c.Assert(len(cb.Actions), gc.Equals, len(rcb.Actions))

Expand Down Expand Up @@ -435,7 +435,7 @@ func (refreshConfigSuite) TestRefreshByChannel(c *gc.C) {
ch := channel.String()
instanceKey := charmhub.ExtractConfigInstanceKey(cfg)

build, _, err := cfg.Build()
build, err := cfg.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(build, gc.DeepEquals, transport.RefreshRequest{
Actions: []transport.RefreshRequestAction{{
Expand Down Expand Up @@ -468,7 +468,7 @@ func (refreshConfigSuite) TestRefreshByChannelVersion(c *gc.C) {
ch := channel.String()
instanceKey := charmhub.ExtractConfigInstanceKey(cfg)

build, _, err := cfg.Build()
build, err := cfg.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(build, gc.DeepEquals, transport.RefreshRequest{
Actions: []transport.RefreshRequestAction{{
Expand Down Expand Up @@ -500,7 +500,7 @@ func (refreshConfigSuite) TestRefreshByRevision(c *gc.C) {

instanceKey := charmhub.ExtractConfigInstanceKey(cfg)

build, _, err := cfg.Build()
build, err := cfg.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(build, gc.DeepEquals, transport.RefreshRequest{
Actions: []transport.RefreshRequestAction{{
Expand Down Expand Up @@ -535,7 +535,7 @@ func (refreshConfigSuite) TestRefreshByID(c *gc.C) {

instanceKey := charmhub.ExtractConfigInstanceKey(cfg)

build, _, err := cfg.Build()
build, err := cfg.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(build, gc.DeepEquals, transport.RefreshRequest{
Actions: []transport.RefreshRequestAction{{
Expand Down
2 changes: 1 addition & 1 deletion apiserver/facades/controller/charmrevisionupdater/interface_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (m charmhubConfigMatcher) Matches(x interface{}) bool {
if !ok {
return false
}
request, _, err := config.Build()
request, err := config.Build()
if err != nil {
return false
}
Expand Down
11 changes: 4 additions & 7 deletions charmhub/http_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,21 +228,18 @@ func (s *RESTSuite) TestGetWithUnmarshalFailure(c *gc.C) {

func (s *RESTSuite) TestComposeHeaders(c *gc.C) {
clientHeaders := http.Header{
"User-Agent": []string{"Juju/3.14.159"},
"X-Juju-Metadata": []string{"cloud=cloud-9", "cloud_region=juju-land"},
"User-Agent": []string{"Juju/3.14.159"},
}
requestHeaders := http.Header{
"X-Juju-Metadata": []string{"arch=amd64", "os=ubuntu", "series=focal"},
"Something-Else": []string{"foo"},
"Something-Else": []string{"foo"},
}

client := NewHTTPRESTClient(nil, clientHeaders)
got := client.composeHeaders(requestHeaders)

c.Assert(got, gc.DeepEquals, http.Header{
"User-Agent": []string{"Juju/3.14.159"},
"X-Juju-Metadata": []string{"arch=amd64", "os=ubuntu", "series=focal", "cloud=cloud-9", "cloud_region=juju-land"},
"Something-Else": []string{"foo"},
"User-Agent": []string{"Juju/3.14.159"},
"Something-Else": []string{"foo"},
})
}

Expand Down
81 changes: 20 additions & 61 deletions charmhub/refresh.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"net/http"
"strings"

"github.com/juju/collections/set"
"github.com/juju/errors"
"github.com/juju/loggo"
"github.com/juju/names/v4"
Expand Down Expand Up @@ -88,7 +87,7 @@ func NewRefreshClient(path path.Path, client RESTClient, logger Logger) *Refresh
// RefreshConfig defines a type for building refresh requests.
type RefreshConfig interface {
// Build a refresh request for sending to the API.
Build() (transport.RefreshRequest, Headers, error)
Build() (transport.RefreshRequest, error)

// Ensure that the request back contains the information we requested.
Ensure([]transport.RefreshResponse) error
Expand All @@ -100,18 +99,18 @@ type RefreshConfig interface {
// Refresh is used to refresh installed charms to a more suitable revision.
func (c *RefreshClient) Refresh(ctx context.Context, config RefreshConfig) ([]transport.RefreshResponse, error) {
c.logger.Tracef("Refresh(%s)", pretty.Sprint(config))
req, headers, err := config.Build()
req, err := config.Build()
if err != nil {
return nil, errors.Trace(err)
}
return c.refresh(ctx, config.Ensure, req, headers)
return c.refresh(ctx, config.Ensure, req)
}

// RefreshWithRequestMetrics is to get refreshed charm data and provide metrics
// at the same time. Used as part of the charm revision updater facade.
func (c *RefreshClient) RefreshWithRequestMetrics(ctx context.Context, config RefreshConfig, metrics map[charmmetrics.MetricKey]map[charmmetrics.MetricKey]string) ([]transport.RefreshResponse, error) {
c.logger.Tracef("RefreshWithRequestMetrics(%s, %+v)", pretty.Sprint(config), metrics)
req, headers, err := config.Build()
req, err := config.Build()
if err != nil {
return nil, errors.Trace(err)
}
Expand All @@ -120,7 +119,7 @@ func (c *RefreshClient) RefreshWithRequestMetrics(ctx context.Context, config Re
return nil, errors.Trace(err)
}
req.Metrics = m
return c.refresh(ctx, config.Ensure, req, headers)
return c.refresh(ctx, config.Ensure, req)
}

// RefreshWithMetricsOnly is to provide metrics without context or actions. Used
Expand All @@ -136,12 +135,11 @@ func (c *RefreshClient) RefreshWithMetricsOnly(ctx context.Context, metrics map[
Actions: []transport.RefreshRequestAction{},
Metrics: m,
}
headers := make(map[string][]string)

// No need to ensure data which is not expected.
ensure := func(responses []transport.RefreshResponse) error { return nil }

_, err = c.refresh(ctx, ensure, req, headers)
_, err = c.refresh(ctx, ensure, req)
return err
}

Expand All @@ -161,13 +159,8 @@ func contextMetrics(metrics map[charmmetrics.MetricKey]map[charmmetrics.MetricKe
return m, nil
}

func (c *RefreshClient) refresh(ctx context.Context, ensure func(responses []transport.RefreshResponse) error, req transport.RefreshRequest, headers Headers) ([]transport.RefreshResponse, error) {
func (c *RefreshClient) refresh(ctx context.Context, ensure func(responses []transport.RefreshResponse) error, req transport.RefreshRequest) ([]transport.RefreshResponse, error) {
httpHeaders := make(http.Header)
for k, values := range headers {
for _, value := range values {
httpHeaders.Add(MetadataHeader, fmt.Sprintf("%s=%s", k, value))
}
}

var resp transport.RefreshResponses
restResp, err := c.client.Post(ctx, c.path, httpHeaders, req, &resp)
Expand Down Expand Up @@ -241,10 +234,10 @@ func CreateInstanceKey(app names.ApplicationTag, model names.ModelTag) string {
}

// Build a refresh request that can be past to the API.
func (c refreshOne) Build() (transport.RefreshRequest, Headers, error) {
func (c refreshOne) Build() (transport.RefreshRequest, error) {
base, err := constructRefreshBase(c.Base)
if err != nil {
return transport.RefreshRequest{}, nil, errors.Trace(err)
return transport.RefreshRequest{}, errors.Trace(err)
}

return transport.RefreshRequest{
Expand All @@ -264,7 +257,7 @@ func (c refreshOne) Build() (transport.RefreshRequest, Headers, error) {
InstanceKey: c.instanceKey,
ID: &c.ID,
}},
}, constructMetadataHeaders(c.Base), nil
}, nil
}

// Ensure that the request back contains the information we requested.
Expand Down Expand Up @@ -407,10 +400,10 @@ func DownloadOneFromChannel(id string, channel string, base RefreshBase) (Refres
}

// Build a refresh request that can be past to the API.
func (c executeOne) Build() (transport.RefreshRequest, Headers, error) {
func (c executeOne) Build() (transport.RefreshRequest, error) {
base, err := constructRefreshBase(c.Base)
if err != nil {
return transport.RefreshRequest{}, nil, errors.Trace(err)
return transport.RefreshRequest{}, errors.Trace(err)
}

var id *string
Expand All @@ -434,7 +427,7 @@ func (c executeOne) Build() (transport.RefreshRequest, Headers, error) {
Channel: c.Channel,
Base: &base,
}},
}, constructMetadataHeaders(c.Base), nil
}, nil
}

// Ensure that the request back contains the information we requested.
Expand Down Expand Up @@ -478,27 +471,27 @@ func RefreshMany(configs ...RefreshConfig) RefreshConfig {
}

// Build a refresh request that can be past to the API.
func (c refreshMany) Build() (transport.RefreshRequest, Headers, error) {
func (c refreshMany) Build() (transport.RefreshRequest, error) {
if len(c.Configs) == 0 {
return transport.RefreshRequest{}, nil, errors.NotFoundf("configs")
return transport.RefreshRequest{}, errors.NotFoundf("configs")
}
var composedHeaders Headers

// Not all configs built here have a context, start out with an empty
// slice, so we do not call Refresh with a nil context.
// See executeOne.Build().
result := transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
}
for _, config := range c.Configs {
req, headers, err := config.Build()
req, err := config.Build()
if err != nil {
return transport.RefreshRequest{}, nil, errors.Trace(err)
return transport.RefreshRequest{}, errors.Trace(err)
}
result.Context = append(result.Context, req.Context...)
result.Actions = append(result.Actions, req.Actions...)
composedHeaders = composeMetadataHeaders(composedHeaders, headers)

}
return result, composedHeaders, nil
return result, nil
}

// Ensure that the request back contains the information we requested.
Expand Down Expand Up @@ -562,40 +555,6 @@ func constructRefreshBase(base RefreshBase) (transport.Base, error) {
}, nil
}

// constructHeaders adds X-Juju-Metadata headers for the charms' unique channel
// and architecture values, for example:
//
// X-Juju-Metadata: channel=bionic
// X-Juju-Metadata: arch=amd64
// X-Juju-Metadata: channel=focal
func constructMetadataHeaders(base RefreshBase) map[string][]string {
headers := make(map[string][]string)
if base.Architecture != "" {
headers["arch"] = []string{base.Architecture}
}
if base.Name != "" {
headers["name"] = []string{base.Name}
}
if base.Channel != "" {
headers["channel"] = []string{base.Channel}
}
return headers
}

func composeMetadataHeaders(a, b Headers) Headers {
result := make(map[string][]string)
for k, v := range a {
result[k] = append(result[k], v...)
}
for k, v := range b {
result[k] = append(result[k], v...)
}
for k, v := range result {
result[k] = set.NewStrings(v...).SortedValues()
}
return result
}

// validateBase ensures that we do not pass "all" as part of base.
// This function is to help find programming related failures.
func validateBase(rp RefreshBase) error {
Expand Down
26 changes: 10 additions & 16 deletions charmhub/refresh_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,6 @@ func (s *RefreshSuite) TestRefreshMetadata(c *gc.C) {
response, err := client.Refresh(context.Background(), config)
c.Assert(err, jc.ErrorIsNil)

c.Assert(httpTransport.requestHeaders[MetadataHeader], jc.SameContents, []string{
"arch=amd64",
"name=ubuntu",
"channel=20.04",
"channel=14.04",
})
c.Assert(httpTransport.requestHeaders["User-Agent"], jc.SameContents, []string{"Test Agent 1.0"})

c.Assert(response, gc.DeepEquals, []transport.RefreshResponse{
Expand Down Expand Up @@ -388,7 +382,7 @@ func (s *RefreshConfigSuite) TestRefreshOneBuild(c *gc.C) {
})
c.Assert(err, jc.ErrorIsNil)

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{{
Expand Down Expand Up @@ -438,7 +432,7 @@ func (s *RefreshConfigSuite) TestRefreshOneWithMetricsBuild(c *gc.C) {
})
c.Assert(err, jc.ErrorIsNil)

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{{
Expand Down Expand Up @@ -491,7 +485,7 @@ func (s *RefreshConfigSuite) TestInstallOneBuildRevision(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
Expand Down Expand Up @@ -522,7 +516,7 @@ func (s *RefreshConfigSuite) TestInstallOneBuildChannel(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
Expand Down Expand Up @@ -551,7 +545,7 @@ func (s *RefreshConfigSuite) TestInstallOneWithPartialPlatform(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
Expand All @@ -578,7 +572,7 @@ func (s *RefreshConfigSuite) TestInstallOneWithMissingArch(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

_, _, err = config.Build()
_, err = config.Build()
c.Assert(errors.IsNotValid(err), jc.IsTrue)
}

Expand Down Expand Up @@ -642,7 +636,7 @@ func (s *RefreshConfigSuite) TestDownloadOneFromChannelBuild(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
Expand Down Expand Up @@ -672,7 +666,7 @@ func (s *RefreshConfigSuite) TestDownloadOneFromChannelBuildK8s(c *gc.C) {

config = DefineInstanceKey(c, config, "foo-bar")

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{},
Expand Down Expand Up @@ -726,7 +720,7 @@ func (s *RefreshConfigSuite) TestRefreshManyBuildContextNotNil(c *gc.C) {
config2 = DefineInstanceKey(c, config2, "foo-baz")
config := RefreshMany(config1, config2)

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req.Context, gc.NotNil)
}
Expand Down Expand Up @@ -762,7 +756,7 @@ func (s *RefreshConfigSuite) TestRefreshManyBuild(c *gc.C) {

config := RefreshMany(config1, config2, config3)

req, _, err := config.Build()
req, err := config.Build()
c.Assert(err, jc.ErrorIsNil)
c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
Context: []transport.RefreshRequestContext{{
Expand Down

0 comments on commit dbd1167

Please sign in to comment.