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.

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

Initial support of asg and alb. #11

Merged
merged 1 commit into from Sep 25, 2018

Conversation

jsauter
Copy link
Collaborator

@jsauter jsauter commented Sep 18, 2018

Initial implementation of the alb and asg steps.

This pull request contains steps that will facilitate the deployment of an alb and asg. There are some assumptions around scaling groups and the creation of an alb at this point.

generic_pipeline = codepipeline.Pipeline(
"Pipeline",
# Name=chain_context.instance_name,
Name=chain_context.instance_name,
Copy link
Owner

Choose a reason for hiding this comment

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

How come this is being added back in ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Conflict error, will fix.

alb_sg = ALB_SG_NAME % self.name

# TODO: this SG shouldn't be required.. test.
template.add_resource(ec2.SecurityGroup(
Copy link
Owner

Choose a reason for hiding this comment

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

TODO!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should probably get to that eventually in the infra blueprint that this came from.

Copy link
Owner

Choose a reason for hiding this comment

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

we're not using the one where it came from anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed this one, and I don't see any ramifications in the net result, so I will leave it out.


def create_default_target_group(self, template):
template.add_resource(alb.TargetGroup(
TARGET_GROUP_DEFAULT % self.name,
Copy link
Owner

Choose a reason for hiding this comment

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

If we run two instances of this, ie a bswift and a sauter instance separately, this looks like we'll get name collisions. Prefix support in cumulus is probably a bigger discussion.

EvaluateTargetHealth=False,
),
Name=Join("", [
Ref("stackname"),
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if this was brought over from the builder project but we have a concept of self.name, chain_context.instance_name, and now Ref("stackname").

Can we streamline these ? Is stackname really required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not required, can use the other options if they are providing a stackname.

FromPort=self.port_to_open,
ToPort=self.port_to_open,
CidrIp=self.cidr,
GroupId=Ref(chain_context.metadata[META_SECURITY_GROUP_NAME])
Copy link
Owner

Choose a reason for hiding this comment

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

This is a slight departure from a convention the pipeline introduced.. it's not a big deal but might be good to streamline as much as possible.
Instead of popping the name on the metadata, pipeline put the Ref object on it. Which makes the most sense? Should the pipeline change or should this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found putting it on the metadata was pretty intuitive!

Copy link
Owner

Choose a reason for hiding this comment

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

so did I. The question was should we put a string or a ref. If we stick with the precedence in master we'd put a Ref object on the metadata.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Either works. I don't see either case being more or less easy to work with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will make this one use a ref on the metadata.

self.meta_data = meta_data
self.instance_profile_name = instance_profile_name
self.vpc_id = vpc_id

Copy link
Owner

Choose a reason for hiding this comment

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

It seems like the following could be derived, and at least made optional here, to simplify the consumer requirements.

  • asg_name
  • launch_config_name
  • instance_profile_name

asg_sg_list = [self._get_launch_configuration_security_groups(chain_context.metadata[META_SECURITY_GROUP_NAME])]

parameters = {
'ImageId': FindInMap('AmiMap',
Copy link
Owner

Choose a reason for hiding this comment

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

ugh, so like stacker and parameters, stacker has an opinion on mappings.

These mappings are required to exist in stacker.yaml, for the sample code here. This poses another problem if cumulus is going to be non-stacker specific..

I think the best way to do this would be to supply a way to generate mappings in the way cumulus wants. Something for a separate story/feature.

Copy link
Owner

Choose a reason for hiding this comment

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

issue #14 created.

Copy link
Owner

Choose a reason for hiding this comment

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

for this PR we should probably just document an example mapping section in the class docs of the class.

Values=[
Ref("stackname"),
"-",
Ref("namespace"),
Copy link
Owner

Choose a reason for hiding this comment

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

namespace, and stackname again. Are these bleeding in from stacker? Namespace is a stacker concept.. although the code that this was brought in from.. (the builder) didn't have a working host_pattern. So maybe this could be tackled separately.. as I'm guessing this isn't tested in this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

A fix / documents here are still pending.

from cumulus.chain import step


class Port(step.Step):
Copy link
Owner

Choose a reason for hiding this comment

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

The name here is too generic. This is specifically a class for ASG--> ALB traffic.

I think it's ok to nuke this class and have this done directly where the target group is added. Likely in the consumer code there are ports being passed into a few places.

Copy link
Owner

Choose a reason for hiding this comment

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

please fix

from cumulus.util.tropo import TemplateQuery


class Role(step.Step):
Copy link
Owner

Choose a reason for hiding this comment

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

This role is specific to an instance profile, so we might want to name it specifically that.

There will be many other things that want to call themselves "Role_xxx".

Copy link
Owner

Choose a reason for hiding this comment

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

please fix


# todo: why is this not allowing a reference?

name = 'CumulusTargetGroup'
Copy link
Owner

Choose a reason for hiding this comment

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

This will cause collisions on multiple instances.

Copy link
Owner

Choose a reason for hiding this comment

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

please fix

from cumulus.steps.ec2 import META_SECURITY_GROUP_NAME, META_TARGET_GROUP_NAME


class LaunchType:
Copy link
Owner

Choose a reason for hiding this comment

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

duplication, also exists in cumulus/types.py

Copy link
Owner

Choose a reason for hiding this comment

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

remove one of them

the_chain.add(ingress_rule.IngressRule(
name="SgCidrAccess",
port_to_open="22",
asg_sg_name="sg1" + self.name,
Copy link
Owner

Choose a reason for hiding this comment

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

as above, this is the client code that gets really confusing, having to pass around asg_sg_name in a bunch of places. When it gets derived and pulled into one of the steps we won't have to worry about it here.

I think the user needs to know too much about steps and sequencing

@brettswift brettswift mentioned this pull request Sep 20, 2018
@brettswift brettswift merged commit 460662e into brettswift:master Sep 25, 2018
brettswift pushed a commit that referenced this pull request Sep 25, 2018
brettswift pushed a commit that referenced this pull request Sep 25, 2018
Add flake8 to the ci build
brettswift added a commit that referenced this pull request Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants