Page MenuHomeFreeBSD

[NEW PORT] syslogin/google-compute-engine-oslogin: Enable Google Compute OS Login features on Google Compute Engine instances
ClosedPublic

Authored by helen.koike_collabora.com on Jan 9 2018, 3:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 7:06 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM
Unknown Object (File)
Sat, Mar 2, 10:19 AM

Details

Summary
Test Plan

Install the package in a machine at the Google Compute Engine (GCE), run google_oslogin_control activate.
From an external machine install an ssh key using the oslogin GCE API:
gcloud beta compute os-login ssh-keys add --key-file=/tmp/test-key.pub
Now a new user should be seen when running getent passwd, and ssh using the above key to this user should work.

Diff Detail

Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 14297
Build 14457: arc lint + arc unit

Event Timeline

sysutils/google-compute-engine-oslogin/Makefile
7

Not needed.

18

Usually shared objects are LIB_DEPENDS. Are you sure this is a RUN_DEPEND?

20–23

This should be after USES and before MAKE_ARGS

sysutils/google-compute-engine-oslogin/pkg-deinstall
4

This should probably be "PRE-DEINSTALL".

sysutils/google-compute-engine-oslogin/pkg-plist
4

This should have a PLIST_SUB entry for PORTVERSION, then use %%PORTVERSION%% so the plist doesn't need to be generated for each update.

sysutils/google-compute-engine-oslogin/Makefile
18

Pam modules are only loaded at runtime.

23

If the version is 20171213, then the version should be that, not 1.1.2.

sysutils/google-compute-engine-oslogin/Makefile
39–47

There seems to be two tabs at the start of the lines here, one is enough.

sysutils/google-compute-engine-oslogin/pkg-plist
4

s/should/could/

sysutils/google-compute-engine-oslogin/Makefile
23

The git tag is listed as 20171213, but in the source Makefile the version is defined as 1.1.2. Its kind of weird but I think this is actually correct. Do you think this should still be changed?

https://github.com/GoogleCloudPlatform/compute-image-packages/blob/20171213/google_compute_engine_oslogin/Makefile

sysutils/google-compute-engine-oslogin/pkg-plist
4

Yes, thanks I have a bad habit of being authoritative in my wording. This is not my intent.

sysutils/google-compute-engine-oslogin/Makefile
23

I would say yes, but there is also a 1.1.2 tag that dates back from 2014, and there have been tags up to 1.3.3, at which point they changed to date tags. I do not think the 1.1.2 in the Makefile is the version of the software, only the version of the shared library.
Maybe the question should be asked upstream because it is a bit strange.

sysutils/google-compute-engine-oslogin/pkg-plist
4

Words have meaning :-)
And even in this review, there are places where "should" could have been used, for the MASTER_SITES "this is not needed and should be removed". This, here, is not something that must be changed, only one that you think could be done in another way.

sysutils/google-compute-engine-oslogin/pkg-deinstall
4

According to this doc https://www.freebsd.org/doc/en/books/porters-handbook/pkg-deinstall.html
Or this script is called with "DEINSTALL" in its argument or with "POST-DEINSTALL", there is no "PRE-DEINSTALL"

sysutils/google-compute-engine-oslogin/Makefile
23

In the Debian package that the upstream project maintain, the version is 1.1.2

koike@deb9:~$ dpkg -l | grep google-compute
ii  google-compute-engine                 2.7.2-2                        all          Google Compute Engine guest environment.
ii  google-compute-engine-oslogin         1.1.2-1+deb9                   amd64        Google Compute Engine OS Login
ii  python-google-compute-engine          2.7.2-2                        all          Google Compute Engine python library for Python 2.x.
ii  python3-google-compute-engine         2.7.2-2                        all          Google Compute Engine python library for Python 3.x.

This same upstream git repository maintains several packages. the google-compute-engine package has its version number and the google-compute-engine-oslogin has another version number.
The first one comes from this file https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/setup.py#L35
And the second from this https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/google_compute_engine_oslogin/Makefile#L5

Makefile:
MASTER_SITES removed
USES order altered
Double tabs removed
PLIST_SUB added
pkg-plist:
use %%PORTVERSION%%

