From 257a0e8a827e0344d3fdc29016eac16a830216aa Mon Sep 17 00:00:00 2001 From: Kin Ueng Date: Tue, 3 Mar 2020 22:44:07 -0600 Subject: [PATCH 1/4] Move command action user from label to annotation - Move the `kappnav-job-user-id` key/value from labels to annotations in support of special characters in usernames. This change impacts the APIs that get and submit command actions. - Add more logging - Word wrap a few very long code lines --- .../application/rest/v1/ActionsEndpoint.java | 85 ++++++++++++++----- 1 file changed, 66 insertions(+), 19 deletions(-) diff --git a/src/main/java/application/rest/v1/ActionsEndpoint.java b/src/main/java/application/rest/v1/ActionsEndpoint.java index 1ac2945..febb822 100644 --- a/src/main/java/application/rest/v1/ActionsEndpoint.java +++ b/src/main/java/application/rest/v1/ActionsEndpoint.java @@ -115,7 +115,6 @@ public class ActionsEndpoint extends KAppNavEndpoint { // App nav job labels private static final String KAPPNAV_JOB_TYPE = "kappnav-job-type"; private static final String KAPPNAV_JOB_ACTION_NAME = "kappnav-job-action-name"; - private static final String KAPPNAV_JOB_USER_ID = "kappnav-job-user-id"; private static final String KAPPNAV_JOB_COMPONENT_KIND = "kappnav-job-component-kind"; private static final String KAPPNAV_JOB_COMPONENT_SUB_KIND = "kappnav-job-component-sub-kind"; @@ -131,6 +130,12 @@ public class ActionsEndpoint extends KAppNavEndpoint { private static final String STATUS_PROPERTY_NAME = "status"; private static final String COMPLETION_TIME_PROPERTY_NAME = "completionTime"; + + // Annotation properties. + private static final String METADATA_PROPERTY_NAME = "metadata"; + private static final String ANNOTATIONS_PROPERTY_NAME = "annotations"; + private static final String KAPPNAV_JOB_USER_ID = "kappnav-job-user-id"; + @Inject private ComponentInfoRegistry registry; @@ -293,9 +298,6 @@ public Response getCommands(@CookieParam("kappnav-user") @DefaultValue("") @Para // Build the selector for the query. final Selector s = new Selector().addMatchLabel(KAPPNAV_JOB_TYPE, KAPPNAV_JOB_COMMAND_TYPE); - if (user != null && !user.isEmpty()) { - s.addMatchLabel(KAPPNAV_JOB_USER_ID, user); - } // convert time to timestamp in yyyy-MM-dd'T'HH:mm:sss format Timestamp timelaterTimestamp = convertTimeStringToTimestamp(time); @@ -313,10 +315,20 @@ public Response getCommands(@CookieParam("kappnav-user") @DefaultValue("") @Para List commands = getItemsAsList(client, batch.listNamespacedJob(GLOBAL_NAMESPACE, null, null, null, null, labelSelector, null, null, null, null)); final CommandsResponse response = new CommandsResponse(); - commands.forEach(v -> { + commands.forEach(v -> { if (v.get(KIND_PROPERTY_NAME) == null) { v.addProperty(KIND_PROPERTY_NAME, JOB_KIND); - } + } + + if(user != null && !user.isEmpty()) { + // Only return jobs belonging to the user + final String job_username = getCommandUserName(v); + if(! user.equals(job_username)) { + // Completely skip this command because it does + // not belong to the user requested by the API caller + return; + } + } if (time == null || time.isEmpty()) { //no time specified, return all jobs @@ -345,6 +357,7 @@ public Response getCommands(@CookieParam("kappnav-user") @DefaultValue("") @Para } } }); + // If there are jobs, get actions available for jobs and add to response if ( ! commands.isEmpty() && response.size() > 0) { JsonObject job= commands.get(0); // get first job, any job, so we can retrieve actions @@ -525,10 +538,15 @@ private String resolve(ApiClient client, String name, String kind, String apiVer return resolvedValue.getValue(); } - private Response executeCommand(String jsonstr, String name, String kind, String apiVersion, String namespace, String commandName, String appName, String appNamespace, String user) { - String methodName = "executeCommand"; + private Response executeCommand(String jsonstr, String name, String kind, String apiVersion, String namespace, + String commandName, String appName, String appNamespace, String user) { + + final String methodName = "executeCommand"; if (Logger.isErrorEnabled()) { - Logger.log(className, methodName, Logger.LogType.ENTRY, "Name=" + name + ", kind="+ kind + ", apiVersion="+apiVersion + ", namespace="+namespace + ", commandName="+commandName + ", appName="+appName + ", appNamespace=" + appNamespace + ", user=" +user); + Logger.log(className, methodName, Logger.LogType.ENTRY, + "name=" + name + ", kind=" + kind + ", apiVersion=" + apiVersion + ", namespace=" + namespace + + ", commandName=" + commandName + ", appName=" + appName + ", appNamespace=" + appNamespace + + ", user=" + user); } try { @@ -630,10 +648,10 @@ private Response executeCommand(String jsonstr, String name, String kind, String // Add context labels to the job, allowing for queries using selectors. final Map labels = createJobLabels(client, resource, name, kind, - namespace, appName, appNamespace, commandName, user); + namespace, appName, appNamespace, commandName); meta.setLabels(labels); - final Map annotations = createJobAnnotations(text); + final Map annotations = createJobAnnotations(text, user); if (annotations != null) { meta.setAnnotations(annotations); } @@ -733,27 +751,27 @@ private void setSecurityContextAndServiceAccountName(ApiClient client, V1PodSpec spec.setServiceAccountName(config.getkAppNavServiceAccountName()); } - private Map createJobAnnotations(String text) { + private Map createJobAnnotations(String text, String user) { if (text != null && !text.isEmpty()) { - return Collections.singletonMap(KAPPNAV_JOB_ACTION_TEXT, text); + Map annotations = new HashMap(); + annotations.put(KAPPNAV_JOB_ACTION_TEXT, text); + annotations.put(KAPPNAV_JOB_USER_ID, user); + return Collections.unmodifiableMap(annotations); } return null; } private Map createJobLabels(ApiClient client, JsonObject resource, String name, String kind, - String namespace, String appName, String appNamespace, String actionName, String userId) { + String namespace, String appName, String appNamespace, String actionName) { String methodName = "createJobLabels"; if (Logger.isEntryEnabled()) { - Logger.log(className, methodName, Logger.LogType.ENTRY, "Resource=" + resource.toString() + ", name="+ name + ", kind=" + kind + ", namespace="+namespace + ", appName=" + appName + ", appNamespace=" + appNamespace + ", actionName=" + actionName + ", userId="+userId); + Logger.log(className, methodName, Logger.LogType.ENTRY, "Resource=" + resource.toString() + ", name="+ name + ", kind=" + kind + ", namespace="+namespace + ", appName=" + appName + ", appNamespace=" + appNamespace + ", actionName=" + actionName); } final Map labels = new HashMap<>(); labels.put(KAPPNAV_JOB_TYPE, KAPPNAV_JOB_COMMAND_TYPE); labels.put(KAPPNAV_JOB_ACTION_NAME, actionName); - // Set the user id if it's available. - if (userId != null && !userId.isEmpty()) { - labels.put(KAPPNAV_JOB_USER_ID, userId); - } + // If appName is not set this resource is an application. Only set the application labels. if (appName == null || appName.isEmpty()) { labels.put(KAPPNAV_JOB_APPLICATION_NAME, name); @@ -871,4 +889,33 @@ private static String urlDecode(String value) { throw new RuntimeException(ex.getCause()); } } + + /** + * Return the username associated with the command action + * @param job - JsonObject of the job details + * @return String - username who created the job, else empty String + */ + private String getCommandUserName(JsonObject job) { + final String methodName = "getCommandUserName"; + if (Logger.isEntryEnabled()) { + Logger.log(className, methodName, Logger.LogType.ENTRY, "job=" + job); + } + String result = ""; + + final JsonObject metadata = job.getAsJsonObject(METADATA_PROPERTY_NAME); + if (metadata != null) { + final JsonObject annotations = metadata.getAsJsonObject(ANNOTATIONS_PROPERTY_NAME); + if (annotations != null) { + JsonElement userId = annotations.get(KAPPNAV_JOB_USER_ID); + if (userId != null) { + result = userId.getAsString(); + } + } + } + + if (Logger.isExitEnabled()) { + Logger.log(className, methodName, Logger.LogType.EXIT, "result=" + result); + } + return result; + } } From 1ca2947112e02bfe2afd65df61e8322e04d8042b Mon Sep 17 00:00:00 2001 From: Kin Ueng Date: Tue, 3 Mar 2020 22:57:17 -0600 Subject: [PATCH 2/4] Add missing `final` to userId variable --- src/main/java/application/rest/v1/ActionsEndpoint.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/application/rest/v1/ActionsEndpoint.java b/src/main/java/application/rest/v1/ActionsEndpoint.java index febb822..c4f8ef5 100644 --- a/src/main/java/application/rest/v1/ActionsEndpoint.java +++ b/src/main/java/application/rest/v1/ActionsEndpoint.java @@ -906,7 +906,7 @@ private String getCommandUserName(JsonObject job) { if (metadata != null) { final JsonObject annotations = metadata.getAsJsonObject(ANNOTATIONS_PROPERTY_NAME); if (annotations != null) { - JsonElement userId = annotations.get(KAPPNAV_JOB_USER_ID); + final JsonElement userId = annotations.get(KAPPNAV_JOB_USER_ID); if (userId != null) { result = userId.getAsString(); } From 128c66ab47416f1941aff94f02cc01ee55bec877 Mon Sep 17 00:00:00 2001 From: Kin Ueng Date: Wed, 4 Mar 2020 10:28:18 -0600 Subject: [PATCH 3/4] Support command actions with username in labels - Look for any legacy command actions that still have usernames in the labels instead of annotations. - Rename the method to be very clear it is for "command actions" and not "command" --- .../application/rest/v1/ActionsEndpoint.java | 43 ++++++++++++++++--- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/main/java/application/rest/v1/ActionsEndpoint.java b/src/main/java/application/rest/v1/ActionsEndpoint.java index c4f8ef5..7108b4b 100644 --- a/src/main/java/application/rest/v1/ActionsEndpoint.java +++ b/src/main/java/application/rest/v1/ActionsEndpoint.java @@ -113,6 +113,7 @@ public class ActionsEndpoint extends KAppNavEndpoint { private static final String KAPPNAV_JOB_ACTION_TEXT = "kappnav-job-action-text"; // App nav job labels + private static final String LABELS_PROPERTY_NAME = "labels"; private static final String KAPPNAV_JOB_TYPE = "kappnav-job-type"; private static final String KAPPNAV_JOB_ACTION_NAME = "kappnav-job-action-name"; @@ -322,7 +323,7 @@ public Response getCommands(@CookieParam("kappnav-user") @DefaultValue("") @Para if(user != null && !user.isEmpty()) { // Only return jobs belonging to the user - final String job_username = getCommandUserName(v); + final String job_username = getCommandActionUserName(v); if(! user.equals(job_username)) { // Completely skip this command because it does // not belong to the user requested by the API caller @@ -891,24 +892,52 @@ private static String urlDecode(String value) { } /** - * Return the username associated with the command action - * @param job - JsonObject of the job details - * @return String - username who created the job, else empty String + * Return the username associated with the command action + * @param commandAction - JsonObject of the command action details + * @return String - username who created the command action, else empty String */ - private String getCommandUserName(JsonObject job) { + private String getCommandActionUserName(JsonObject commandAction) { final String methodName = "getCommandUserName"; if (Logger.isEntryEnabled()) { - Logger.log(className, methodName, Logger.LogType.ENTRY, "job=" + job); + Logger.log(className, methodName, Logger.LogType.ENTRY, "job=" + commandAction); } String result = ""; - final JsonObject metadata = job.getAsJsonObject(METADATA_PROPERTY_NAME); + // + // Look for the username in the annotations + // + final JsonObject metadata = commandAction.getAsJsonObject(METADATA_PROPERTY_NAME); if (metadata != null) { final JsonObject annotations = metadata.getAsJsonObject(ANNOTATIONS_PROPERTY_NAME); if (annotations != null) { final JsonElement userId = annotations.get(KAPPNAV_JOB_USER_ID); if (userId != null) { result = userId.getAsString(); + if (Logger.isDebugEnabled()) { + Logger.log(className, methodName, Logger.LogType.DEBUG, "userId=" + userId); + } + } + } + } + + // + // Legacy Command Actions: + // Look for the username in the labels, if we did not find the username + // in the annotations + // + if(result != null && result.isEmpty()) { + if (metadata != null) { + final JsonObject metadataObj = metadata.getAsJsonObject(); + final JsonElement labels = metadataObj.get(LABELS_PROPERTY_NAME); + if (labels != null && labels.isJsonObject()) { + final JsonObject labelsObj = labels.getAsJsonObject(); + final JsonElement legacy_userId = labelsObj.get(KAPPNAV_JOB_USER_ID); + if (legacy_userId != null) { + result = legacy_userId.getAsString(); + if (Logger.isDebugEnabled()) { + Logger.log(className, methodName, Logger.LogType.DEBUG, "legacy_userId=" + legacy_userId); + } + } } } } From f49d15e0cb093ae02dee8ead34b236726f8bc022 Mon Sep 17 00:00:00 2001 From: Kin Ueng Date: Wed, 4 Mar 2020 14:02:28 -0600 Subject: [PATCH 4/4] Separate text and user annotations logic - Per code review, do not use the if-condition for `text` when determining whether to add the username to the annotations. --- src/main/java/application/rest/v1/ActionsEndpoint.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/application/rest/v1/ActionsEndpoint.java b/src/main/java/application/rest/v1/ActionsEndpoint.java index 7108b4b..6b289fe 100644 --- a/src/main/java/application/rest/v1/ActionsEndpoint.java +++ b/src/main/java/application/rest/v1/ActionsEndpoint.java @@ -753,10 +753,16 @@ private void setSecurityContextAndServiceAccountName(ApiClient client, V1PodSpec } private Map createJobAnnotations(String text, String user) { + Map annotations = new HashMap(); + if (text != null && !text.isEmpty()) { - Map annotations = new HashMap(); annotations.put(KAPPNAV_JOB_ACTION_TEXT, text); + } + if(user != null && !user.isEmpty()) { annotations.put(KAPPNAV_JOB_USER_ID, user); + } + + if(! annotations.isEmpty()) { return Collections.unmodifiableMap(annotations); } return null;