Skip to content
This repository has been archived by the owner on Oct 11, 2019. It is now read-only.

Refactor to remove legacyconfig object #163

Merged
merged 6 commits into from
Feb 15, 2019
Merged

Refactor to remove legacyconfig object #163

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f708a33
The common package for ec2 is no longer common, remove it
amckague Feb 14, 2019
7d1bbdf
Refactor to remove legacyconfig object
amckague Feb 14, 2019
3e64e43
Take location specific configuration as constructor args
amckague Feb 14, 2019
c613565
Remove remaining stub
amckague Feb 14, 2019
e51288a
Merge branch 'master' into remove_legacy
amckague Feb 14, 2019
3329943
Merge branch 'master' into remove_legacy
amckague Feb 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/orchestration/app/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ go_library(
deps = [
"//:go_default_library",
"//cmd:go_default_library",
"//pkg/apis/orchestration/meta:go_default_library",
"//pkg/apis/orchestration/v1:go_default_library",
"//pkg/k8s:go_default_library",
"//pkg/options:go_default_library",
Expand All @@ -20,7 +21,6 @@ go_library(
"//pkg/orchestration/informer:go_default_library",
"//pkg/orchestration/updater:go_default_library",
"//pkg/orchestration/wiring:go_default_library",
"//pkg/orchestration/wiring/legacy:go_default_library",
"//pkg/orchestration/wiring/registry:go_default_library",
"//pkg/orchestration/wiring/wiringplugin:go_default_library",
"//pkg/util/crash:go_default_library",
Expand Down
16 changes: 4 additions & 12 deletions cmd/orchestration/app/controller_constructor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
orchInf "github.com/atlassian/voyager/pkg/orchestration/informer"
orchUpdater "github.com/atlassian/voyager/pkg/orchestration/updater"
"github.com/atlassian/voyager/pkg/orchestration/wiring"
"github.com/atlassian/voyager/pkg/orchestration/wiring/legacy"
"github.com/atlassian/voyager/pkg/orchestration/wiring/wiringplugin"
prom_util "github.com/atlassian/voyager/pkg/util/prometheus"
"github.com/pkg/errors"
Expand All @@ -30,9 +29,9 @@ import (
)

type ControllerConstructor struct {
FlagConfigFile string
GetLegacyConfigFunc func(voyager.Location) *legacy.Config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real problem, I think, is that autowiring functions at the moment are not configurable (they are all hardcoded), and the only way to pass configuration is via context and orchestration controller constructor

Copy link
Contributor

@ychen-atlassian ychen-atlassian Feb 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving the plugins out of wiring/registry? We can pass them as parameters into the constructor for orchestration controller.

This matches up with what @amckague has done for the example tag generator, and it's what we sort of do for smith too. We're going to want to split the wiring out of this repo anyway.

Plugins map[voyager.ResourceType]wiringplugin.WiringPlugin
FlagConfigFile string
Plugins map[voyager.ResourceType]wiringplugin.WiringPlugin
Tags wiring.TagGenerator
}

func (cc *ControllerConstructor) AddFlags(flagset ctrl.FlagSet) {
Expand Down Expand Up @@ -88,14 +87,7 @@ func (cc *ControllerConstructor) New(config *ctrl.Config, cctx *ctrl.Context) (*
Plugins: cc.Plugins,
ClusterLocation: opts.Location.ClusterLocation(),
ClusterConfig: toClusterConfig(opts.Cluster),
TagNames: wiring.TagNames{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Seems unrelated to LegacyConfig. Could you split into multiple PRs please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment about introducing the tag generator. If the tags come from a function there's no need to make their keys come in via config.

ServiceNameTag: opts.TagNames.ServiceName,
BusinessUnitTag: opts.TagNames.BusinessUnit,
ResourceOwnerTag: opts.TagNames.ResourceOwner,
PlatformTag: opts.TagNames.Platform,
EnvironmentTypeTag: opts.TagNames.EnvironmentType,
},
GetLegacyConfigFunc: cc.GetLegacyConfigFunc,
Tags: cc.Tags,
}

// Spec check
Expand Down
20 changes: 13 additions & 7 deletions cmd/orchestration/app/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
ctrlApp "github.com/atlassian/ctrl/app"
"github.com/atlassian/voyager"
"github.com/atlassian/voyager/cmd"
"github.com/atlassian/voyager/pkg/orchestration/wiring/legacy"
orch_meta "github.com/atlassian/voyager/pkg/apis/orchestration/meta"
"github.com/atlassian/voyager/pkg/orchestration/wiring/registry"
"github.com/atlassian/voyager/pkg/orchestration/wiring/wiringplugin"
"github.com/atlassian/voyager/pkg/util/crash"
Expand All @@ -23,18 +23,18 @@ const (
)

func Main() {
CustomMain(emptyLegacyConfigFunc, registry.KnownWiringPlugins)
CustomMain(registry.KnownWiringPlugins)
}

func CustomMain(getLegacyConfigFunc func(voyager.Location) *legacy.Config, plugins map[voyager.ResourceType]wiringplugin.WiringPlugin) {
func CustomMain(plugins map[voyager.ResourceType]wiringplugin.WiringPlugin) {
rand.Seed(time.Now().UnixNano())
klog.InitFlags(nil)
cmd.RunInterruptably(func(ctx context.Context) error {
crash.InstallAPIMachineryLoggers()
controllers := []ctrl.Constructor{
&ControllerConstructor{
GetLegacyConfigFunc: getLegacyConfigFunc,
Plugins: plugins,
Plugins: plugins,
Tags: ExampleTags,
},
}

Expand All @@ -48,6 +48,12 @@ func CustomMain(getLegacyConfigFunc func(voyager.Location) *legacy.Config, plugi
})
}

func emptyLegacyConfigFunc(location voyager.Location) *legacy.Config {
return &legacy.Config{}
func ExampleTags(
_ voyager.ClusterLocation,
_ wiringplugin.ClusterConfig,
_ voyager.Location,
_ voyager.ServiceName,
_ orch_meta.ServiceProperties,
) map[voyager.Tag]string {
return make(map[voyager.Tag]string)
}
13 changes: 0 additions & 13 deletions cmd/orchestration/app/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,15 @@ package app
import (
"io/ioutil"

"github.com/atlassian/voyager"
"github.com/atlassian/voyager/pkg/options"
"github.com/pkg/errors"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"sigs.k8s.io/yaml"
)

// TagNames tell us what tag to store each of these properties under.
// e.g. ServiceName = "application_name", and the service name is put into the
// application_name tag.
type TagNames struct {
ServiceName voyager.Tag `json:"serviceName"`
BusinessUnit voyager.Tag `json:"businessUnit"`
ResourceOwner voyager.Tag `json:"resourceOwner"`
Platform voyager.Tag `json:"platform"`
EnvironmentType voyager.Tag `json:"environmentType"`
}

type Options struct {
Location options.Location `json:"location"`
Cluster options.Cluster `json:"cluster"`
TagNames TagNames `json:"tagNames"`
}

func (o *Options) DefaultAndValidate() []error {
Expand Down
2 changes: 1 addition & 1 deletion pkg/creator/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ go_library(
"//pkg/apis/creator/v1:go_default_library",
"//pkg/creator/luigi:go_default_library",
"//pkg/creator/ssam:go_default_library",
"//pkg/orchestration/wiring/ec2compute/common:go_default_library",
"//pkg/orchestration/wiring/ec2compute/v2:go_default_library",
"//pkg/pagerduty:go_default_library",
"//pkg/servicecentral:go_default_library",
"//pkg/util:go_default_library",
Expand Down
4 changes: 2 additions & 2 deletions pkg/creator/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
creator_v1 "github.com/atlassian/voyager/pkg/apis/creator/v1"
"github.com/atlassian/voyager/pkg/creator/luigi"
"github.com/atlassian/voyager/pkg/creator/ssam"
ec2compute_common "github.com/atlassian/voyager/pkg/orchestration/wiring/ec2compute/common"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also unrelated to LegacyConfig? separate PR please.

ec2compute_v2 "github.com/atlassian/voyager/pkg/orchestration/wiring/ec2compute/v2"
"github.com/atlassian/voyager/pkg/pagerduty"
"github.com/atlassian/voyager/pkg/servicecentral"
"github.com/atlassian/voyager/pkg/util"
Expand All @@ -33,7 +33,7 @@ const (
ServiceNameMinimumLength = 1
ServiceNameMaximumLength = 24
ServiceNameExpr = schema.ResourceNameSchemaPattern
EC2ComputeNameMaximumLength = ec2compute_common.MaximumServiceNameLength
EC2ComputeNameMaximumLength = ec2compute_v2.MaximumServiceNameLength
)

var (
Expand Down
2 changes: 1 addition & 1 deletion pkg/orchestration/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ go_test(
race = "on",
deps = [
"//:go_default_library",
"//pkg/apis/orchestration/meta:go_default_library",
"//pkg/apis/orchestration/v1:go_default_library",
"//pkg/orchestration/client/fake:go_default_library",
"//pkg/orchestration/wiring:go_default_library",
"//pkg/orchestration/wiring/legacy:go_default_library",
"//pkg/orchestration/wiring/registry:go_default_library",
"//pkg/orchestration/wiring/wiringplugin:go_default_library",
"//pkg/util/testutil:go_default_library",
Expand Down
32 changes: 18 additions & 14 deletions pkg/orchestration/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (

smith_v1 "github.com/atlassian/smith/pkg/apis/smith/v1"
"github.com/atlassian/voyager"
orch_meta "github.com/atlassian/voyager/pkg/apis/orchestration/meta"
orch_v1 "github.com/atlassian/voyager/pkg/apis/orchestration/v1"
stateclient_fake "github.com/atlassian/voyager/pkg/orchestration/client/fake"
"github.com/atlassian/voyager/pkg/orchestration/wiring"
"github.com/atlassian/voyager/pkg/orchestration/wiring/legacy"
"github.com/atlassian/voyager/pkg/orchestration/wiring/registry"
"github.com/atlassian/voyager/pkg/orchestration/wiring/wiringplugin"
"github.com/atlassian/voyager/pkg/util/testutil"
Expand All @@ -27,6 +27,10 @@ const (
fixtureBundleOutputSuffix = ".bundle.output.yaml"
fixtureStateOutputSuffix = ".state.output.yaml"
fixtureGlob = "*" + fixtureStateInputSuffix

testAccount = "testaccount"
testEnv = "testenv"
testRegion = "testregion"
)

func testHandleProcessResult(t *testing.T, filePrefix string) {
Expand Down Expand Up @@ -192,26 +196,26 @@ func entanglerForTests() *wiring.Entangler {
return &wiring.Entangler{
Plugins: registry.KnownWiringPlugins,
ClusterLocation: voyager.ClusterLocation{
Account: legacy.TestAccountName,
Region: legacy.TestRegion,
EnvType: legacy.TestEnvironment,
Account: testAccount,
Region: testRegion,
EnvType: testEnv,
},
ClusterConfig: wiringplugin.ClusterConfig{
ClusterDomainName: "internal.ap-southeast-2.kitt-integration.kitt-inf.net",
KittClusterEnv: "test",
Kube2iamAccount: "test",
},
TagNames: wiring.TagNames{
ServiceNameTag: "service_name",
BusinessUnitTag: "business_unit",
ResourceOwnerTag: "resource_owner",
PlatformTag: "platform",
EnvironmentTypeTag: "environment_type",
},
GetLegacyConfigFunc: getTestLegacyConfig,
Tags: testingTags,
}
}

func getTestLegacyConfig(location voyager.Location) *legacy.Config {
return legacy.GetLegacyConfigFromMap(legacy.TestLegacyConfigs, location)
func testingTags(
_ voyager.ClusterLocation,
_ wiringplugin.ClusterConfig,
_ voyager.Location,
_ voyager.ServiceName,
_ orch_meta.ServiceProperties,
) map[voyager.Tag]string {
tags := make(map[voyager.Tag]string)
return tags
}
2 changes: 0 additions & 2 deletions pkg/orchestration/wiring/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ go_library(
"//:go_default_library",
"//pkg/apis/orchestration/meta:go_default_library",
"//pkg/apis/orchestration/v1:go_default_library",
"//pkg/orchestration/wiring/legacy:go_default_library",
"//pkg/orchestration/wiring/wiringplugin:go_default_library",
"//vendor/github.com/atlassian/smith/pkg/apis/smith/v1:go_default_library",
"//vendor/github.com/atlassian/smith/pkg/util:go_default_library",
Expand All @@ -35,7 +34,6 @@ go_test(
"//cmd/smith/config:go_default_library",
"//pkg/apis/orchestration/meta:go_default_library",
"//pkg/apis/orchestration/v1:go_default_library",
"//pkg/orchestration/wiring/legacy:go_default_library",
"//pkg/orchestration/wiring/registry:go_default_library",
"//pkg/orchestration/wiring/wiringplugin:go_default_library",
"//pkg/orchestration/wiring/wiringutil/knownshapes:go_default_library",
Expand Down
7 changes: 6 additions & 1 deletion pkg/orchestration/wiring/aws/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ type WiringPlugin struct {

OAPResourceTypeName oap.ResourceType
generateServiceEnvironment ServiceEnvironmentGenerator

VPC func(location voyager.Location) *oap.VPCEnvironment
}

func Resource(resourceType voyager.ResourceType,
Expand All @@ -36,6 +38,7 @@ func Resource(resourceType voyager.ResourceType,
clusterServicePlanExternalID servicecatalog.PlanExternalID,
generateServiceEnvironment ServiceEnvironmentGenerator,
shapes svccatentangler.ShapesFunc,
vpc func(voyager.Location) *oap.VPCEnvironment,
) *WiringPlugin {
wiringPlugin := &WiringPlugin{
SvcCatEntangler: svccatentangler.SvcCatEntangler{
Expand All @@ -46,6 +49,7 @@ func Resource(resourceType voyager.ResourceType,
},
OAPResourceTypeName: oapResourceTypeName,
generateServiceEnvironment: generateServiceEnvironment,
VPC: vpc,
}
wiringPlugin.SvcCatEntangler.InstanceSpec = wiringPlugin.instanceSpec
wiringPlugin.SvcCatEntangler.ObjectMeta = wiringPlugin.objectMeta
Expand Down Expand Up @@ -86,7 +90,8 @@ func (awp *WiringPlugin) instanceSpec(resource *orch_v1.StateResource, context *
}

serviceName := serviceName(userServiceName, context)
environment := awp.generateServiceEnvironment(oap.MakeServiceEnvironmentFromContext(context))
vpc := awp.VPC(context.StateContext.Location)
environment := awp.generateServiceEnvironment(oap.MakeServiceEnvironmentFromContext(context, vpc))
return instanceSpec(serviceName, resourceName, awp.OAPResourceTypeName, *environment, attributes, alarms)
}

Expand Down
15 changes: 10 additions & 5 deletions pkg/orchestration/wiring/aws/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,16 @@ const (

// All osb-aws-provider resources are 'almost' the same, differing only in the service/plan names,
// what they need passed in the ServiceEnvironment.
var ResourceTypes = map[voyager.ResourceType]wiringplugin.WiringPlugin{
DynamoDB: wiringutil.StatusAdapter(Resource(DynamoDB, DynamoDBName, DynamoDBClass, DynamoDBPlan, dynamoDbServiceEnvironment, dynamoDbShapes).WireUp),
S3: wiringutil.StatusAdapter(Resource(S3, S3Name, S3Class, S3Plan, s3ServiceEnvironment, s3Shapes).WireUp),
Cfn: wiringutil.StatusAdapter(Resource(Cfn, CfnName, CfnClass, CfnPlan, CfnServiceEnvironment, cfnShapes).WireUp),
}
var (
emptyVPC = func(location voyager.Location) *oap.VPCEnvironment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you construct non-empty VPC and make it environment-specific without LegacyConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're passing a function type based on public types. I won't be using know authowire plugins in the close repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't understand this English :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. Follow the code in the orchestration main in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. If you want to construct plugins dynamically, these static plugin variables simply should not exist, at least in the non-test package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Nail's point. Would be better to move the not-really-ready-to-use instances of plugins (and other "stubby" stuff) into some testing package

Copy link
Contributor Author

@amckague amckague Feb 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think these comments are now outdated.

return oap.ExampleVPC(location.Label, location.Region)
}
DynamoDBPlugin = wiringutil.StatusAdapter(Resource(DynamoDB, DynamoDBName, DynamoDBClass, DynamoDBPlan, dynamoDbServiceEnvironment, dynamoDbShapes, emptyVPC).WireUp)

S3Plugin = wiringutil.StatusAdapter(Resource(S3, S3Name, S3Class, S3Plan, s3ServiceEnvironment, s3Shapes, emptyVPC).WireUp)

CfnPlugin = wiringutil.StatusAdapter(Resource(Cfn, CfnName, CfnClass, CfnPlan, CfnServiceEnvironment, cfnShapes, emptyVPC).WireUp)
)

func cfnShapes(resource *orch_v1.StateResource, smithResource *smith_v1.Resource, _ *wiringplugin.WiringContext) ([]wiringplugin.Shape, bool /* externalErr */, bool /* retriableErr */, error) {
templateName, external, retriable, err := oap.TemplateName(resource.Spec)
Expand Down
23 changes: 0 additions & 23 deletions pkg/orchestration/wiring/ec2compute/common/BUILD.bazel
View file
Open in desktop

This file was deleted.

Loading