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

Adding support for egress and ingress settings #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

troykelly
Copy link

@troykelly troykelly commented Nov 20, 2020

Resolves #242

functions:
  score:
    handler: score
    vpc: "projects/${self:provider.project}/locations/${self:provider.region}/connectors/my-vpc-name"
    egress: ALL_TRAFFIC
    ingress: ALLOW_ALL
    events:
      - http: score

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@troykelly Great thanks for that improvement. Please see my comments, also please ensure that CI passes without errors

@@ -62,6 +62,18 @@ module.exports = {
});
}

if (funcObject.egress) {
_.assign(funcTemplate.properties, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use Object.assign

Copy link
Author

Choose a reason for hiding this comment

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

Just following existing established formatting.
I will come back to these change requests in a few weeks, we will just have to work with our fork for now.

@@ -62,6 +62,18 @@ module.exports = {
});
}

if (funcObject.egress) {
_.assign(funcTemplate.properties, {
vpcConnectorEgressSettings: _.get(funcObject, 'egress') || _.get(this, 'serverless.service.provider.egress'),
Copy link
Contributor

Choose a reason for hiding this comment

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

_.get(funcObject, 'egress') is guaranteed to be truthy at this logic point

Comment on lines +71 to +73
if (funcObject.ingress) {
_.assign(funcTemplate.properties, {
ingressSettings: _.get(funcObject, 'ingress') || _.get(this, 'serverless.service.provider.ingress'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

* @param {*} functionName
*/
const validateVpcEgressProperty = (funcObject, functionName) => {
if (funcObject.egress && typeof funcObject.egress === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do just typeof check (other is superfluous)

*/
const validateVpcEgressProperty = (funcObject, functionName) => {
if (funcObject.egress && typeof funcObject.egress === 'string') {
const validTypes = ['VPC_CONNECTOR_EGRESS_SETTINGS_UNSPECIFIED', 'PRIVATE_RANGES_ONLY', 'ALL_TRAFFIC'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's define it as Set instance and outside of function body

Comment on lines +199 to +200
if (funcObject.ingress && typeof funcObject.ingress === 'string') {
const validTypes = ['INGRESS_SETTINGS_UNSPECIFIED', 'ALLOW_ALL', 'ALLOW_INTERNAL_ONLY', 'ALLOW_INTERNAL_AND_GCLB'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@ivanguimam
Copy link

Hello, when will this be merged? I need this feature

@medikoo
Copy link
Contributor

medikoo commented Mar 2, 2021

Hello, when will this be merged? I need this feature

@ivanguimam can you help us in finalizing this PR?

@ivanguimam
Copy link

Hello, when will this be merged? I need this feature

@ivanguimam can you help us in finalizing this PR?

How can I make these corrections being that PR belongs to someone else?

@medikoo
Copy link
Contributor

medikoo commented Mar 2, 2021

How can I make these corrections being that PR belongs to someone else?

You can fork the repository, do updates, and propose another PR with updates

@ivanguimam
Copy link

How can I make these corrections being that PR belongs to someone else?

You can fork the repository, do updates, and propose another PR with updates

I'll try to do that by the weekend

@medikoo
Copy link
Contributor

medikoo commented Mar 3, 2021

I'll try to do that by the weekend

@ivanguimam that'll be great. Thank you!

@Ganitzsh
Copy link

@ivanguimam awesome let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to set egress or ingress settings
4 participants