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

add authorized ip range variable for azure #2880

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

Conversation

dcmcand
Copy link
Contributor

@dcmcand dcmcand commented Dec 11, 2024

Reference Issues or PRs

Closes #2784

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

Ensure you can deploy to azure with an existing azure config.

Then

Add a authorized_ip_ranges line to the azure provider of your nebari-config.yaml. Deploy to azure and ensure that access to api server is limited to that ip range.

example config block

azure:
  authorized_ip_ranges: ["80.145.170.197/32"]  # make this your own ip address
  kubernetes_version: 1.29.2
  region: eastus
  storage_account_postfix: esrr
  node_groups:
    general:
      instance: Standard_D8_v3
      min_nodes: 1
      max_nodes: 1
    user:
      instance: Standard_D4_v3
      min_nodes: 0
      max_nodes: 5
    worker:
      instance: Standard_D4_v3
      min_nodes: 0
      max_nodes: 5

Any other comments?

@dcmcand dcmcand added needs: review 👀 This PR is complete and ready for reviewing provider: Azure area: security 🔐 impact: critical Highest level of impact labels Dec 11, 2024
@dcmcand dcmcand added this to the 2024.12.2 release milestone Dec 11, 2024
@dcmcand dcmcand added the area: feature parity inconsistencies in features between cloud providers label Dec 11, 2024
@dcmcand dcmcand changed the title adad authorized ip range variable for azure add authorized ip range variable for azure Dec 11, 2024
@dcmcand
Copy link
Contributor Author

dcmcand commented Dec 11, 2024

The failing tests appear unrelated to these changes.

@viniciusdc
Copy link
Contributor

Hi @dcmcand, I am not sure if this will be related at all, but last time we changed something related to IP ranges (specifically for AWS CIDRs blocks) we ended up having issues with Dask (here's some of the discussion points #828)

So I would just suggest to do a full Azure deploy with these settings and double check if dask can spin correctly.

@dcmcand
Copy link
Contributor Author

dcmcand commented Dec 12, 2024

Hi @dcmcand, I am not sure if this will be related at all, but last time we changed something related to IP ranges (specifically for AWS CIDRs blocks) we ended up having issues with Dask (here's some of the discussion points #828)

So I would just suggest to do a full Azure deploy with these settings and double check if dask can spin correctly.

That looks unrelated, but I will test it.

@viniciusdc viniciusdc removed this from the 2024.12.2 release milestone Dec 12, 2024
@dcmcand
Copy link
Contributor Author

dcmcand commented Dec 16, 2024

@viniciusdc tested and dask works fine. Now that 2024.12.1 is out, could I get a review here?

Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

It looks good to me; I only have a single question regarding the aks cluster new block api_server_access_profile; did you test this modification on a previously deployed cluster? And is can this change be safely rolled back?

Comment on lines +7 to +9
api_server_access_profile {
authorized_ip_ranges = var.authorized_ip_ranges
}
Copy link
Contributor

@viniciusdc viniciusdc Dec 17, 2024

Choose a reason for hiding this comment

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

Including this field leads to a resource update. Does it affect the running nodes at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be I can test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All it does is update and attribute, no replacement needed. It also runs on the cluster, not on the node.

@dcmcand dcmcand requested a review from viniciusdc December 20, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: feature parity inconsistencies in features between cloud providers area: security 🔐 impact: critical Highest level of impact needs: review 👀 This PR is complete and ready for reviewing provider: Azure
Projects
Status: New 🚦
Development

Successfully merging this pull request may close these issues.

Fix code scanning alert - Ensure AKS has an API Server Authorized IP Ranges enabled
2 participants