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(AwsVllmComponent): Add AWS Cognito as auth service #32

Merged
merged 14 commits into from
Oct 10, 2024

Conversation

bramelfrink
Copy link
Contributor

@bramelfrink bramelfrink commented Oct 4, 2024

Addresses #26

The assumption that I'm making in this PR is that: A Cognito user pool is created elsewhere; so it is not the responsibility of the AwsVllmComponent to do so.

To do:

  • test this with a real application / user pool.

@bramelfrink bramelfrink self-assigned this Oct 4, 2024
@kkiani
Copy link
Contributor

kkiani commented Oct 4, 2024

Thanks, @bramelfrink, for picking this up. One thing I forgot to mention while writing the issue—and was about to update just before receiving your PR—is that the APIGateway is supposed to provide an interface fully compatible with the OpenAI Chat Completion API Model. This means the end user should be able to make a request using a standard API key. I am not sure if AWS Cognito can provide such functionality. However, a user authentication feature might be a nice addition if we eventually decide to build a Chat UI on top of this, which is not currently on the roadmap.

@kkiani
Copy link
Contributor

kkiani commented Oct 4, 2024

The APIGateway does have a functionality for creating API Keys but one thing that is important is to be able to create API Keys after api deployment without a need for pulumi up.

@bramelfrink
Copy link
Contributor Author

bramelfrink commented Oct 4, 2024

@kkiani The creation dynamic creation of API keys should not be part of this application IMO. What you could do here is define (configurable) usage plans, as a key needs to be linked to a usage plan.

The main question is: do we create the usage plans here, or should that go hand in hand with the API key creation? And we delegate that to whomever is going to operate the app.

My proposal would be to create the usage plan(s) here, as part of the application, as they shouldn't change that often.

@kkiani
Copy link
Contributor

kkiani commented Oct 4, 2024

@bramelfrink Usage plans are good addition. I totally agree with having usage plans. I think we can provide 3-4 predefined user plans to make life easier. If an end user needs more customize one they can create it then.

With regards to API Keys, The Component should provide at least one default API Key (with largest usage plan?) for the code owner in case they wants to interact with the model. The default could be shared on ssm parameter or bettter, aws secret manager so later the controller class can have access to it.

I did take a look at AWS Docs and noticed that it is not possible to create API Key after deployment. So, dynamic creation of API Keys after deployment is not possible anyway. However, the component should provide such interface for registering a list of generated API keys. Something like below:

AwsVllmComponent(
    name="my-vllm",
    args=AwsVllmComponentArgs(
        region="eu-west-1",
        public_internet_access=True,
        api_keys=[
            APIKey(id_="my-api-key", value="api-key-value", usage_plan_tier=APIKey.developer_tier_usage_plan()),
        ]
    ),
)

@bramelfrink bramelfrink marked this pull request as ready for review October 8, 2024 11:50
Comment on lines 346 to 364
if self.args.public_internet_access:
return aws.apigateway.Method(
resource_name=f"{self._name}-api-method",
opts=ResourceOptions(parent=self),
rest_api=self.api.id,
resource_id=self.api_resource_completions.id,
http_method="POST",
authorization="NONE",
)
else:
return aws.apigateway.Method(
resource_name=f"{self._name}-api-method",
opts=ResourceOptions(parent=self),
rest_api=self.api.id,
resource_id=self.api_resource_completions.id,
http_method="POST",
authorization="NONE",
api_key_required=True,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the if condition just by passing the public_internet_access to the api_key_required argument:

Suggested change
if self.args.public_internet_access:
return aws.apigateway.Method(
resource_name=f"{self._name}-api-method",
opts=ResourceOptions(parent=self),
rest_api=self.api.id,
resource_id=self.api_resource_completions.id,
http_method="POST",
authorization="NONE",
)
else:
return aws.apigateway.Method(
resource_name=f"{self._name}-api-method",
opts=ResourceOptions(parent=self),
rest_api=self.api.id,
resource_id=self.api_resource_completions.id,
http_method="POST",
authorization="NONE",
api_key_required=True,
)
return aws.apigateway.Method(
resource_name=f"{self._name}-api-method",
opts=ResourceOptions(parent=self),
rest_api=self.api.id,
resource_id=self.api_resource_completions.id,
http_method="POST",
authorization="NONE",
api_key_required=self.args.public_internet_access,
)

_ = self.api_resource_completions

# Only create API key if public internet access is set to False
if not self.args.public_internet_access:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you are changing the behavior to always create the API Gateway which I do agree with but, change the arg name to something meaningful and related like this:

Suggested change
if not self.args.public_internet_access:
if not self.args.is_api_key_required:

Comment on lines 430 to 452
def default_usage_plan(self) -> aws.apigateway.UsagePlan:
"""
Return a default usage plan for the API Gateway, that does not limit the usage.

Raises
------
AttributeError
When public_internet_access is True.
"""
if self.args.public_internet_access:
raise AttributeError("`default_usage_plan` is only available when public_internet_access is False")

return aws.apigateway.UsagePlan(
resource_name=f"{self._name}-api-usage-plan",
opts=ResourceOptions(parent=self),
api_stages=[
aws.apigateway.UsagePlanApiStageArgs(
api_id=self.api.id,
# NOTE: How do we want to deal with API stages vs. AWS environments?
stage=self.args.api_env_name,
)
],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a two or three other default usage plan?

)
],
throttle_settings=aws.apigateway.UsagePlanThrottleSettingsArgs(
rate_limit=500
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkiani I'm only changing the rate limit at the moment.

It's also possible to define quotas per day/week/month, but I think that's only interesting when you start pricing API's.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bramelfrink, No I think rate_limit is enough.

@bramelfrink bramelfrink merged commit 3b9dce4 into main Oct 10, 2024
2 checks passed
@bramelfrink bramelfrink deleted the feat/cognito-auth-service branch October 10, 2024 07:04
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