sysutils/google-compute-engine-oslogin/Makefile
4

Use DISTVERSION here.

sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12

What is, exactly, the purpose of this?

Ports MUST NOT, by themselves, start, stop, restart, whatever, when being installed/deinstalled/upgraded.

sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12

It is just because when the user forget to deactivate it before deinstalling it, the user cannot ssh to the machine anymore because the pam module is not removed from /etc/pam.d/sshd and it keeps trying to load a module that doesn't exist anymore

sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12

The reason for doing it does not matter at all. Ports MUST NOT start/stop themselves.

It is the job of the package manager to do the stopping and starting, which it does, if you enable it by setting HANDLE_RC_SCRIPTS to true in its configuration, or on the command line.

With it disabled:

# pkg -o HANDLE_RC_SCRIPTS=false delete bind912
Checking integrity... done (0 conflicting)
Deinstallation has been requested for the following 1 packages (of 0 packages in the universe):

Installed packages to be REMOVED:
        bind912-9.12.0.rc1_2

Number of packages to be removed: 1

The operation will free 56 MiB.

Proceed with deinstalling packages? [y/N]: y
[1/1] Deinstalling bind912-9.12.0.rc1_2...
You may need to manually remove /usr/local/etc/mtree/BIND.chroot.local.dist if it is no longer needed.
You may need to manually remove /usr/local/etc/namedb/named.conf if it is no longer needed.
[1/1] Deleting files for bind912-9.12.0.rc1_2: 100%

With it enabled:

# pkg -o HANDLE_RC_SCRIPTS=true delete bind912
Checking integrity... done (0 conflicting)
Deinstallation has been requested for the following 1 packages (of 0 packages in the universe):

Installed packages to be REMOVED:
        bind912-9.12.0.rc1_2

Number of packages to be removed: 1

The operation will free 56 MiB.

Proceed with deinstalling packages? [y/N]: y
[1/1] Deinstalling bind912-9.12.0.rc1_2...
Stopping named.
Waiting for PIDS: 53385.
You may need to manually remove /usr/local/etc/mtree/BIND.chroot.local.dist if it is no longer needed.
You may need to manually remove /usr/local/etc/namedb/named.conf if it is no longer needed.

s/PORTVERSION/DISTVERSION
delete pkg-deinstall file

helen.koike_collabora.com added inline comments.
sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12

ok, we live and learn :)

sysutils/google-compute-engine-oslogin/Makefile
23

I missed the versioning at the bottom. The devs probably decided its too annoying to use the major/minor/patch versioning based on the README.md at the bottom and changed to using date.

Using date as the devs decided to switch to is the right call IMO.

36

The append flag here is probably not necessary. This can be checked with "make -V PLIST_SUB".

helen.koike_collabora.com marked 2 inline comments as done.

s/PLIST_SUB=+/PLIST_SUB=/

sysutils/google-compute-engine-oslogin/Makefile
23

Please, see comment on the matter from the upstream maintainer here https://github.com/GoogleCloudPlatform/compute-image-packages/issues/533#issuecomment-357001546
According to him the release tag is not the package version number and we should use version in the OS Login Makefile for the package version aka 1.1.2

I'm running QA now I think this is the last item that needs to be addressed.

sysutils/google-compute-engine-oslogin/Makefile
34

SHEBANG_FILES is listed but shebangfix is not in USES. Are you sure this is needed?

sysutils/google-compute-engine-oslogin/Makefile
39–40

This could be combined into a single command:

@${REINPLACE_CMD} -e 's|/etc/sudoers.d|${PREFIX}/etc/sudoers.d|g ; \

s|/usr/bin|${PREFIX}/bin|g' ${WRKSRC}/bin/google_oslogin_control
44–47

This could be combined into a single command.

QA looks good. Update one more time then will wait a couple more days just in case anyone else sees something I missed and will get it in the tree on the weekend.

Thanks for working on this Helen. =]

Remove unecessary SHEBANG_FILES
Combine REINPLACE_CMD in a single command
Combine STRIP_CMD in a single command

This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2018, 12:14 AM
This revision was automatically updated to reflect the committed changes.