Escaping in AppD UPS

[brall77]

Hello,

In our appd setup we use a PCF user provided for setting the node-name. In the configuration we use dynamic value similar to the tier-name example here:

Tier-$(expr "$VCAP_APPLICATION" : '.*instance_index[": ]*\([[:digit:]]*\).*')

From this example:
https://github.com/cloudfoundry/java-buildpack/blob/main/docs/framework-app_dynamics_agent.md

It no longer seems possible to put values in the UPS that are interpreted by the shell since version 4.39. I think this change prevents this:
7e8f8d8#diff-42cb59bb574adf07b1047cab6e4fb133a32ffd5a5f4ee5f76ad5528ee53372e5R111

Does this seem like it would be the case now? Is there any known workarounds to this issue?

[dmikusa]

Exactly how are you setting this value with your application? Can you provide an example?

[brall77]

Hi @dmikusa-pivotal,

The values are being defined in a user provided service in PCF named 'appdynamics', so the values would be from VCAP_SERVICES. The values have expressions that pull in other variables from the environment similar to the expressions in app_dynamics_agent.yml. This had worked in prior versions of the buildpacks but now on startup any expression in the values are being escaped into string literals.

[dmikusa]

Yes, that's is tricky. We had to start escaping values because some users' services contained information that legitimately required it. I don't think we could roll back the escaping behavior or we're going to break other users.

What you might try is overriding the default configuration value instead. We set dynamic commands by default. If you remove the entries from the service binding and our default, then nothing else needs to be done. If you need to customize with your own dynamic command, you could again remove the entries from the service binding and override with your own custom default command.

The difference with the default commands is that we don't escape them.

You could change the default with these instructions or by packaging your own copy of the buildpack after modifying the config/ file. If you have many applications, the latter would be easier.

[brall77]

Hi @dmikusa-pivotal,

We are aware of the per-manifest option, but we do have many applications using this configuration. Unfortunately my group does not have access to change the platform defaults or run a custom buildpack. We may be forced to update all manifests to get back up and running.

Thanks for confirming. It's unfortunate that existing functionally was broken to fix other users without some workaround. I would advise changing the example in framework-app_dynamics_agent.md which will no longer work.

[dmikusa]

Unfortunately my group does not have access to change the platform defaults or run a custom buildpack

Anyone can run a custom buildpack, it's not possibly to restrict unless you are in an air-gapped environment and there is literally no access to even a local network. With cf push -b <url-to-git> you can reference a fork of the buildpack where you change the default config.

It's not a good long-term solution, but it might give you a way to get back to a working state more quickly than making changes per app.

It's unfortunate that existing functionally was broken to fix other users without some workaround.

Yes, I agree. There are technically still ways to accomplish this, but these won't work for everyone and every use case, as you have pointed out.

I'm not opposed to adding something else, but I don't think there are a lot of options. The shell escaping is not going to allow you direct access to run arbitrary commands in a shell (and arguably it should not as that smells like a security issue).

What specifically are you trying to set in the tier name? Do you have other dynamic configuration options you're trying to set? If so, for which parameters and what values are you trying to use? I need to know the specifics of what you're using. Thanks.

[brall77]

Hi @dmikusa-pivotal,

Thanks for the response. When I said we do not have the option to run a custom buildback, that is more about platform policy vs a hard technical restriction. I can work with our platform team to see what the best option, however unless there is a future fix for this in the standard buidpack I do not think that is a viable option. We would not want to maintain a custom branch long term.

For the specific properties, we are setting 'node-name' very similar to the default but including a datacenter prefix to namespace app instance in AppD:

node-name  = 
DCX:$(echo $VCAP_APPLICATION | jq -r '.application_name'):$(echo $VCAP_APPLICATION | jq -r '.instance_index')

In the example DCX is the datacenter prefix.

For a possible workarounds some easy options would be:

• add an optional 'escape-values= false' config key in the user provided service maybe
• you could have a naming convention for each field like X-raw that is not escaped for example:

node-name-raw = $(....)

[dmikusa]

What about something like this?

• pull the value from the service binding
• use \$\(.*\)|\$\{.*\} regex to check if it has any bash subshell or bash variables (only supports format ${NAME}, not $NAME)
• if matches, then doesn't escape the value
• if no match, then it escapes the value

The regex is simple enough and I think reliable enough. It would then prevent you from needing to modify all of your services. Does that seem reasonable?

I can have this in the next release which should be later this month, after the Java Quarterly updates.

[brall77]

@dmikusa-pivotal,

Thanks that sounds great. Our upgrade cycle is pretty slow so we may have to look at fixing some of our apps through manifest in the mean time. This would help us in the future for sure.

[dmikusa]

@brall77 Hi, I have PR #911 to address this issue ready. If you want to take a look you can run cf push -b https://github.com/cloudfoundry/java-buildpack#gh_issue_904 to give it a try. Let me know if you have any issues with it. I'll probably try to merge it on Monday next week. Thanks!