Skip to content

Conversation

@Sharu95
Copy link

@Sharu95 Sharu95 commented Feb 20, 2025

Proposed Changes

This PR adds support for parsing the config file for the basti init command.

Related Issues/PRs

Checklist

  • I cleaned up my code.
  • All the tests and checks passed (npm run test).
  • I have added necessary documentation and/or updated existing documentation.
  • I have added or modified tests to cover the changes.

@BohdanPetryshyn
Copy link
Collaborator

Hi @Sharu95, thank you for your interest in contributing to Basti!

I'll try to look at the changes this weekend. Meanwhile, could you please explain the motivation for the changes and the example of the new config?

@Sharu95
Copy link
Author

Sharu95 commented Feb 21, 2025

Hi @BohdanPetryshyn, happy to contribute!

Still just a draft PR but I wanted to put it out there so its easier to iterate on it with feedback. Currently not perfectly parsing from config, but the setup works. Tags are passed from CLI options as well.

My main motivation was to be able to run basti init --target=<some-target> similar to how it is possible to run basti connect --connection. It makes initialization way easier and the config can just be set once in the config file. I think it is a nice abstraction on top of the CLI command ( and the interactive setup)! 👍🏾 An example config file for reference;

connections:
  basti-production:
    target: my-target
    localPort: 8080

targets:
  my-target:
    rdsCluster: basti-production
    awsProfile: AdministratorAccess
    awsRegion: us-east-1
    bastionSubnet: subnet

If the target is part of the connection setup, it should still be parsed as a connection target, but this setup is more or less to adhere to the init command as well, with the config file 🙌🏾

Another motivation I've had is that we manage some of our RDS resources in terraform. The security group used by basti/bastion instance is often deleted during terraform deployments/applies, because its not tracked in terraform, so we end up running basti init more often than ideal if we use terraform a lot

@BohdanPetryshyn
Copy link
Collaborator

Hello @Sharu95! I didn’t include initialization from config because ideally, basti init should be a one-time task.

In the scenario you described, you can stop Terraform from deleting the security group created by Basti by specifically listing it in the Terraform configuration:

 vpc_security_group_ids = [<your-existing-security-groups>, <basti-created-security-group-id>]

However, I believe that config-driven automatic Basti initialization could be beneficial in other situations, so your contribution remains valuable!

@Sharu95
Copy link
Author

Sharu95 commented Feb 27, 2025

Hey again @BohdanPetryshyn!

That is probably one way to solve it, but I was under the impression that Basti creates new security groups, so this changes
dynamically and would be a bit of a hassle in Terraform. I personally think having the config driven approach might be beneficial all over, but let me know regardless how you want to move forward with this PR.

I can continue iterating on it if you have plans making a release of this anytime soon. If not, I have some other prioritizations I'd have to work with before I can come back to this 🙌🏾

@BohdanPetryshyn
Copy link
Collaborator

The basti connect command was designed to not create any resources so the security groups should remain stable. The solution I suggested worked well in my previous organization and will likely work in your Terraform use case as well!

That said, I'm open to collaborating and releasing the feature at your pace!

@Sharu95
Copy link
Author

Sharu95 commented Mar 4, 2025

@BohdanPetryshyn, ok nice, I will test the terraform solution as well and see how it works! 🙌🏾 Then I'll work a bit on this PR on the side to make the contribution 👍🏾 Thanks for input!

@BohdanPetryshyn
Copy link
Collaborator

@Sharu95 Looking forward to your contribution! 🚀

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.

2 participants