Adding a new flag to the deploy command and the related new functionality in order to support the collecting of secrets and sending them to the backend#252
Conversation
commands/deploy_command.go
Outdated
|
|
||
| Deploy a multi-target app archive referenced by a remote URL | ||
| <write MTA archive URL to STDOUT> | cf deploy [-e EXT_DESCRIPTOR[,...]] [-t TIMEOUT] [--version-rule VERSION_RULE] [-u MTA_CONTROLLER_URL] [--retries RETRIES] [--no-start] [--namespace NAMESPACE] [--apply-namespace-app-names true/false] [--apply-namespace-service-names true/false] [--apply-namespace-app-routes true/false] [--apply-namespace-as-suffix true/false ] [--delete-services] [--delete-service-keys] [--delete-service-brokers] [--keep-files] [--no-restart-subscribed-apps] [--do-not-fail-on-missing-permissions] [--abort-on-error] [--strategy STRATEGY] [--skip-testing-phase] [--skip-idle-start] [--apps-start-timeout TIMEOUT] [--apps-stage-timeout TIMEOUT] [--apps-upload-timeout TIMEOUT] [--apps-task-execution-timeout TIMEOUT]` + util.UploadEnvHelpText, | ||
| (EXPERIMENTAL) Deploy a multi-target app archive referenced by a remote URL |
There was a problem hiding this comment.
why experimental here?
There was a problem hiding this comment.
Must have added it accidentally - fixed. I have just put it for the new flag, since we have talked about the feature being presented as experimental.
|
|
||
| var jsonObject map[string]interface{} | ||
|
|
||
| err2 := json.Unmarshal([]byte(envValue), &jsonObject) |
There was a problem hiding this comment.
can you reuse err?
There was a problem hiding this comment.
I know that reusing the same err is not recommended- I will just change the names and make them more descriptive instead of a plain err2 :)
commands/deploy_command.go
Outdated
| if GetBoolOpt(requireSecureParameters, flags) { | ||
| // Collect special ENVs: __MTA___<name>, __MTA_JSON___<name>, __MTA_CERT___<name> | ||
| parameters, err := secure_parameters.CollectFromEnv("__MTA") |
There was a problem hiding this comment.
This check happens after the upload of the mtar. Could be optimized so the validation is earlier to avoid unnecessary upload in case of some failure
There was a problem hiding this comment.
thanks for the suggestion, fixed :)
| return result, nil | ||
| } | ||
|
|
||
| func BuildSecureExtension(parameters map[string]ParameterValue, mtaID string, schemaVersion string) ([]byte, error) { |
There was a problem hiding this comment.
extract in a separate file
| if schemaVersion == "" { | ||
| schemaVersion = "3.3" | ||
| } |
There was a problem hiding this comment.
let's not hardcode versions in client
| switch currentParameterValue.Type { | ||
| case typeJSON: | ||
| parametersDescriptor[name] = currentParameterValue.ObjectContent | ||
| default: | ||
| parametersDescriptor[name] = currentParameterValue.StringContent | ||
| } |
There was a problem hiding this comment.
You can add a new method on ParameterValue and use it here (currentParameterValue.getValue())
There was a problem hiding this comment.
fixed, thanks for the suggestion :)
| func CollectFromEnv(prefix string) (map[string]ParameterValue, error) { | ||
| plainValue := prefix + "___" | ||
| jsonValue := prefix + "_JSON___" | ||
| certificateValue := prefix + "_CERT___" //X509value beacuse the certiciates are of type X509 (should be renamed) |
There was a problem hiding this comment.
Improve or remove the comment description
| return nil | ||
| } | ||
|
|
||
| func CollectFromEnv(prefix string) (map[string]ParameterValue, error) { |
There was a problem hiding this comment.
This method is quite long, try to shorten it, split in functions
| ObjectContent map[string]interface{} | ||
| } | ||
|
|
||
| func nameDuplicated(name, prefix string, result map[string]ParameterValue) error { |
There was a problem hiding this comment.
rename to something including "validate"
| envName := nameValuePair[:equalsIndex] | ||
| envValue := nameValuePair[equalsIndex+1:] | ||
|
|
||
| var name string |
There was a problem hiding this comment.
do you need to define here?
There was a problem hiding this comment.
Otherwise I have to define it in each separate case
commands/deploy_command.go
Outdated
| func (c *DeployCommand) doesUpsExist(userProvidedServiceName string) (bool, error) { | ||
| servicesOutput, err := c.cliConnection.CliCommandWithoutTerminalOutput("services") | ||
| if err != nil { | ||
| return false, fmt.Errorf("Error while checking if the UPS for secure encryption exists: %w", err) | ||
| } | ||
| stringTable := strings.Join(servicesOutput, "\n") | ||
| return findServiceName(stringTable, userProvidedServiceName), nil |
There was a problem hiding this comment.
Can we call the v3/services here instead a command?
There was a problem hiding this comment.
Completely fixed, thanks for the suggestion :)
| return true, encryptionKey, nil | ||
| } | ||
|
|
||
| func getRandomEncryptionKey() (string, error) { |
There was a problem hiding this comment.
is there some library that can do this for us?
There was a problem hiding this comment.
Could not manage to find anything in particular - saw that many use the base64 encoding which I tried but since for every 3 bytes it spits out 4 characters - I was getting 43 chars out of my 32 bytes. Also since I wanted the constraint of having only alphanumerical values and not any / or + which base64 brings, I decided the use the explicit "alphabet" of allowed symbols and then just randomly choose each character from it.
commands/deploy_command.go
Outdated
|
|
||
| userProvidedServiceName := getUpsName(mtaId, namespace) | ||
|
|
||
| isUpsCreated, _, err := c.validateUpsExistsOrElseCreateIt(userProvidedServiceName, "v1") |
There was a problem hiding this comment.
what is the idea of:
"keyId": "v1"
There was a problem hiding this comment.
we have discussed it during the multiapps-controller changes :)
| for i := range encryptionKeyBytes { | ||
| encryptionKeyBytes[i] = alphabet[int(encryptionKeyBytes[i]&63)] | ||
| } | ||
|
|
||
| return string(encryptionKeyBytes), nil |
There was a problem hiding this comment.
Does this really creates a proper 256 bits keys? Why not something like:
func generateAES256Key() ([]byte, error) {
key := make([]byte, 32) // 256 bits
_, err := rand.Read(key)
if err != nil {
return nil, err
}
return string([]byte(fmt.Sprintf("%x", key))), nil
}
Why is alphabet needed?
There was a problem hiding this comment.
Yes, it does. I tried doing it like that myself in the beginning, but the problem was that the argument in the Sprintf method determines the encoding used, thus giving different lenghts as a result. Using the fmt.Sprinf("%x", key) with the "x" argument, Go does a base16 encoding which actually converts the 32 generated bytes into 64 characters. The base16 encoding doubles the size of the byte input when it is converted to a string output, since base16 uses only the letters from 'a' to 'f' and 1 through 9, 16 symbols altogether, and since one byte can be between 0-255 - in order to represent all the 256 different values we need to have for each of the 16 unique symbols, 16 more - meaning 16x16 = 16^2 = 256
| Type typeOfValue | ||
| StringContent string | ||
| JSONContent interface{} | ||
| //ObjectContent map[string]interface{} |
There was a problem hiding this comment.
Remove this comment if not needed.
| } | ||
| } | ||
|
|
||
| func TestCollectFromEnvWhenDuplciateNames(t *testing.T) { |
| "encryptionKey": encryptionKey, | ||
| "keyId": keyID, | ||
| } | ||
| jsonBody, _ := json.Marshal(upsCredentials) |
There was a problem hiding this comment.
Why is the error from json.Marshal ignored here?
There was a problem hiding this comment.
Thanks for the suggestion :)
…lity in order to support the collecting of secrets and sending them to the backend LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
77d8ac3 to
8922dd2
Compare
LMCROSSITXSADEPLOY-2301