Code Monkey home page Code Monkey logo

Comments (26)

bobveringa avatar bobveringa commented on August 26, 2024 1

Alright, I think I can work with this. I'll start prototyping in python first because it's the language I'm fastest in currently. I'll contact you for setting up a call, that might help speed things up.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024 1

Oke looking at what Basti is doing, it might not be such a great idea to essentially rebuild the entire init command but for CDK. This would only make sense if making a native CDK version had significant benefits. Because you are doing the work maintaining basti, however, if we decide to make a native version supporting CDK, then all new features would also have to be supported by the CDK construct. While I like programming, I don't think anyone likes programming the same thing twice but with very slight variations.

Instead, we might want to take a closer look at the interface, and determine what do people who want to use this, really need. And I think that in most cases this is not that exciting.

So, I propose the following, we expand the functionality of basti to allow JSON responses to CLI input. The CDK construct then simply1 calls basti through a lambda, and uses the JSON response to make interfaces available in CDK. These are IInterface types classes, so the actions that you can execute on those are a bit more limited, but I think that it should be more than enough for what people are using this for.

The arguments that the construct takes will then be the same as what the basti init command uses. And any bug fixes and new features are available "for free" to the construct.

This method would also allow far easier porting to other infrastructure-as-code platforms, as you are only building a wrapper for an existing tool compared to rebuilding a significant amount of functionality.

Another option instead of JSON is to allow functions to be called directly. However, This might require more work to maintain.

@BohdanPetryshyn Your thoughts on this?

1 Not that simple but not that hard either.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024 1

I certainly understand your point of view, but I don't agree with all your points. This might also be because I did a poor job explaining the concept and the differences.

CDK constructs have 3 levels. L2 constructs often consist of the interface and the implementation, E.G. IRole and Role. The Role construct creates a "Physical" role through CloudFormation, while the IRole just provides an interface to do things with the role. These L2 constructs exist for all the important resources created by basti:

  • IInstance/Instance
  • ISecurityGroup/SecurityGroup
  • IRole/Role

The interface constructs allow users to the most things you would want to do with these constructs. Like adding an inline policy to a role or getting the instance ARN to create roles with permissions to access the instance through SSM. In my opinion, the additional benefit of creating these instances through CDK/CloudFormation/IaC provides very little benefit on top of the interfaces on their own.

  1. Should you provide more control when setting up Basti through IaC? I, personally, don't think so, I want to be able to use some of the information from Basti in my managed permissions, and maybe I'd want to add some policies to reduce/modify the permissions. But I don't see the need for fully custom security groups, perhaps this is just something I am not seeing the need for. Just allowing the data from Basti to be used in CDK should provide enough flexibility.

  2. Yes, that would seem like a requirement, But on the other hand, this seems like a great feature to add anyway. Adding support for this enhances the experience of both regular users and CDK users.

  3. CDK and Terraform are indeed tools that mange resource creation etc. but to say they excel could be a bit of a stretch. Especially CDK gets confused all the time with security groups. This could just be because of the way our groups are managed, but it is very annoying at times. Basti already does this great in my opinion, and any fixes and improvements to this would also be passed along to the CDK construct for Basti.

  4. This one is probably the most important from the list. But you could also make the case that it doesn't matter. You are already using a third-party construct, and especially when you are using JSII to run typescript constructs in other languages, you can already barely tell what a construct is doing (this also applies to AWS CDK native constructs). I know what it is doing, I'm getting a Basti host, how it does that is not relevant (at least to me)

In my experience, whenever you try to maintain 2 (or more) versions of something that try to do the same thing, 1 of them is always going to up neglected. I would much rather have a working construct that wasn't entirely native than have a fully native construct that no longer works with CLI because there was an update that added new features that are not yet supported in CDK.

As for using CDK within Basti to avoid reuse of resources, I would highly recommend against this, the power of Basti (in my opinion, you could have another view on this) is that it just works without any complicated stuff, adding CDK to this might make everything complicated for users.

I'm not aware of other "CLI-first" tools, but I also can't think of any other CLI tools that I would want a construct for 😄.

If you think that it is best to go for the native option, then I'll happily help with implementation, but I'm just afraid that the maintenance will be significant for a tiny group of users.

