Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(moniker): fix cluster if detail is set to empty via SpEL #1832

Merged
merged 2 commits into from
Dec 5, 2017
Merged

fix(moniker): fix cluster if detail is set to empty via SpEL #1832

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
513b1ea
fix(moniker): fix cluster if detail is set to empty via SpEL
asher Dec 5, 2017
a9dab66
Merge branch 'master' into moniker
asher Dec 5, 2017
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
7 changes: 6 additions & 1 deletion ...ouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/MonikerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver;
import com.netflix.spinnaker.orca.pipeline.model.Stage;
import org.apache.commons.lang3.StringUtils;
import org.springframework.stereotype.Component;


Expand Down Expand Up @@ -56,7 +57,11 @@ public String getClusterNameFromStage(Stage stage, String fallbackFriggaName) {

static public Moniker monikerFromStage(Stage stage) {
if (stage.getContext().containsKey("moniker")) {
return (Moniker) stage.mapTo("/moniker", Moniker.class);
Moniker moniker = stage.mapTo("/moniker", Moniker.class);
if (moniker.getCluster().endsWith("-")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem quite right ... the Moniker shouldn't have an invalid cluster in the first place.

Can we fix it at Moniker creation time vs. fixing it on read afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue in question is triggered when a pipeline.json contains something like:

          "moniker": {
            "app": "spintest",
            "cluster": "spintest-asher-${parameters.foo ?: ''}",
            "detail": "${parameters.foo ?: ''}",
            "stack": "asher"
          },

Unless we don't want to support an optional parameterized cluster detail, seems like something needs to happen after SpEL evaluation in orca.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lwander @andrewbackes Is it the expected behavior for deck to pre-construct a cluster of the form application-stack-details?

This happens in a deploy stage if you use a SPeL expression for details.

Copy link
Member

Choose a reason for hiding this comment

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

@asher - Deck will not always constructs a cluster name of the form app-stack-detail. Only providers still using frigga follow that scheme. The other Namers treat cluster as a free form field. Like @ajordens pointed out, I think it would be safest to handle this when the moniker is created. Something like:

          "moniker": {
            "app": "spintest",
            "cluster": "spintest-asher${parameters.foo ? '-' + parameters.foo : ''}",
            "detail": "${parameters.foo ?: ''}",
            "stack": "asher"
          },

There may be some unexpected behavior with this PR because users can still set the cluster value to whatever they want. Clouddriver does not reject cluster names that end in hyphen. But anything in Orca that uses the monikerFromStage() helper method will be stripping the hyphen. The monikers will not match. I'm not sure what all of the downstream consequences will be, but I think there will be some. TrafficGuard comes to mind.

Copy link
Contributor Author

@asher asher Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@andrewbackes the example you give would work with this PR reverted, but to automatically create that would require deck to parse and modify user provided SPeL expressions when generating a moniker. I'm hesitant to go down that path.

Can we avoid pre-constructing moniker.cluster entirely if using frigga naming?

Copy link
Member

Choose a reason for hiding this comment

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

In the spirit of sharing information, here is a screenshot of the k8s v2 deploy wizard.

k8s-v2 deploy stage

Disclaimer: This is an early draft and is will probably change a few times.

Copy link
Member

Choose a reason for hiding this comment

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

My only concern is around Orca chopping off the hyphen but clouddriver not. Seems like that would be something very hard to debug if it ever came up for somebody. I'd prefer consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually edited my test redblack pipeline that contained the following (auto generated via the aws deploy wizard):

          "moniker": {
            "app": "spintest",
            "cluster": "spintest-asher-${parameters.foo ?: ''}",
            "detail": "${parameters.foo ?: ''}",
            "stack": "asher"
          },

And removed the pre-generated cluster to get:

          "moniker": {
            "app": "spintest",
            "detail": "${parameters.foo ?: ''}",
            "stack": "asher"
          },

And, oops - got a NPE from monikerFromStage() thanks to this very PR. Fixed the NPE and everything worked as it would have pre-moniker with an empty parameters.foo.

So, how about if instead of this PR, we modify deck to stop pre-constructing moniker.cluster in the form of application-stack-details? My test obviously wasn't exhaustive in confirming that there aren't dependencies on the presence of a moniker.cluster if moniker exists, but it feels like the right direction to go in.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry i've been OOO - what you're suggesting sounds best to me asher, and this is the direction the k8s provider takes moniker.

Copy link
Member

Choose a reason for hiding this comment

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

@asher - I think that sounds like a good approach.

moniker.setCluster(StringUtils.stripEnd(moniker.getCluster(), "-"));
}
return moniker;
} else {
return null;
}
Expand Down
3 changes: 2 additions & 1 deletion ...ddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/pipeline/support/StageData.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.kato.pipeline.support

import com.netflix.frigga.autoscaling.AutoScalingGroupNameBuilder
import com.netflix.spinnaker.moniker.Moniker
import org.apache.commons.lang3.StringUtils

class StageData {
String strategy
Expand Down Expand Up @@ -47,7 +48,7 @@ class StageData {

String getCluster() {
if (moniker?.cluster) {
return moniker.cluster
return StringUtils.stripEnd(moniker.cluster, "-")
} else {
def builder = new AutoScalingGroupNameBuilder()
builder.appName = application
Expand Down