Conditional at Launch #38

nebhale · 2020-11-13T19:17:43Z

Previously, a binding was required at build time in order to contribute the layers for the Stackdriver components. We've started to think that requiring a binding during build is too onerous a restriction and instead environment variables should be used to indicate that a dependency should be contributed. The knock-on effect of this change is that we have to move a lot more of the conditional logic out to launch time including ensuring that if the binding doesn't exist nothing is contributed. This change makes those updates to the buildpack's behavior.

In addition, this change also includes an update that enables the buildpack to determine if the application is running in GCP by detecting the metadata server. In this case, no binding is required at launch time either as the credentials typically provided by the binding are instead provided by the metadata server.

Finally, this includes a fix where the provided build plan wasn't anywhere near exhaustive for combinations that a user need when using the buildpack.

[#2]

helper/in_gcp.go

helper/java_debugger.go

stackdriver/build_test.go

stackdriver/detect.go

README.md

helper/nodejs_profiler.go

helper/java_debugger.go

helper/credentials.go

ekcasey · 2020-11-16T14:49:29Z

I think we may need to explicitly exclude add the metadata server host to no_proxy if we want our InGCP function to play nice with a user-configured proxy at runtime. paketo-buildpacks/azure-application-insights#32 (review)

Previously, a binding was required at build time in order to contribute the layers for the Stackdriver components. We've started to think that requiring a binding during build is too onerous a restriction and instead environment variables should be used to indicate that a dependency should be contributed. The knock-on effect of this change is that we have to move a lot more of the conditional logic out to launch time including ensuring that if the binding doesn't exist _nothing_ is contributed. This change makes those updates to the buildpack's behavior. In addition, this change also includes an update that enables the buildpack to determine if the application is running in GCP by detecting the metadata server. In this case, no binding is required at launch time either as the credentials typically provided by the binding are instead provided by the metadata server. Finally, this includes a fix where the provided build plan wasn't anywhere near exhaustive for combinations that a user need when using the buildpack. Signed-off-by: Ben Hale <[email protected]>

ekcasey

Love the direction this is going! But I still have a couple questions.

ekcasey · 2020-11-18T18:43:47Z

README.md

+| `$BP_GOOGLE_CLOUD_DEBUGGER_ENABLED` | Whether to add Google Cloud Debugger during build
+| `$BP_GOOGLE_CLOUD_PROFILER_ENABLED` | Whether to add Google Cloud Profiler during build 
+| `$BPL_GOOGLE_CLOUD_MODULE` | Configure the name of the application (required)
+| `$BPL_GOOGLE_CLOUD_PROJECT_ID` | Configure the project id for the application (required if running outside of Google Cloud)


I know I made the opposite comment earlier but you mentioned that the project ID is now included in the service account key and therefore is not actually required in all situations. If we wanted to be clever (maybe too clever) we could pull the project ID out of the key name or project_id fields https://cloud.google.com/iam/docs/creating-managing-service-account-keys.

ekcasey · 2020-11-18T18:48:41Z

README.md

+### Type: `GoogleCloud`
+|Key                      | Value            | Description
+|-------------------------|------------------|------------
+|`ApplicationCredentials` | `<JSON Payload>` | Google Cloud Application Credentials in JSON form


Suggested change

|`ApplicationCredentials` | `<JSON Payload>` | Google Cloud Application Credentials in JSON form

|`ApplicationCredentials` | `<JSON Payload>` | Google Cloud [Service Account Key][s] (JSON)

[s]: https://cloud.google.com/iam/docs/creating-managing-service-account-keys

I think using the more specific terminology and a link will help folks understand the workflow more quickly.

ekcasey · 2020-11-18T21:33:57Z

internal/common/common.go

+	Credentials    = "google-cloud-credentials"
+	DebuggerJava   = "google-cloud-debugger-java"
+	DebuggerNodeJS = "google-cloud-debugger-nodejs"
+	ProfilerJava   = "google-cloud-profiler-java"
+	ProfilerNodeJS = "google-cloud-profiler-nodejs"


All of the plan entry constants are only used in the google package. Why not move them there?

ekcasey · 2020-11-18T21:35:47Z

internal/common/common.go

+
+const (
+	Credentials    = "google-cloud-credentials"
+	DebuggerJava   = "google-cloud-debugger-java"


I like provide a common prefix for a set of related constants. What do you think about prefixing all plan entry constants with PlanEntry (e.g. PlanEntryJavaDebugger) so it is clear what the constant is for.

Other prefix suggestions:

Env* (e.g. EnvModule)

Source* (e.g. SourceBinding)

ekcasey · 2020-11-18T21:49:20Z

cmd/helper/main.go

+		)
+
+		if hasMetadataServer() {
+			c = common.MetadataServer


Wouldn't we want the binding to take precedence over the metadata server?

ekcasey · 2020-11-19T22:04:19Z

detect.go

+					Provides: []libcnb.BuildPlanProvide{
+						{Name: common.Credentials},
+						{Name: common.DebuggerJava},
+						{Name: common.ProfilerJava},
+						{Name: common.DebuggerNodeJS},
+						{Name: common.ProfilerNodeJS},
+					},
+					Requires: []libcnb.BuildPlanRequire{
+						{Name: common.Credentials},
+						{Name: common.DebuggerJava},
+						{Name: common.ProfilerJava},
+						{Name: "jvm-application"},
+						{Name: common.DebuggerNodeJS},
+						{Name: common.ProfilerNodeJS},
+						{Name: "node", Metadata: map[string]interface{}{"build": true}},
+					},


Looking at this more carefully, won't we always select this plan if both jvm-application and node are provided by other buildpacks, rendering the subsequent two plans in the debugger && profiler conditional moot?

ekcasey · 2020-11-19T22:10:29Z

detect.go

+						{Name: "jvm-application"},
+						{Name: common.DebuggerNodeJS},
+						{Name: common.ProfilerNodeJS},
+						{Name: "node", Metadata: map[string]interface{}{"build": true}},


This is interesting.... if node is provided by another buildpack but only at build time (i.e node is necessary at build time but the resulting app is not a NodeJS application we probably don't want to install the NodeJS profiler and debugger. However, the https://github.com/paketo-buildpacks/npm-start and https://github.com/paketo-buildpacks/yarn-start buildpacks don't seem to provide anything we can key off of that would be analogous to jvm-application.

ekcasey · 2020-11-19T22:23:39Z

internal/nodejs/module.go

+)
+
+func IsModuleRequired(module string, content []byte) (bool, error) {
+	p := fmt.Sprintf(`require\(['"]%s['"]\)`, module)


I am not sure what all the allowed characters are in a module name; is there any danger that special characters in the name could prevent the regex from compiling? Some alternatives would be:

add a capture group and then compare the matched module names to module.

use two naive strings.Contains, one with "s and one with 's and return if either is true

ekcasey · 2020-11-19T22:28:34Z

internal/nodejs/module.go

+	t, err := template.New("require-module").
+		Parse(`require('{{.Module}}').start({
+  serviceContext: {
+    service: '{{.Service}}',
+    version: '{{.Version}}',
+  },
+});
+`)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse template\n%w", err)
+	}


Suggested change

t, err := template.New("require-module").

Parse(`require('{{.Module}}').start({

serviceContext: {

service: '{{.Service}}',

version: '{{.Version}}',

},

});

`)

if err != nil {

return nil, fmt.Errorf("unable to parse template\n%w", err)

}

t := template.Must(template.New("require-module").

Parse(`require('{{.Module}}').start({

serviceContext: {

service: '{{.Service}}',

version: '{{.Version}}',

},

});

`))

Given the template is constant it is safe to use a Must here.

ekcasey · 2020-11-19T22:29:49Z

internal/nodejs/module.go

+	t, err := template.New("require-module-external").
+		Parse(`require('{{.Module}}').start({
+  projectId: '{{.ProjectId}}',
+  serviceContext: {
+    service: '{{.Service}}',
+    version: '{{.Version}}',
+  },
+});
+`)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse template\n%w", err)
+	}


Suggested change

t, err := template.New("require-module-external").

Parse(`require('{{.Module}}').start({

projectId: '{{.ProjectId}}',

serviceContext: {

service: '{{.Service}}',

version: '{{.Version}}',

},

});

`)

if err != nil {

return nil, fmt.Errorf("unable to parse template\n%w", err)

}

t := template.Must(template.New("require-module-external").

Parse(`require('{{.Module}}').start({

projectId: '{{.ProjectId}}',

serviceContext: {

service: '{{.Service}}',

version: '{{.Version}}',

},

});

`))

nebhale added semver:major A change requiring a major version bump type:enhancement A general enhancement labels Nov 13, 2020

nebhale requested a review from ekcasey November 13, 2020 19:17

nebhale force-pushed the conditional-at-launch branch from a68009c to 9a3a727 Compare November 13, 2020 20:19

ekcasey linked an issue Nov 13, 2020 that may be closed by this pull request

Ability to contribute the layer and configure the startup even w/o the credentials binding #2

Open

nebhale force-pushed the conditional-at-launch branch from 9a3a727 to 33100e5 Compare November 13, 2020 21:38

ekcasey suggested changes Nov 14, 2020

View reviewed changes

nebhale requested a review from ekcasey November 17, 2020 21:53

nebhale force-pushed the conditional-at-launch branch 6 times, most recently from d1e647b to 1c37f23 Compare November 18, 2020 00:52

nebhale force-pushed the conditional-at-launch branch from 1c37f23 to 300f282 Compare November 18, 2020 01:05

ekcasey reviewed Nov 19, 2020

View reviewed changes

dmikusa approved these changes Jul 13, 2021

View reviewed changes

dmikusa self-requested a review July 13, 2021 12:35

dmikusa marked this pull request as ready for review December 17, 2021 14:13

dmikusa requested a review from a team December 17, 2021 14:13

dmikusa marked this pull request as draft December 17, 2021 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional at Launch #38

Conditional at Launch #38

nebhale commented Nov 13, 2020

ekcasey commented Nov 16, 2020

ekcasey left a comment

ekcasey Nov 18, 2020

ekcasey Nov 18, 2020

ekcasey Nov 18, 2020

ekcasey Nov 18, 2020

ekcasey Nov 18, 2020

ekcasey Nov 19, 2020

ekcasey Nov 19, 2020

ekcasey Nov 19, 2020

ekcasey Nov 19, 2020

ekcasey Nov 19, 2020

-|`ApplicationCredentials` | `<JSON Payload>` | Google Cloud Application Credentials in JSON form
+|`ApplicationCredentials` | `<JSON Payload>` | Google Cloud [Service Account Key][s] (JSON)
+[s]: https://cloud.google.com/iam/docs/creating-managing-service-account-keys

Conditional at Launch #38

Are you sure you want to change the base?

Conditional at Launch #38

Conversation

nebhale commented Nov 13, 2020

ekcasey commented Nov 16, 2020

ekcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment