Refactor to remove legacyconfig object #163

amckague · 2019-02-13T11:57:46Z

At best legacyconfig belongs to the individual plugins, simplify the
existing wiring contract by removing it from the wiring context.
ec2compute v2 no longer has a v1 version present so remove the extra
package, add an api to allow outside users to set the default set of
tags passed in the entangler context

amckague · 2019-02-13T12:10:51Z

pkg/orchestration/wiring/ec2compute/v2/plugin.go

+func New() *WiringPlugin {
+	return &WiringPlugin{
+		DeveloperRole: func(_ voyager.Location) []string {
+			return []string{"arn:aws:iam::123456789012:role/micros-server-iam-MicrosServer-ABC"} //example


Legacy config is a major issue for manual or experimental testing of autowiring functions. You have to use the internal build to access this data.
I can probably move most or all of these to envvar lookups.

Tthe way shapes hold things together also practically requires all the functions be in this repo so you can find and reference them simply.

I don't disagree, it would be nice to replace LegacyConfig with resource-specific configuration.
The problem is, as I said above, that at the moment there is no way to pass configuration to autowiring plugins, and at the moment we are abusing WiringContext and LegacyConfig as a global per-environment configuration.

There is a ticket to make autowiring functions accept configuration into their constructor. Even with legacy config completely gone we'd still need this functionality.

That's exactly what I meant, if it wasn't clear :)

We build the list of plugins. All of their "constructors" are accessible. There is not requirement to use known plugins object.

Then the first trivial change would be to pass LegacyConfig to plugin constructors which use information from it, and remove it from the context? This would be a global LegacyConfig I suppose (not location-specific), but that's ok. It could also be a function providing a LegacyConfig for a given location. This should be a relatively simple isolated change, easy to review.

Actually, we already have this function: https://github.com/atlassian/voyager/blob/master/pkg/orchestration/wiring/entangler.go#L161-L173
So we just need to pass this function to plugin constructors, instead of Entangler invoking it and adding to the context. The only problem again is that list of plugins is static at the moment IIRC, i.e. you can't pass a different configuration to it for testing vs production.

Look at the main function for orchestration. It takes registry.KnownWiringPlugins as an arg of type map[voyager.ResourceType]wiringplugin.WiringPlugin. There is no reason this needs to be the same map inside the close source repo.

Also notice that master of this repo currently stubs legacy config with a nothing but empty strings.

amckague · 2019-02-13T12:12:51Z

pkg/orchestration/wiring/entangler.go

-	// We hack this Atlassianism in here rather than putting in user
-	// tags in the configmap because it's at the same level as legacyConfig
-	// (i.e. want to make it obvious they should be removed at the same time).
-	legacyEnvironmentTagName = "environment"


Get to kill some todo's with this move as well

amckague · 2019-02-13T12:13:31Z

pkg/orchestration/wiring/entangler.go

@@ -68,12 +59,19 @@ type TagNames struct {
 	EnvironmentTypeTag voyager.Tag
 }

+type TagGenerator func(


This API is possibly too broad. Please comment on possible alternatives.

amckague · 2019-02-13T12:14:46Z

pkg/orchestration/wiring/entangler.go

-	tags[en.TagNames.ResourceOwnerTag] = context.ServiceProperties.ResourceOwner
-	tags[en.TagNames.EnvironmentTypeTag] = string(location.EnvType)
-	tags[en.TagNames.PlatformTag] = "voyager"
-	tags[legacyEnvironmentTagName] = legacyConfig.MicrosEnv


This lookup is what requires the tags generation to be a function and not just a static map. The other keys could be seen as opinionated defaults.

amckague · 2019-02-13T12:15:30Z

pkg/orchestration/wiring/k8scompute/plugin.go

+func New() *WiringPlugin {
+	return &WiringPlugin{
+		VPC: func(location voyager.Location) *oap.VPCEnvironment {
+			return oap.ExampleVPC(location.Label, location.Region)


VPCEnvironment has a much smaller surface area than legacy config.

amckague · 2019-02-13T12:17:57Z

pkg/orchestration/wiring/postgres/plugin.go

@@ -158,7 +163,7 @@ func (p *WiringPlugin) WireUp(resource *orch_v1.StateResource, context *wiringpl

 // instanceParameters constructs ServiceInstance parameters.
 // sharedDbDep may be nil
-func instanceParameters(resource *orch_v1.StateResource, context *wiringplugin.WiringContext, sharedDbDep *wiringplugin.WiredDependency) ([]byte, bool /* externalErr */, bool /* retriable */, error) {
+func (p *WiringPlugin) instanceParameters(resource *orch_v1.StateResource, context *wiringplugin.WiringContext, sharedDbDep *wiringplugin.WiredDependency) ([]byte, bool /* externalErr */, bool /* retriable */, error) {


@nilebox I opted for a receiver param over an additional function param to avoid clashing with your plans in this place. I would prefer a function param if it doesn't get in the way.

that's fine

amckague · 2019-02-13T12:18:37Z

pkg/orchestration/wiring/wiringplugin/types.go

@@ -122,12 +121,6 @@ type StateContext struct {
 	// from the EntanglerContext.
 	Location voyager.Location

-	// LegacyConfig is read by a function specified in the entangler struct.
-	// TODO this is a temporary container for 'stuff that's in Micros config.js'.


ash2k

I'm not sure I understand the intent. I think you are doing two big things in a single PR - moving stuff from common and changing the legacy config. Can it be done in separate steps? Or we need to sit down together and go though it because I cannot tell what code is new and what code has just been moved. Too many changes.

ash2k · 2019-02-13T12:24:47Z

pkg/orchestration/wiring/ec2compute/v2/plugin.go

+	bindingOutputInstanceProfileARNKey = "InstanceProfileARN"
+	inputParameterEnvVarName           = "secretEnvVars"
+
+	MaximumServiceNameLength = 26
 )

 // HACK: Some tags the EC2 provider doesn't like, because it wants to
 // set them itself... (NB handles business_unit/resource_owner separately)
 // We only really worry about the tags that we're likely to set here
 // (it's ok if the user errors out from the provider).
 var forbiddenTags = map[string]struct{}{


set of strings?

ash2k · 2019-02-13T12:33:22Z

Legacy configuration should be moved to providers, we have tickets to do that. But I'm not opposed to refactoring this if it makes our life easier.

amckague · 2019-02-13T23:22:35Z

pkg/orchestration/wiring/ec2compute/v2/plugin.go

@@ -159,7 +194,27 @@ func constructComputeParameters(origSpec *runtime.RawExtension, iamRoleRef, iamI
 	return rawExtension, false, false, nil
 }

-func WireUp(stateResource *orch_v1.StateResource, context *wiringplugin.WiringContext) wiringplugin.WiringResult {
+func New() *WiringPlugin {


The members of this struct and it's use are the only changes to this class, everything else is just moving code from the extra package.

nilebox

I like the idea of replacing LegacyConfig with resource-specific configuration for wiring plugins, but at the moment it's just stubs without actual solution for per-environment plugin configurations.

So instead of starting with deletion of LegacyConfig, I would first solve the problem of making wiring plugins configurable (e.g. via additional AddFlags function, plus some shared flags for common configuration replacing LegacyConfig), then start migrating function to it, and once LegacyConfig is not used anymore, we can delete it.

nilebox · 2019-02-13T23:15:52Z

pkg/orchestration/wiring/postgres/plugin.go

-	return &WiringPlugin{}
+	return &WiringPlugin{
+		Environment: func(_ voyager.Location) string {
+			return "microstestenv"


shouldn't you accept environment as parameter in constructor instead?

For all of them yes.

nilebox · 2019-02-13T23:16:06Z

pkg/orchestration/wiring/postgres/plugin.go

@@ -158,7 +163,7 @@ func (p *WiringPlugin) WireUp(resource *orch_v1.StateResource, context *wiringpl

 // instanceParameters constructs ServiceInstance parameters.
 // sharedDbDep may be nil
-func instanceParameters(resource *orch_v1.StateResource, context *wiringplugin.WiringContext, sharedDbDep *wiringplugin.WiredDependency) ([]byte, bool /* externalErr */, bool /* retriable */, error) {
+func (p *WiringPlugin) instanceParameters(resource *orch_v1.StateResource, context *wiringplugin.WiringContext, sharedDbDep *wiringplugin.WiredDependency) ([]byte, bool /* externalErr */, bool /* retriable */, error) {


that's fine

nilebox · 2019-02-13T23:18:00Z

cmd/orchestration/app/controller_constructor.go

@@ -30,9 +29,9 @@ import (
 )

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


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

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.

nilebox · 2019-02-13T23:18:56Z

cmd/orchestration/app/controller_constructor.go

@@ -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{


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

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.

nilebox · 2019-02-13T23:20:26Z

pkg/creator/handler.go

@@ -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"


This is also unrelated to LegacyConfig? separate PR please.

nilebox · 2019-02-13T23:22:07Z

pkg/orchestration/wiring/aws/resources.go

-	Cfn:      wiringutil.StatusAdapter(Resource(Cfn, CfnName, CfnClass, CfnPlan, CfnServiceEnvironment, cfnShapes).WireUp),
-}
+var (
+	emptyVPC = func(location voyager.Location) *oap.VPCEnvironment {


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

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

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

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

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.

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

Think these comments are now outdated.

nilebox · 2019-02-13T23:24:43Z

pkg/orchestration/wiring/ec2compute/v2/plugin.go

+func New() *WiringPlugin {
+	return &WiringPlugin{
+		DeveloperRole: func(_ voyager.Location) []string {
+			return []string{"arn:aws:iam::123456789012:role/micros-server-iam-MicrosServer-ABC"} //example


I don't disagree, it would be nice to replace LegacyConfig with resource-specific configuration.
The problem is, as I said above, that at the moment there is no way to pass configuration to autowiring plugins, and at the moment we are abusing WiringContext and LegacyConfig as a global per-environment configuration.

At best legacyconfig belongs to the individual plugins, simplify the existing wiring contract by removing it from the wiring context. ec2compute v2 no longer has a v1 version present so remove the extra package, add an api to allow outside users to set the default set of tags passed in the entangler context

Overly complicated solution that doesn't solve removing legacy config from the entagler api

amckague · 2019-02-14T00:51:07Z

@nilebox @ash2k separate commits for moving common vs removing legacy. See cmd/orchestration/app main.go for an idea of how I'll be using this code inside our closed source repo.

pkg/orchestration/wiring/wiringutil/oap/types.go

nilebox · 2019-02-14T01:40:40Z

cmd/orchestration/app/main.go

@@ -23,18 +25,23 @@ const (
 )

 func Main() {
-	CustomMain(emptyLegacyConfigFunc, registry.KnownWiringPlugins)
+	CustomMain(registry.KnownWiringPlugins(


This looks better 👍

nilebox · 2019-02-14T01:46:25Z

@amckague the code looks better now (there is still a lot unrelated changes, so I would prefer to have separate PRs rather than commits, but that's up to you).

My concern though is that if we merge this change, we won't be able to use the latest master until we copy bits from deleted LegacyConfig to a Stash repo and construct all the things there. As alternative, we could keep such code inside this repo for now as a default behavior, and once we rewrite the code in Stash to explicitly pass configuration, we can get rid of this backward compatibility.

i.e. I propose to keep KnownWiringPlugins constructless and mark it as deprecated, and add a new method that will accept configuration that we need to migrate to.

ychen-atlassian · 2019-02-14T02:16:01Z

pkg/orchestration/wiring/registry/registry.go

-	sqs.ResourceType:            wiringutil.StatusAdapter(sqs.WireUp),
-	asapkey.ResourceType:        wiringutil.StatusAdapter(asapkey.New().WireUp),
-	apiplatformdns.ResourceType: wiringutil.StatusAdapter(platformdns.New().WireUp),
+func KnownWiringPlugins(


Sort of related to Nail's comment up https://github.com/atlassian/voyager/pull/163/files#r256667823, we only need to do this because we need to pass a bunch of parameters through the orchestration constructor. When we end up extracting these I assume we'll have a custom orchestration cmd in stash voyager that combines the wiring (like how we combine smith plugins and smith)

I have some ideas of how we can have configuration passed to plugins from a shared config file but in a typed way. I view passing functions is a temporary shortcut - instantiating code (orchstration) should not have any knowledge about what configuration is needed i.e. that "configuration" cannot be a function. It should be just a blob of data. But this is one of the next steps.

And same with smith - we need a way to pass configuration to plugins atlassian/smith#319 without recompiling smith to change that configuration. We obviously don't want to have a binary per environment, for example.

ychen-atlassian · 2019-02-14T02:40:05Z

pkg/orchestration/wiring/ec2compute/v2/plugin.go

@@ -2,34 +2,59 @@ package v2



Now we can get rid of v2 altogether and just call it ec2compute!

just wait for V3 :)
just kidding - yeah, version is not needed anymore (but again, I would prefer this change to be a separate PR)

In our glorious future of go modules that would be breaking the version number in the package path requirement. Also it's separate commits.

ash2k

conflict

amckague · 2019-02-15T07:18:36Z

pkg/orchestration/wiring/wiringutil/oap/wiring.go

-func MakeServiceEnvironmentFromContext(context *wiringplugin.WiringContext) *ServiceEnvironment {
-	config := context.StateContext.LegacyConfig
-	location := context.StateContext.Location
+func MakeServiceEnvironmentFromContext(context *wiringplugin.WiringContext, vpc *VPCEnvironment) *ServiceEnvironment {
 	serviceProperties := context.StateContext.ServiceProperties
 	alarmEndpoints := PagerdutyAlarmEndpoints(
 		serviceProperties.Notifications.PagerdutyEndpoint.CloudWatch,
 		serviceProperties.Notifications.LowPriorityPagerdutyEndpoint.CloudWatch)

 	return &ServiceEnvironment{


Look here for the vpc info

amckague requested review from ash2k, nilebox and ychen-atlassian February 13, 2019 11:57

amckague commented Feb 13, 2019

View reviewed changes

ash2k reviewed Feb 13, 2019

View reviewed changes

amckague commented Feb 13, 2019

View reviewed changes

nilebox previously requested changes Feb 13, 2019

View reviewed changes

amckague added 2 commits February 14, 2019 11:11

The common package for ec2 is no longer common, remove it

f708a33

amckague force-pushed the remove_legacy branch from 0534b03 to 7d1bbdf Compare February 14, 2019 00:15

Take location specific configuration as constructor args

3e64e43

amckague force-pushed the remove_legacy branch from d44c5f9 to 3e64e43 Compare February 14, 2019 01:02

ash2k reviewed Feb 14, 2019

View reviewed changes

pkg/orchestration/wiring/wiringutil/oap/types.go Outdated Show resolved Hide resolved

Remove remaining stub

c613565

nilebox reviewed Feb 14, 2019

View reviewed changes

ychen-atlassian previously approved these changes Feb 14, 2019

View reviewed changes

amckague dismissed ychen-atlassian’s stale review via d8d32dc February 14, 2019 03:40

Merge branch 'master' into remove_legacy

e51288a

amckague force-pushed the remove_legacy branch from d8d32dc to e51288a Compare February 14, 2019 03:43

ychen-atlassian previously approved these changes Feb 14, 2019

View reviewed changes

ash2k previously approved these changes Feb 14, 2019

View reviewed changes

Merge branch 'master' into remove_legacy

3329943

amckague dismissed stale reviews from ash2k and ychen-atlassian via 3329943 February 15, 2019 04:57

ychen-atlassian approved these changes Feb 15, 2019

View reviewed changes

amckague merged commit 8314b18 into master Feb 15, 2019

amckague commented Feb 15, 2019

View reviewed changes

Refactor to remove legacyconfig object #163

Refactor to remove legacyconfig object #163

Conversation

amckague commented Feb 13, 2019

amckague Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckague Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckague Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Feb 13, 2019

Choose a reason for hiding this comment

nilebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ychen-atlassian Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckague Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckague commented Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

nilebox commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckague Feb 13, 2019 •

edited

Loading

amckague Feb 14, 2019 •

edited

Loading

amckague Feb 13, 2019 •

edited

Loading

ychen-atlassian Feb 14, 2019 •

edited

Loading

amckague Feb 14, 2019 •

edited

Loading

amckague commented Feb 14, 2019 •

edited

Loading