hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Vasudev (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (YARN-6623) Add support to turn off launching privileged containers in the container-executor
Date Wed, 20 Sep 2017 18:40:00 GMT

     [ https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Varun Vasudev updated YARN-6623:
--------------------------------
    Attachment: YARN-6623.010.patch

Thanks for the reviews [~miklos.szegedi@cloudera.com] and [~shanekumpf@gmail.com]!

I've uploaded a new patch to address your comments.

{noformat}
84 printWriter.println(" " + entry.getKey() + "=" + StringUtils 
85 .join(",", entry.getValue())); 

writeCommandToTempFile: entry.getKey() can still contain an = in the latest patch, which is
an issue. It probably needs to be filtered in addCommandArguments.
{noformat}

Fixed.

{noformat}
701    char *get_config_path(const char *argv0) {
702      char *executable_file = get_executable((char *) argv0);
703      if (!executable_file) {
704        fprintf(ERRORFILE, "realpath of executable: %s\n",
705                errno != 0 ? strerror(errno) : "unknown");
706        return NULL;
707      }

It is probably a good idea to check for executable_file[0] != 0 as well
{noformat}

Fixed.

{noformat}
1150      size_t command_size = MIN(sysconf(_SC_ARG_MAX), 128*1024);
1151      char *buffer = alloc_and_clear_memory(command_size, sizeof(char));
1152      ret = get_docker_command(command_file, &CFG, buffer, EXECUTOR_PATH_MAX);

The code passes in a different size than the actual size of the buffer.
{noformat}

Good catch! Fixed.

{noformat}
157    inline void* alloc_and_clear_memory(size_t num, size_t size) {
158      void *ret = calloc(num, size);
159      if (ret == NULL) {
160        exit(OUT_OF_MEMORY);
161      }
162      return ret;
163    }

It might be a good idea to print an error message here.
{noformat}

Fixed.

{noformat}
42    static int add_to_buffer(char *buff, const size_t bufflen, const char *string) {

Why do not you use strncat inside? It would spare one of the strlen’s.
{noformat}

How would you detect the condition where the buffer doesn't have enough size?

{noformat}
105            if(prefix != 0) {
106              tmp_ptr = strchr(values[i], prefix);
107              if (tmp_ptr == NULL) {
...

This feels a little bit less readable. I would suggest having a len instead of tmp_ptr defaulted
to strlen(tmp_ptr); Also, am I right that we are checking only if the left device is allowed?
{noformat}

I didn't quite understand this. What would the len do? Your understanding is correct, we're
checking if the left device is allowed.

{noformat}
150      if (ret != 0) {
151        memset(out, 0, outlen);
152      }

out[0]=0 should be sufficient, if outlen > 0 and ret != 0
{noformat}

I prefer to memset the entire buffer.

{noformat}
162 if (0 == strncmp(container_name, CONTAINER_NAME_PREFIX, strlen(CONTAINER_NAME_PREFIX)))
{ 

There is no need of an strlen here, sizeof is sufficient and calculates compile time
{noformat}

I tried to change this to sizeof but I got a compiler warning complaining about it.

{noformat}
283 ret = add_docker_config_param(&command_config, out, outlen); 
284 if (ret != 0) { 
285 return BUFFER_TOO_SMALL; 

Container name is not freed.
{noformat}

Fixed.

{noformat}
330 if (ret != 0) { 
331 return BUFFER_TOO_SMALL; 
332 } 

Image name is not freed.
{noformat}

Fixed.

{noformat}
381 quote_and_append_arg(&tmp_buffer, &tmp_buffer_size, " ", image_name); 

That space might need to be added to the quote_and_append_arg function for safety reasons.
{noformat}

I didn't get this. Can you please explain?

{noformat}
564 * 2. If the path is a directory, add a '/' at the end( if not present) 

There is a small typo here.
{noformat}

Fixed.

{noformat}
585 if (len <= 0) { 
586 return NULL; 
587 } 

There is a missing free here
{noformat}

Fixed.

{noformat}
731 strncpy(tmp_buffer_2, values[i], strlen(values[i])); 
732 strncpy(tmp_buffer_2 + strlen(values[i]), ro_suffix, strlen(ro_suffix)); 

Why do you use strncpy here? Why not strcpy and strcat?
{noformat}

Is there any benefit to strcpy + strcat?

{noformat}
Can we print out the mount that was requested but failed for the read-only and read write
checks? I found it a bit difficult to determine which mount was the cause of the failure.
Doing the same for the devices option would be helpful as well. Something like the following:

        if (permitted_rw == 0) {
          fprintf(ERRORFILE, "Requested rw mount not found in the rw whitelist '%s', realpath=%s\n",
values[i], mount_src);
          ret = INVALID_DOCKER_RW_MOUNT;
          goto free_and_exit;
-snip-
     if (ro != 0 && permitted_ro == 0 && permitted_rw == 0) {
        fprintf(ERRORFILE, "Requested ro mount not found in the ro whitelist '%s', realpath=%s\n",
values[i], mount_src);
        ret = INVALID_DOCKER_RO_MOUNT;
        goto free_and_exit;
      }
{noformat}

Fixed.

{noformat}
Currently only docker run works with this patch. I know you are aware of this. There are a
few regressions in this patch from what was fixed in YARN-6726. It would be great if we could
try to address them here. I'll list the ones I'm aware of below.
{noformat}

Fixed.

> Add support to turn off launching privileged containers in the container-executor
> ---------------------------------------------------------------------------------
>
>                 Key: YARN-6623
>                 URL: https://issues.apache.org/jira/browse/YARN-6623
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>            Priority: Blocker
>         Attachments: YARN-6623.001.patch, YARN-6623.002.patch, YARN-6623.003.patch, YARN-6623.004.patch,
YARN-6623.005.patch, YARN-6623.006.patch, YARN-6623.007.patch, YARN-6623.008.patch, YARN-6623.009.patch,
YARN-6623.010.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should add a flag
to the container-executor.cfg allowing admins to disable launching privileged containers at
the container-executor level.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message