-
Notifications
You must be signed in to change notification settings - Fork 74
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
Support alt. namespace resource uuid as tenant id to API gatway service #1076
Changes from all commits
48703d7
79b38f1
6e20393
921103c
6de5cb9
0b5f385
39c0596
16fcda0
6c011fc
338a3e5
e4dff0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ import ( | |
const ( | ||
SOURCE_WSKPROPS = ".wskprops" | ||
SOURCE_WHISK_PROPERTIES = "whisk.properties" | ||
SOURCE_DEFAULT_VALUE = "wskdeploy default" // TODO() i18n? | ||
SOURCE_DEFAULT_VALUE = "wskdeploy default" | ||
) | ||
|
||
var ( | ||
|
@@ -46,6 +46,7 @@ var ( | |
key = PropertyValue{} | ||
cert = PropertyValue{} | ||
apigwAccessToken = PropertyValue{} | ||
apigwTenantId = PropertyValue{} | ||
additionalHeaders = make(http.Header) | ||
) | ||
|
||
|
@@ -70,6 +71,7 @@ var GetWskPropFromWhiskProperty = func(pi whisk.Properties) (*whisk.Wskprops, er | |
return whisk.GetWskPropFromWhiskProperty(pi) | ||
} | ||
|
||
// TODO implement a command line flag for APIGW_TENANT_ID | ||
var GetCommandLineFlags = func() (string, string, string, string, string, string) { | ||
return utils.Flags.ApiHost, utils.Flags.Auth, utils.Flags.Namespace, utils.Flags.Key, utils.Flags.Cert, utils.Flags.ApigwAccessToken | ||
} | ||
|
@@ -92,6 +94,7 @@ func resetWhiskConfig() { | |
key = PropertyValue{} | ||
cert = PropertyValue{} | ||
apigwAccessToken = PropertyValue{} | ||
apigwTenantId = PropertyValue{} | ||
} | ||
|
||
func readFromCLI() { | ||
|
@@ -103,13 +106,16 @@ func readFromCLI() { | |
key = GetPropertyValue(key, keyfile, wski18n.COMMAND_LINE) | ||
cert = GetPropertyValue(cert, certfile, wski18n.COMMAND_LINE) | ||
apigwAccessToken = GetPropertyValue(apigwAccessToken, accessToken, wski18n.COMMAND_LINE) | ||
// TODO optionally allow this value to be set from command line arg. | ||
//apigwTenantId = GetPropertyValue(apigwTenantId, tenantId, wski18n.COMMAND_LINE) | ||
} | ||
|
||
func setWhiskConfig(cred string, ns string, host string, token string, source string) { | ||
credential = GetPropertyValue(credential, cred, source) | ||
namespace = GetPropertyValue(namespace, ns, source) | ||
apiHost = GetPropertyValue(apiHost, host, source) | ||
apigwAccessToken = GetPropertyValue(apigwAccessToken, token, source) | ||
// TODO decide if we should allow APIGW_TENANT_ID in manifest | ||
} | ||
|
||
func readFromDeploymentFile(deploymentPath string) { | ||
|
@@ -129,6 +135,7 @@ func readFromManifestFile(manifestPath string) { | |
mm := parsers.NewYAMLParser() | ||
manifest, _ := mm.ParseManifest(manifestPath) | ||
p := manifest.GetProject() | ||
// TODO look to deprecate reading Namespace, APIGW values from manifest or depl. YAML files | ||
setWhiskConfig(p.Credential, p.Namespace, p.ApiHost, p.ApigwAccessToken, path.Base(manifestPath)) | ||
} | ||
} | ||
|
@@ -143,6 +150,7 @@ func readFromWskprops(pi whisk.PropertiesImp, proppath string) { | |
key = GetPropertyValue(key, wskprops.Key, SOURCE_WSKPROPS) | ||
cert = GetPropertyValue(cert, wskprops.Cert, SOURCE_WSKPROPS) | ||
apigwAccessToken = GetPropertyValue(apigwAccessToken, wskprops.AuthAPIGWKey, SOURCE_WSKPROPS) | ||
apigwTenantId = GetPropertyValue(apigwTenantId, wskprops.APIGWTenantId, SOURCE_WSKPROPS) | ||
} | ||
|
||
func readFromWhiskProperty(pi whisk.PropertiesImp) { | ||
|
@@ -174,6 +182,12 @@ func readFromWhiskProperty(pi whisk.PropertiesImp) { | |
map[string]interface{}{wski18n.KEY_KEY: wski18n.APIGW_ACCESS_TOKEN}) | ||
wskprint.PrintlnOpenWhiskWarning(warnMsg) | ||
} | ||
apigwTenantId = GetPropertyValue(apigwTenantId, whiskproperty.APIGWTenantId, SOURCE_WHISK_PROPERTIES) | ||
if apigwTenantId.Source == SOURCE_WHISK_PROPERTIES { | ||
warnMsg = wski18n.T(wski18n.ID_WARN_WHISK_PROPS_DEPRECATED, | ||
map[string]interface{}{wski18n.KEY_KEY: wski18n.APIGW_TENANT_ID}) | ||
wskprint.PrintlnOpenWhiskWarning(warnMsg) | ||
} | ||
} | ||
|
||
// we are reading openwhisk credentials (apihost, namespace, and auth) in the following precedence order: | ||
|
@@ -198,7 +212,7 @@ func NewWhiskConfig(proppath string, deploymentPath string, manifestPath string) | |
|
||
// TODO() i18n | ||
// Print all flags / values if verbose | ||
wskprint.PrintlnOpenWhiskVerbose(utils.Flags.Verbose, wski18n.CONFIGURATION+":\n"+utils.Flags.Format()) | ||
//wskprint.PrintlnOpenWhiskVerbose(utils.Flags.Verbose, wski18n.CONFIGURATION+":\n"+utils.Flags.Format()) | ||
|
||
// now, read them from deployment file if not found on command line | ||
readFromDeploymentFile(deploymentPath) | ||
|
@@ -228,17 +242,22 @@ func NewWhiskConfig(proppath string, deploymentPath string, manifestPath string) | |
} | ||
|
||
clientConfig = &whisk.Config{ | ||
AuthToken: credential.Value, //Authtoken | ||
Namespace: namespace.Value, //Namespace | ||
Host: apiHost.Value, | ||
Version: "v1", // TODO() should not be hardcoded, should warn user of default | ||
AuthToken: credential.Value, //Authtoken | ||
Namespace: namespace.Value, //Namespace | ||
Host: apiHost.Value, | ||
Version: "v1", // TODO() should not be hardcoded, should warn user of default | ||
//Version: Apiversion | ||
Cert: cert.Value, | ||
Key: key.Value, | ||
Insecure: mode, // true if you want to ignore certificate signing | ||
ApigwAccessToken: apigwAccessToken.Value, | ||
ApigwTenantId: apigwTenantId.Value, | ||
AdditionalHeaders: additionalHeaders, | ||
} | ||
|
||
// Print all flags / values if verbose | ||
wskprint.PrintlnOpenWhiskVerbose(utils.Flags.Verbose, wski18n.CLI_FLAGS+":\n"+utils.Flags.Format()) | ||
|
||
// validate we have credential, apihost and namespace | ||
err := validateClientConfig(credential, apiHost, namespace) | ||
return clientConfig, err | ||
|
@@ -258,13 +277,11 @@ func validateClientConfig(credential PropertyValue, apiHost PropertyValue, names | |
if len(apiHost.Value) == 0 { | ||
errorMsg = wskderrors.AppendDetailToErrorMessage( | ||
errorMsg, wski18n.T(wski18n.ID_MSG_CONFIG_MISSING_APIHOST), 1) | ||
|
||
} | ||
|
||
if len(namespace.Value) == 0 { | ||
errorMsg = wskderrors.AppendDetailToErrorMessage( | ||
errorMsg, wski18n.T(wski18n.ID_MSG_CONFIG_MISSING_NAMESPACE), 1) | ||
|
||
} | ||
|
||
if len(errorMsg) > 0 { | ||
|
@@ -291,5 +308,11 @@ func validateClientConfig(credential PropertyValue, apiHost PropertyValue, names | |
wskprint.PrintOpenWhiskVerbose(utils.Flags.Verbose, stdout) | ||
} | ||
|
||
if len(apigwTenantId.Value) != 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is verbose, should the user know that their APIGW_TENANT_ID is empty if they are targeting CF namespace? I think we should print out the apigwAccessTenantId regardless the type of namespace being targeted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is my belief that as the code is not authored that all values are either printed (verbose) or not (default/non-verbose); there should be no distinction between these 2 values or any correlation between them that assumes one or the other (i.e., should always print all known properties without control logic). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok good to know. |
||
stdout = wski18n.T(wski18n.ID_MSG_CONFIG_INFO_APIGW_TENANT_ID_X_source_X, | ||
map[string]interface{}{wski18n.KEY_UUID: apigwTenantId, wski18n.KEY_SOURCE: apigwTenantId.Source}) | ||
wskprint.PrintOpenWhiskVerbose(utils.Flags.Verbose, stdout) | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,7 +229,7 @@ type Project struct { | |
Namespace string `yaml:"namespace"` | ||
Credential string `yaml:"credential"` | ||
ApiHost string `yaml:"apiHost"` | ||
ApigwAccessToken string `yaml:"apigwAccessToken"` | ||
ApigwAccessToken string `yaml:"apigwAccessToken"` // TODO: support apigwTenantId? deprecate? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users potentially could use the wrong CRN# in yml file, and if that happens, the value would've been overwritten by the values in wskprops file. The api would be deployed in a namespace unexpected by the users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is the intent, that is the values provided in the manifest/deployment files are per-deployment overrides... if they fail that is OK. Also, it seems that if you have more than 1 IAM namespace, there needs to be a way to toggle between them more dynamically. Looking at my account, i have 2 "default" IAM namespaces now... there is no UI element (or command line) to allow me to say which is truly my "Default" now; therefore, without a way to set this manually, the one auto-set in .wskprops seems to pick one of the 2 arbitrarily... These options allow user to decide and explicitly provide the namespace they intended (via this mechanism, while treating the .wskprops value as the default) |
||
Version string `yaml:"version"` | ||
Packages map[string]Package `yaml:"packages"` | ||
Inputs map[string]Parameter `yaml: parameters` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this is necessary. I don't think CRN # is intended for users to manipulate directly. I'll double check with whisk-team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with wskdeploy, we had taken the approach of allowing users to provide per-deployment settings via command line or a deployment file that accounted for different values in wskprops as it is often the case that diff. namespaces, tokens, certs, etc. are used for different stages of lifecycle (e.g., dev/test/prod). If we do not provide a way to set these values per-deployment, then we have to ask the user/developer to always go back and manually edit the .wskprops or provide multiple whisk.properties files they change between. We tried to take a friendlier approach... otherwise, I see it very challenging to manage namespaces dynamically as it is part of the programming model and can change very dynamically (not ideal for changing a supposedly hidden .wskprops file).