What are your thoughts on this @BohdanPetryshyn

from basti.

bobveringa avatar bobveringa commented on August 26, 2024 1

Yeah, it certainly not an easy decision. Everything is a tradeoff.

It could look at setting that up, and also start taking a look at how JSII would factor into this. I'm uncertain when I have time this week, some other items I'm working on have changed priority.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024 1

A quick summary of our call:

There is a significant difference between how CDK and terraform work, CDK is a lot more dynamic while terraform is more static. Because we want these interfaces to mostly match, we have determined that it is best to make both the CDK and terraform implementations of basti native, without using the CLI. With the expectation that maintenance will be fairly low.

The solution will be implemented with 2 constructs, 1 for the security group and one for the bastion host.

Some structure changes will be made to the repo to allow for multiple modules within the same repo.

I will prototype in python first to validate the concept, then move over to typescript and work on an implementation that works with JSII.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Hello @bobveringa 👋

Thank you for submitting this idea! It will bring a lot of value to the project. In fact, I've been considering creating a Terraform module. Ideally, we would need support for all the major infrastructure-as-code tools.

I will need some time to refresh my knowledge of CDK and provide more precise technical feedback. I'll make sure to reply here tomorrow evening.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

I'd suggest the following interface for Basti CDK constructs:

  1. BastiInstance - Bastion EC2 instance
    • Inputs / Props
      1. subnetId
    • Outputs / Public fields
      1. id
      2. vpcId
      3. subnetId
      4. securityGroupId
  2. BastiRdsInstanceAccessSecurityGroup - Security group for RDS instance access
    • Inputs / Props
      1. bastiInstanceId - Basti instance the security group will allow connection from
      2. rdsInstanceId - Construct will read the configuration of the RDS instance. For example, the DB port
    • Outputs / Public fields
      1. id - basti-specific Id
      2. securityGroupId
      3. bastiInstanceId
      4. rdsInstanceId
  3. BastiRdsClusterAccessSecurityGroup - Security group for RDS cluster access (symmetric to BastiRdsInstanceAccessSecurityGroup)

Users will use it in the following way:

  1. RDS instance (similar for RDS cluster or any other supported target)
    1. Create BastiInstance (or use a previously created one)
    2. Create BastiRdsInstanceAccessSecurityGroup
    3. Add the new access security group to the RDS instance security groups
  2. Custom target
    1. Create BastiInstance (or use a previously created one)
    2. Manually create an "access security group" using native CDK constructs. The security group must allow connections from the BastiInstance to the target's port. It can use the BastiInstance security group or a range of subnet IPs as a connection source
    3. Add the access security group to the target

