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

feat: added pre-wired CBR rules #686

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: added pre-wired CBR rules #686

wants to merge 8 commits into from

Conversation

Ak-sky
Copy link
Member

@Ak-sky Ak-sky commented Jan 23, 2024

Description

Added support for CBR prewired rules in SLZ.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@Ak-sky
Copy link
Member Author

Ak-sky commented Jan 24, 2024

Not triggering the PR pipeline as it still hitting the RG pending reclamation instances issue.

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@Ak-sky We need more info on what "pre-wired CBR rules" are. Perhaps we need a cloud doc on this @SirSpidey ? And then the variable description for create_prewired_cbr could point to it?

Also I don't think we want to run the fscloud CBR module with the default variables values. Thats going to create CBR rules for services that SLZ doesn't even create (such as ICD). We probably need logic to confirm what services SLZ have been set up to deploy, and only create CBR rules for those.
Perhaps worth a deep dive discussion?

@Ak-sky
Copy link
Member Author

Ak-sky commented Jan 29, 2024

@Ak-sky We need more info on what "pre-wired CBR rules" are. Perhaps we need a cloud doc on this @SirSpidey ? And then the variable description for create_prewired_cbr could point to it?

Also I don't think we want to run the fscloud CBR module with the default variables values. Thats going to create CBR rules for services that SLZ doesn't even create (such as ICD). We probably need logic to confirm what services SLZ have been set up to deploy, and only create CBR rules for those. Perhaps worth a deep dive discussion?

I agree @ocofaigh, we had a discussion on this with @vburckhardt initially that if in case CBR rules for other services are required later in future, the flow will already be opened.

If needed more discussion on this I can put it in the the deep dive session.

@vburckhardt
Copy link
Member

vburckhardt commented Jan 29, 2024

It's correct to create global cbr rules in the account including for services not included in landing zone. This is aligned with the design where we lock down all flows in the account - we just need to make sure this is documented. It is just like the AT routing set up for the account. We can use the doc at https://github.com/terraform-ibm-modules/terraform-ibm-cbr/tree/main/modules/fscloud as a base for the content ending up in the public slz doc.

@vburckhardt
Copy link
Member

vburckhardt commented Jan 29, 2024

perhaps name the variable configure_locked_down_cbr_rules_account_wide (other idea activate_restricted_cbr_config_account_wide)

@vburckhardt
Copy link
Member

Also for the description of the variable - something along those lines: "creates default coarse-grained CBR rules in a given account following a "secure by default" approach - that is: deny all flows by default, except known documented communication in the Financial Services Cloud Reference Architecture"

@SirSpidey
Copy link
Member

SirSpidey commented Jan 29, 2024

Also for the description of the variable - something along those lines: "creates default coarse-grained CBR rules in a given account following a "secure by default" approach - that is: deny all flows by default, except known documented communication in the Financial Services Cloud Reference Architecture"

Here's what I'm taking away from the discussion.

  • Update the description of the variable (and perhaps rename it). See suggestion.
  • Update the readme list item to something aligned with the variable description.

Suggestion for description:

Creates CBR rules in the account that follow a "secure by default" approach. The rules deny all flows by default except those that are documented in the VPC reference architecture for IBM Cloud for Financial Services.

If that ref arch link to too specific to VPC, you could link instead to Reference architectures for IBM Cloud for Financial Services

So it doesn't seem that we need a separate cloud doc topic.

@Ak-sky
Copy link
Member Author

Ak-sky commented Jan 30, 2024

I have updated the variable name and description as per the suggestion. Should I also add the details from this doc https://github.com/terraform-ibm-modules/terraform-ibm-cbr/tree/main/modules/fscloud to the public slz doc?
@vburckhardt @SirSpidey

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

@Ak-sky what kind of impact is this going to have in our SLZ tests? I think we will end up with conflicting CBR rules in our account when tests run right?

@Ak-sky
Copy link
Member Author

Ak-sky commented Jan 31, 2024

@Ak-sky what kind of impact is this going to have in our SLZ tests? I think we will end up with conflicting CBR rules in our account when tests run right?

Yes, with CBR FSCloud module there will be issues when SLZ test will run parallelly.

@ocofaigh
Copy link
Member

OK so we need to figure that out then @Ak-sky - perhaps we need to leverage enterprise sub accounts now so every test spins up a new account. We need to solve the apikey issue first - I know there was a feature being added where apikey could be created in the new account for you, and then you could look it up. But I'm assuming its going to be a service ID apikey, which means we couldn't use to provision roks right now. Perhaps add to next deep dive?

@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 2, 2024

OK so we need to figure that out then @Ak-sky - perhaps we need to leverage enterprise sub accounts now so every test spins up a new account. We need to solve the apikey issue first - I know there was a feature being added where apikey could be created in the new account for you, and then you could look it up. But I'm assuming its going to be a service ID apikey, which means we couldn't use to provision roks right now. Perhaps add to next deep dive?

Yes, I had a discussion on enterprise sub accounts with Todd before and looks like we need this now as we have started consuming CBR FScloud profile and this may cause issues later with other modules consuming CBR FScloud.

I can put this in deep dive and discuss with Todd if there is any POC/investigation I can do to use enterprise sub account.

@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 3, 2024

/run pipeline

@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 3, 2024

Also the patterns in test will run in parallel which will be an issue.

@ocofaigh
Copy link
Member

ocofaigh commented Feb 4, 2024

@Ak-sky Don't run the pipeline until we have a solution as it may impact other tests

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

4 participants