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.

Sign up for GitHub

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

Jump to bottom

Add amazon_web_services configuration option to specify EKS cluster api server endpoint access setting #2618

Merged
merged 14 commits into from
Sep 9, 2024
Merged

Add amazon_web_services configuration option to specify EKS cluster api server endpoint access setting #2618

merged 14 commits into from
Sep 9, 2024

Conversation

joneszc
Copy link
Contributor

@joneszc joneszc commented Aug 9, 2024

Reference Issues or PRs

Fixes #2586
Fixes #2587

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] 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

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

Any other comments?

This PR corrects original PR #2587 by enabling cluster endpoint access configuration by means of a single variable:
amazon_web_services.eks_endpoint_access

This was referenced Aug 9, 2024
Closed
Closed
Open
dcmcand
dcmcand previously requested changes Aug 26, 2024
View reviewed changes
src/_nebari/stages/infrastructure/__init__.py Outdated
@@ -146,6 +146,7 @@ class AWSInputVars(schema.Base):
existing_subnet_ids: Optional[List[str]] = None
region: str
kubernetes_version: str
eks_endpoint_access: str = "public"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having an Enum here (https://docs.pydantic.dev/2.2/usage/types/enums/) rather than a string would make sense. That way you don't need to write your own validator.

src/_nebari/stages/infrastructure/__init__.py Outdated
@@ -465,6 +466,7 @@ class AmazonWebServicesProvider(schema.Base):
kubernetes_version: str
availability_zones: Optional[List[str]]
node_groups: Dict[str, AWSNodeGroup] = DEFAULT_AWS_NODE_GROUPS
eks_endpoint_access: str = "public"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having an Enum here (https://docs.pydantic.dev/2.2/usage/types/enums/) rather than a string would make sense. That way you don't need to write your own validator.

src/_nebari/stages/infrastructure/__init__.py Outdated
@@ -520,6 +522,18 @@ def _check_input(cls, data: Any) -> Any:
raise ValueError(
f"Amazon Web Services instance {node_group.instance} not one of available instance types={available_instances}"
)

# check if eks cluster endpoint access config is valid
Copy link
Contributor

Choose a reason for hiding this comment

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

This validator could be removed if you switch to an enum with a default value

Copy link
Contributor Author

@joneszc joneszc Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dcmcand
Removed validator per your suggestion; using typing.Literal, in lieu of pydantic enums, for congruency with pending PR#2668's invocation of Literal

src/_nebari/stages/infrastructure/template/aws/modules/kubernetes/main.tf Fixed Show resolved Hide resolved
@joneszc joneszc self-assigned this Sep 4, 2024
@joneszc joneszc requested a review from dcmcand September 4, 2024 17:35
@dcmcand dcmcand added this to the Next Release milestone Sep 5, 2024
@viniciusdc
Copy link
Contributor

Hi @joneszc things that are important to test on general infrastructure PRs:

  • Deploying without specifying any settings, it should succeed and should be backward compatible with the current state;
  • I'm just deploying with any available choices to prove the configuration applies (you linked a print above; I assume that works). However, it is interesting to do those tests with a fresh AWS deployment to make sure there are no leftover modifications that could interfere with the resources presented in this PR;

This is one specific to this changes:

  • Garantee that the exposed endpoint can be reached and worked with

@joneszc
Copy link
Contributor Author

joneszc commented Sep 9, 2024

@viniciusdc

Testing outputs:

amazon_web_services:
  eks_endpoint_access: private
image 
amazon_web_services:
  eks_endpoint_access: public_and_private
image 
amazon_web_services:
  eks_endpoint_access: public
image

@viniciusdc
Copy link
Contributor

Also asserted on a separate deployment 🚀

viniciusdc
viniciusdc approved these changes Sep 9, 2024
View reviewed changes
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.

LGTM, no compatibility issues as well. Thanks @joneszc for the contribution!!

@viniciusdc viniciusdc dismissed dcmcand’s stale review September 9, 2024 19:22

tested with aws deployments for each configuration

@viniciusdc viniciusdc merged commit aed2b92 into nebari-dev:develop Sep 9, 2024
11 checks passed
@joneszc
Copy link
Contributor Author

joneszc commented Sep 9, 2024

LGTM, no compatibility issues as well. Thanks @joneszc for the contribution!!

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viniciusdc viniciusdc viniciusdc approved these changes

@dcmcand dcmcand Awaiting requested review from dcmcand

Assignees

@joneszc joneszc

Labels
None yet
Projects
🪴 Nebari Project Management
Status: Done 💪🏾
Milestone
2024.9.1 Release
3 participants
@joneszc @viniciusdc @dcmcand