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

VPC Naming and Egress Setting #250

Closed
wants to merge 7 commits into from
Closed

VPC Naming and Egress Setting #250

wants to merge 7 commits into from

Conversation

arei
Copy link

@arei arei commented Mar 16, 2021

Fixes #242

This PR address to issues with defining VPC connectors for a google cloud function:

  • First, VPC Connectors can be named with a url (which is the current pattern) or with just the name of the connector if it is within the same project and region. This is described here: https://cloud.google.com/functions/docs/reference/rest/v1/projects.locations.functions#resource:-cloudfunction
  • Second, this PR adds the ability to specify a VPC Egress setting. This is done using the vpcEgress property in the serverless.yml file. This property can take one of four different values:
    • ALL_TRAFFIC
    • PRIVATE_RANGES_ONLY
    • ALL (A shortcut for ALL_TRAFFIC)
    • PRIVATE (A shortcut for PRIVATE_RANGES_ONLY)

Also added smattering of tests to verify these changes.

Enjoy.

@edgaregonzalez
Copy link

Hello, when we can have this feature available?
Thank you.

@pendexgabo
Copy link

would be useful for us as well. how can we help to get this merged.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Thank you @arei - it looks good in general, I have a few comments, please let me know what do you think

};

const validateVpcConnectorEgressProperty = (funcObject, functionName) => {
if (funcObject.vpcEgress && typeof funcObject.vpcEgress === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This new property should be added to configuration schema in provider/googleProvider.js

Copy link
Contributor

Choose a reason for hiding this comment

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

hey! how are you? sorry, I was trying to make these changes since these would be of great help to me.
I can't find where to add that property in that file. Would you help me?

Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello 👋 The schema for provider and function properties is here:

provider: {
properties: {
credentials: { type: 'string' },
project: { type: 'string' },
region: { $ref: '#/definitions/cloudFunctionRegion' },
runtime: { $ref: '#/definitions/cloudFunctionRuntime' }, // Can be overridden by function configuration
serviceAccountEmail: { type: 'string' }, // Can be overridden by function configuration
memorySize: { $ref: '#/definitions/cloudFunctionMemory' }, // Can be overridden by function configuration
timeout: { type: 'string' }, // Can be overridden by function configuration
environment: { $ref: '#/definitions/cloudFunctionEnvironmentVariables' }, // Can be overridden by function configuration
vpc: { type: 'string' }, // Can be overridden by function configuration
labels: { $ref: '#/definitions/resourceManagerLabels' }, // Can be overridden by function configuration
},
},
function: {
properties: {
handler: { type: 'string' },
runtime: { $ref: '#/definitions/cloudFunctionRuntime' }, // Override provider configuration
serviceAccountEmail: { type: 'string' }, // Override provider configuration
memorySize: { $ref: '#/definitions/cloudFunctionMemory' }, // Override provider configuration
timeout: { type: 'string' }, // Override provider configuration
environment: { $ref: '#/definitions/cloudFunctionEnvironmentVariables' }, // Override provider configuration
vpc: { type: 'string' }, // Override provider configuration
labels: { $ref: '#/definitions/resourceManagerLabels' }, // Override provider configuration
},
},
});

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

if (funcObject.vpcEgress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I might be missing something here - if we're only checking for setting on funcObject, does that mean that setting on provider level will always be ineffective?

if (funcObject.vpcEgress) {
let egress =
_.get(funcObject, 'vpcEgress') || _.get(this, 'serverless.service.provider.vpcEgress');
if (egress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't that check redundant? The whole if will be executed only if funcObject has vpcEgress set, which means that egress will always be set.

const validateVpcConnectorEgressProperty = (funcObject, functionName) => {
if (funcObject.vpcEgress && typeof funcObject.vpcEgress === 'string') {
const egress = funcObject.vpcEgress.toUpperCase();
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

This check can be achieved by proper schema definition in for vpcEgress property

@pgrzesik
Copy link
Contributor

pgrzesik commented Sep 3, 2021

I'm closing this one in favor of: #271

@pgrzesik pgrzesik closed this Sep 3, 2021
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.

Unable to set egress or ingress settings
6 participants