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

Handle region with care, updates to test suite #1930

Merged
merged 34 commits into from
Sep 11, 2023
Merged

Handle region with care, updates to test suite #1930

merged 34 commits into from
Sep 11, 2023

Conversation

iameskild
Copy link
Member

@iameskild iameskild commented Aug 23, 2023

Reference Issues or PRs

This PR does a few things:

  • ensures that the region value provided (or defaults to a sensible value if not) can be validated against the cloud API
    • with the exception of Azure (for now)
  • validates the kubernetes_version at nebari init
  • updates the kubernetes version to 1.26
  • adds Azure to the integration tests
    • I also replaced the @on_cloud decorator for now because it was parameterizing the session fixture which caused the deployment to attempt to deploy all of the clouds even if the -m <cloud> was not specified. My alternative for now is just to add the following flag --cloud <cloud> when you want to run the integration tests for that particular cloud.

Closes #1824
Closes #1969
Related to #1862

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): kubernetes upgrade

Testing

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

Any other comments?

@pavithraes pavithraes added the status: in review 👀 This PR is currently being reviewed by the team label Aug 28, 2023
@iameskild iameskild requested a review from a team August 28, 2023 16:30
@iameskild
Copy link
Member Author

With this PR, running these integration tests locally work for Azure and AWS. I will work on GCP and making sure these can run in CI in another PR. I will also circle back to Digital Ocean once I can resolve #1965.

@iameskild iameskild added this to the Release 2023.9.1 milestone Sep 4, 2023
@kenafoster
Copy link
Contributor

kenafoster commented Sep 7, 2023

Testing the init with region flags and commercial vs. GovCloud partitions:

Works OK when specified region is in the used credentials' partition

Using GovCloud credentials, trying the default (commercial us-east-1) region:

nebari init aws -p testproj
Defaulting to `us-east-1` region.
Please double-check that the AWS credentials are valid and have the correct permissions. If you're deploying into a non-standard partition (e.g. AWS GovCloud), please ensure the region specified exists in that partition

Similar (expected) message when passing in another commercial region with region flag.

I tested the converse (using commercial credentials and trying GovCloud region) and got equivalent errors.

Render and deploy appear to use the following order of priority when choosing the region. If this is the intended behavior then all looks good to me.

  1. Nebari config file
  2. AWS_DEFAULT_REGION env var

Note that the error if neither of the above are available is quite right "Please specify region in the nebari-config.yaml or if initializing the nebari-config, set the region via the --region flag or via the AWS_DEFAULT_REGION environment
variable" ...technically, even if you're not initializing (rendering or deploying), you don't need to have region in nebari-config.yaml as long as AWS_DEFAULT_REGION is set

src/_nebari/render.py Outdated Show resolved Hide resolved
@iameskild
Copy link
Member Author

Thanks for the feedback @kenafoster! I went ahead and made region and kubernetes_version required values for all cloud providers. This means that they need to be set in the nebari-config.yaml and after which, they become the source of truth for any future commands.

As for init, both region and kubernetes_version are set according to the following order:

  1. --region flag in the init
  2. Env vars (only AWS currently)
  3. default to value set in constants.py

This feels like the right approach but it required updating some of the tests to reflect this new standard.

@pavithraes pavithraes added the project: JATIC Work item needed for the JATIC project label Sep 8, 2023
@kenafoster
Copy link
Contributor

Test results look good to me. One minor concern with the content of the error in the scenario where you try 'nebari deploy' without aws.region specified in config.

Note for all of the tests below, my enviornment has a valid key set for AWS GovCloud partition. I have also tested the converse (using a Commerical partition credential and trying to deploy into GovCloud regions) and gotten similar, expected results.

init

  • No AWS_DEFAULT_REGION set and no --region config
    • OK. Nebari does not init (message "Please double-check that the AWS credentials are valid and have the correct permissions. If you're deploying into a non-standard partition (e.g. AWS GovCloud), please ensure the region specified exists in that partition.")
  • AWS_DEFAULT_REGION=us-gov-east-1 and no --region config
    • OK. Nebari inits in us-gov-east-1 (message "Falling back to the region found in the AWS_DEFAULT_REGION environment variable: us-gov-east-1")
  • AWS_DEFAULT_REGION=us-gov-east-1 and --region us-gov-west-1
    • OK. Nebari inits in us-gov-west-1 (--region flag overrides AWS_DEFAULT_REGION and the Nebari default)
  • AWS_DEFAULT_REGION=us-gov-east-1 and --region us-gov-east-999 (doesn't exist)

render/deploy

  • No AWS region set in nebari-config.yaml
    • PARTIAL OK. Behavior is good - it fails regardless of whether AWS_DEFAULT_REGION is configured in env. However, message includes full traceback plus (ValidationError: 5 validation errors for ConfigSchema amazon_web_services -> region field required (type=value_error.missing))
    • AWS GovCloud region set in nebari-config.yaml
      • OK (renders/deploys)
    • AWS Commercial region set in nebari-config.yaml
      • OK. Does not deploy (message: Please double-check that the AWS credentials are valid and have the correct permissions. If you're deploying into a non-standard partition (e.g. AWS GovCloud), please ensure the region specified exists in that partition.)

Also confirmed render, deploy do not take --region option

@iameskild
Copy link
Member Author

Thanks a lot for the detailed testing @kenafoster! Improving the traceback and error messages is definitely worth a look. Thanks to @sblair-metrostar, we're tracking this here.

@pavithraes @kcpevey when you have time, could you please review and approve this PR? Let me know if you have any quesitons or concerns. Thank you :)

Copy link
Member

@pavithraes pavithraes left a comment

Choose a reason for hiding this comment

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

@iameskild Thanks for working on this, I'll approve but note the small merge conflict that needs resolution. :)

@pavithraes pavithraes added status: approved 💪🏾 This PR has been reviewed and approved for merge and removed status: in review 👀 This PR is currently being reviewed by the team labels Sep 11, 2023
@iameskild iameskild merged commit 107e413 into develop Sep 11, 2023
@iameskild iameskild deleted the 20230822 branch September 11, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dependencies 📦 All things dependencies area: testing ✅ Testing project: JATIC Work item needed for the JATIC project provider: Azure status: approved 💪🏾 This PR has been reviewed and approved for merge
Projects
Status: Done 💪🏾
4 participants