I will try to explain some of the nuances in the form of a Q&A

  1. Why BastiInstance takes a subnetId instead of creating its own VPC? (as you initially suggested)
    The bastion instance has to be in the same VPC as the connection target (if we don't use VPC peering). I think it's okay to let the user decide where he wants the instance to be created.
  2. How will these constructs enable more granular control for the user?
    In the future, we can add more and more props to override the default components: IAM role, security group, and even EC2 user data / init script.
  3. Why do we need separate constructs for the bastion instance and access security groups?
    I was writing the CLI in a way to allow maintaining multiple bastion instances in the same VPC. This might be helpful for achieving more fine-grained access control on the networking level. Due to a limited number of security groups that could be attached to each resource in AWS (5 SGs for RDS instance, for example), Basti access security groups have to be reused when multiple bastion instances are used to connect to a single target.
  4. Should we use IDs or whole constructs/objects as inputs?
    To be honest, I don't have much experience with CDK, so I hope you, @bobveringa, can suggest how to better implement the interface.

Please, share your thoughts!

from basti.

bobveringa avatar bobveringa commented on August 26, 2024

At first look, this interface seems well though out. I must admit that I lack the knowledge of how basti works internally, to give proper feedback on this proposal.

I have some ideas on how to implement this, but I would need a little more information about how this configuration works. Or maybe there are already docs for this and I just missed them.

As for your last note. I, personally, like to work with entire objects, as long as these objects also support being created from ID. It allows you more flexibility to change the way some components work, with IDs are you are a lot more locked into something. It also depends on what types of constructs you are dealing with and how mature they are within CDK. In this case, where we are most likely working with IAM, EC2, and RDS, the constructs are (mostly) mature enough to use full objects.

I have worked on a few CDK projects, I think the best way to go is to implement the interface/constructs in CDK and then use JSII to also support other languages. Partly looking out for my own interests, I'd say that typescript and python should cover it initially, as this is what most people use to develop CDK (at least in my experience).

I'm working on this issue in part on company time and (depending on how things go) a little in my personal time. Looking at my current schedule, I have some time to look into this tomorrow. I'll start by figuring out what resources basti creates and how those translate to AWS CDK. Any pointers you can give here are most welcome.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

I have some time to look into this tomorrow

Nice to hear that, @bobveringa!

Unfortunately, there're no docs for developers. Thanks to people like you, who show interest in contributing to the project, I will try to prioritize documentation soon.

We could have a quick call where I would give you an introduction to the project and share some more thoughts on the future implementation. You can contact me on LinkedIn if you're interested: https://www.linkedin.com/in/bpetryshyn/

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Basically, we need to replicate what the init command does.

The two functions the command uses to actually create resources are createBasiton and InitTarget#allowAccess. The later one has multiple (but very similar) implementations for different connection targets (DbInstanceInitTarget, DbClusterInitTarget, and `CustomInitTarget).

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

I assigned you so that it's visible to other potential contributors that somebody's currently working on this

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

@bobveringa, the problem you described is of great importance, and it would be wonderful to see it resolved. The suggested approach sounds reasonable to me, but I will need some time to provide detailed technical feedback. I will be able to take a closer look at it this weekend.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024

To add some context. The CLI would take an additional option (at least for the init command) that allows the output to be configured to JSON. In this instance all basti variables would need to be passed as command line arguments. The command executed might look something like

basti init [params for init here] --output json

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Hello @bobveringa! 👋

First of all, I'm super excited to collaborate with you, and I'm committed to dedicating more time to the Basti CDK project. Usually, I'm available during weekends, but this time I had some long-planned commitments.

While considering the suggested solution, I came across a couple of potential issues:

  1. Flexibility: The infrastructure-as-code approach of initializing Basti aims to provide users with more control over the setup, including IAM roles, SGs, and more. If we incorporate this into the basti init command, we'll need to expand its functionality.

  2. Resource cleanup: To handle precise resource cleanup, we would also need to enhance the basti cleanup command. Currently, it only deletes all the Basti-related resources from your account at once.

  3. Stability: Tools like CDK and Terraform excel at managing resource creation, updates, and deletions. They are optimized for these tasks and handle various edge cases effectively. Leveraging this functionality would be valuable.

  4. Transparency: If the Basti construct is simply a composition of other AWS resources, it would be easier for users to comprehend its workings. Including basti init within the construct might create a similar feeling to using the Basti CLI when all other infrastructure is controlled using an infrastructure-as-code solution.

I agree that duplicating new features and bug fixes between the Basti CLI and infrastructure-as-code packages will come with its costs. However, I believe it will be worth it, especially considering the declarative nature of CDK constructs and Terraform modules. Later, we might even reuse CDK in Basti CLI to eliminate some of the duplication.

Please, share your thoughts! Maybe you know any other "CLI-first" tools that leverage a similar approach?

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Most of the logic in the basti init command is connected to handling user input and analyzing resources in the AWS account to automate the setup. None of these is needed in the CDK construct - users will pass all the necessary input manually

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Hello @bobveringa and thank you for such a detailed reply!

This is definitely not an easy decision to make. I really value your initiative and time so I want to make sure we choose the right path here and avoid re-implementation down the line. I need one more evening to research CDK/CloudFromation Custom Resources and future Terraform module implementation options.

Maybe you could start setting up the CDK package boilerplate meanwhile? It would be great to open a Draft Pull Request where we could discuss implementation details. I'd suggest moving everything Basti CLI related to packages/basti-cli and developing the CDK package in packages/basti-cdk.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

I started thinking about the interface again. The interface dictates what and how can we use to implement the constructs.

When I previously suggested the interface, I mistakenly thought of it in CLI terms. In IaC realities, other common practices and constraints apply. For example, circular dependencies. It's impossible to pass an RdsInstance to a BastiRdsInstanceAccessSecurityGroup as the security group has to also be referenced in RdsInstance's security groups. (I'm extrapolating my Terraform experience here, but I think it should be valid for CDK as well).

The same constraints don't allow us to create a single construct called BastiInstance, provide it with RdsInstances, and RdsClusters the same way we do in CLI and attach security groups to them inside, implicitly.

I suggest the following slightly changed interface:

  1. BastiInstance - Bastion EC2 instance
    • Props
      1. subnetId
    • Public fields
      1. id
      2. vpcId
      3. subnetId
      4. securityGroupId
  2. BastiAccessSecurityGroup - A security group that is created per target and has to be attached to the target
    • Props
      1. bastiSecurityGroupId - access security group will allow access from this group
      2. targetPort - access security group will allow connecting to this port
    • Public fields
      1. id
      2. bastiInstanceSecurityGroupId
      3. port

We can implement the BastiInstance construct with Basti CLI and keep the BastiAccessSecurityGroup native since it's much more straightforward. If things go well with Basti CLI in BastiInstnace, we can add some internal commands to create access security groups with Basti CLI.

For now, I'm still wary of the new approach since I haven't seen it used before. However, the advantages it provides are worth giving a try in my opinion.

@bobveringa, what about quickly (relatively 🙂) creating a POC first? Have you seen Cloudformation Custom Resources / Resource Providers? They work pretty much like you described - using Lambda function to provision resources.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024

Yeah, I have worked with custom resources and resource providers, the only tricky thing might be getting basti as a lambda layer (or similar). But there are constructs that deal with this.

The same constraints don't allow us to create a single construct called BastiInstance, provide it with RdsInstances, and RdsClusters the same way we do in CLI and attach security groups to them inside, implicitly.

I don't have any Terraform experience, so I don't know how that would work. But as far as CDK is concerned, you are passing 2 things, a RDSInstance/RDSCluster and a subnet to the same construct, that shouldn't create any circular dependencies, correct?

My knowledge of basti's internal workings is fairly limited. I did go through all the code, but that only gets you so far. How would BastiAccessSecurityGroup being native work? Does the CLI not create certain resources?

You added targetPort is this the port for the database, or the port that must be used locally? If it is the port for the database, it seems weird that it needs to be provided by the user, while this is all defined in infrastructure.

The closest thing that to the "Construct wrapper for CLI" I could find is a construct provided by AWS called AwsSdkCall which is a custom resource wrapper around a CLI tool.

In this sprint, there are still some items that I need to complete, but it is going better than I anticipated, hopefully this will free up some time for me to work on this and to get going on the PoC.

It might also be a good idea to set up that call to discuss this, before we start doing a bunch of work that has to be thrown away. You should have received my work email on your LinkedIn.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

It might also be a good idea to set up that call to discuss this

I agree! With this number of variables and aspects, it's time to discuss the solution on a call. I'll sum the discussion up here later for documentation purposes.

from basti.

bobveringa avatar bobveringa commented on August 26, 2024

After doing a lot of prototyping today. There are some things that we need to consider when defining the design. Some things we thought of might not be practical to implement in CDK, or would go against how most constructs work within CDK.

Resource Naming
Basti only creates the resources ones, so it can use a random ID each time the code is executed. This will not work for CDK, as a random ID, each time would mean that resources are constantly deployed and redeployed. For now, I have solved this by generating a hash of the construct ID and using the first 8 chars of the hash.

This also adds the point that resources need to follow the specific naming format for basti to find the resources. CDK (and I imagine that terraform would too) are generated most of the time. I'm not expecting this to cause issues ones the construct is deployed, but it is something to keep in mind.

Basti cleanup
The basti cleanup command also removes resources created within CDK. CDK/CloudFormation does not like this. So at some point there needs to be a solution for this. Maybe CDK resources could have an additional Basti tag like basti:created_by: CDK/CLI

Attaching the security group
There are a significant number of differences between the AWS SDK and what the constructs in CDK. In the basti CLI, you can attach the security group to the target. On CDK, you need to pass the security groups that need to be added when creating an instance of the database. This can be done after the physical resource is created, but there is no command (that works at least) to add a security group later. I've tried 3-4 different methods to try and add the security group later, but none of those worked.

It was possible to modify the security group of the database fairly easily, you can just get the security group of the target and then use allow_from and pass the basti instance security group. However, this is not what the basti connect command expects and from looking at the code, expect it is non-trivial to change this.

Another option that I have not yet tried is to have a custom resource lambda to add and remove the security group. This seems like a bad idea, it would require significant effort to test and validate for little benefit.

I've not yet had the time to fully consider the implementations of this, so there is no design I can propose at this time. Looking at my schedule for the next couple of days, I can spend tomorrow on this issue to try and figure out what are options are and what would work best.

I did have something working, but it wasn't pretty, and it shouldn't be the final design. It does, however, validate that it is possible to have a construct work.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Resource Naming - I think we can ask the user to provide logical IDs for Basti instances and access security groups. This is similar to how other resources in AWS work. For example, when you create an RDS instance, you provide an identifier for it. This will even be useful for the user since it will make it possible to give Basti resources meaningful names instead of random IDs.

Basti cleanup - I like the idea with the tag 👍

Attaching the security group - This is what I tried to explain in the previous comment but I didn't provide the full use-case. I think the usage should look the following way (based on how my company uses Basti in Terraform):

  1. User creates a BastiInstance construct

  2. User creates one BastiAccessSecurityGroup per connection target (RDS instance, for example) and provides the BastiInstance's security group (to allow connections from) and ports to allow connections to:

    const bastiAccessSecurityGroupMyDb = new BastiAccessSecurityGroup(this, 'basti-access-security-group-my-db', {
      bastiSecurityGroupId, // or whole BastiSecurityGroup
      port: 5432 // We might want to accept a port range here
    });
  3. User attaches the BastiAccessSecurityGroup to the connection target. Something like this:

    new rds.DatabaseInstance(this, 'my-db', {
      /*...*/
      securityGroups: [
        /*...*/
        bastiAccessSecurityGroupMyDb // I think this can implement ISecurityGroup
      ]
      /*...*/
    });

from basti.

bobveringa avatar bobveringa commented on August 26, 2024

This is one of the iterations I tried out. It works, but it feels a bit weird as you are not using it. Why am I entering the port manually, the construct that I am defining knows it. Maybe we can open up some options to allow ports to be configured both when defining the AccessSecurityGroup and later.

As for the BastiAccessSecurityGroup you can extend SecurityGroup and then you should be able to pass it to everything that takes security groups.

I'll implement this interface, as my prototyping today has not really resulted in any ideas that add significant benefit. The only idea that has a marginal improvement is the following:

basti = BastiInstance(...)

basti_access_security_group = BastiAccessSecurityGroup(
    self, 'basti-access-security-group
)

db = rds.DatabaseInstance(
    self, 'db',
    ...
    security_groups = [...,  basti_access_security_group ]
    ...
)

basti_access_security_group.add_connect_target(basti, port=db.instance_endpoint.port)

This would be more in line with how CDK generally works. If this deviates slightly from terraform, I'd be ok with that. In theory, this would also allow the security group to be re-used by other basti instances. But I'm not sure how the CLI would handle that.

Within CDK multi ports is automatically supported for security group as you can use the ec2.Port which also allows port ranges.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Sounds good! Regarding the add_connect_target method vs providing a port in the constructor, I'll do some research tomorrow. But I guess, it's not that hard to change anytime later, right?

Let's extend from SecurityGroup and take ec2.Port as an argument 👍

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Also, I started working on the monorepo structure. Will share the branch tomorrow as well

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Hey @bobveringa 👋

I created a branch called feat/basti-cdk. I turned the project into a monorepo and moved everything basti-cli related to the packages/basti folder. I created the packages/basti-cdk folder for you to implement the CDK package in.

I might add some more commits later, improving the overall project structure, but won't touch the basti-cdk folder.

from basti.

BohdanPetryshyn avatar BohdanPetryshyn commented on August 26, 2024

Hi @bobveringa!

I just want to let you know that I'm getting back to Basti CDK polishing after finishing some features I started working on before. I'd expect a proper release these weekends or at the beginning of the next week

from basti.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.