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.

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

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ultima added inline comments.Jan 10 2018, 9:23 PM
sysutils/google-compute-engine-oslogin/Makefile
6 ↗(On Diff #37679)

Not needed.

17 ↗(On Diff #37679)

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

19–22 ↗(On Diff #37679)

This should be after USES and before MAKE_ARGS

sysutils/google-compute-engine-oslogin/pkg-deinstall
3 ↗(On Diff #37679)

This should probably be "PRE-DEINSTALL".

sysutils/google-compute-engine-oslogin/pkg-plist
3 ↗(On Diff #37679)

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

mat added inline comments.Jan 10 2018, 10:40 PM
sysutils/google-compute-engine-oslogin/Makefile
17 ↗(On Diff #37679)

Pam modules are only loaded at runtime.

22 ↗(On Diff #37679)

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

mat added inline comments.Jan 10 2018, 10:58 PM
sysutils/google-compute-engine-oslogin/Makefile
38–46 ↗(On Diff #37679)

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

mat added inline comments.Jan 10 2018, 11:00 PM
sysutils/google-compute-engine-oslogin/pkg-plist
3 ↗(On Diff #37679)

s/should/could/

ultima added inline comments.Jan 10 2018, 11:02 PM
sysutils/google-compute-engine-oslogin/Makefile
22 ↗(On Diff #37679)

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

ultima added inline comments.Jan 10 2018, 11:45 PM
sysutils/google-compute-engine-oslogin/pkg-plist
3 ↗(On Diff #37679)

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

mat added inline comments.Jan 11 2018, 9:17 AM
sysutils/google-compute-engine-oslogin/Makefile
22 ↗(On Diff #37679)

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
3 ↗(On Diff #37679)

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
3 ↗(On Diff #37679)

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
22 ↗(On Diff #37679)

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%%

helen.koike_collabora.com marked 7 inline comments as done.Jan 11 2018, 1:50 PM
mat added inline comments.Jan 11 2018, 1:58 PM
sysutils/google-compute-engine-oslogin/Makefile
4 ↗(On Diff #37797)

Use DISTVERSION here.

sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12 ↗(On Diff #37797)

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 ↗(On Diff #37797)

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

mat added inline comments.Jan 11 2018, 2:38 PM
sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12 ↗(On Diff #37797)

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 marked 4 inline comments as done.Jan 11 2018, 3:02 PM
helen.koike_collabora.com added inline comments.
sysutils/google-compute-engine-oslogin/pkg-deinstall
3–12 ↗(On Diff #37797)

ok, we live and learn :)

ultima added inline comments.Jan 11 2018, 3:54 PM
sysutils/google-compute-engine-oslogin/Makefile
22 ↗(On Diff #37679)

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.

35 ↗(On Diff #37804)

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.Jan 11 2018, 5:57 PM
helen.koike_collabora.com updated this revision to Diff 37818.

s/PLIST_SUB=+/PLIST_SUB=/

sysutils/google-compute-engine-oslogin/Makefile
22 ↗(On Diff #37679)

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
33 ↗(On Diff #37818)

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

ultima added inline comments.Jan 18 2018, 4:44 AM
sysutils/google-compute-engine-oslogin/Makefile
38–39 ↗(On Diff #37818)

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
43–46 ↗(On Diff #37818)

This could be combined into a single command.

ultima added a comment.EditedJan 18 2018, 4:50 AM

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

helen.koike_collabora.com marked 2 inline comments as done.Jan 19 2018, 5:43 PM
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.