ref(builder): remove etcd and confd #148

aledbf · 2016-02-05T21:32:51Z

closes #122
closes #81

aledbf · 2016-02-09T13:42:00Z

aledbf · 2016-02-09T13:43:33Z

krancour · 2016-02-09T17:02:27Z

I'm very highly in favor of this, however, it looks like there are commits in this PR that aren't directly part of the narrow concern of removing etcd and confd. It's all good stuff, but should this be broken up into a few separate PRs for easier review and traceability?

arschles · 2016-02-09T17:17:03Z

pkg/commands.go

-	log.Info(c, "Woke up.")
-	return true, nil
-}
-


I'm very happy that this is being removed!

💯

aledbf · 2016-02-09T17:18:03Z

but should this be broken up into a few separate PRs for easier review and traceability?

The changes in the code removes dependencies in packages (etcd and confd), the use of docker, multiples packages now are not required (Dockerfile), tests that makes no sense (interaction with workflow to read app configuration for instance), etc. which make this a difficult PR to break a part. I can remove the "replace current log with github.com/deis/pkg/log commit" but that makes no sense since I already need to update the dependencies for this PR.

That said if this PR is not accepted in the current form please indicate the way to open multiples PR that depends one on another.

arschles · 2016-02-09T17:20:35Z

pkg/gitreceive/run.go


-	client "k8s.io/kubernetes/pkg/client/unversioned"
-)
+	builder_conf "github.com/deis/builder/pkg/conf"


Can you avoid using _ in the package name and rename this builderconf?

arschles · 2016-02-09T17:25:34Z

@aledbf reviewed and left some comments. Also, I agree with @krancour, it would be very helpful if you could split some things out into different pull requests. Below is a list of some ideas for things to split out, each into separate PRs.

The transition from using github.com/deis/builder/pkg/gitreceive/log to github.com/deis/pkg/log
The change from tarUrl => tarURL and putUrl => putURL in slugBuildCase
The restructuring and switch to alpine:3.3 in the Dockerfile. The removal of etcdctl and confd should obviously stay in this PR

Thanks again for this change!

arschles · 2016-02-09T19:36:59Z

glide.lock

 - name: github.com/aws/aws-sdk-go
-  version: 87b1e60a50b09e4812dee560b33a238f67305804
+  version: ""


you may have to run make glideup to get this version back

arschles · 2016-02-09T19:55:21Z

thanks @aledbf

smothiki · 2016-02-09T20:14:27Z

pkg/controller/utils.go

+func UserInfoFromKey(key string) (*UserInfo, error) {
+	keyB64 := base64.RawURLEncoding.EncodeToString([]byte(key))
+	url, err := controllerURLStr("v2", "hooks", "key", keyB64)
+	if err != nil {


@helgi is this the correct url endpoint /v2/hooks/key/key-base64 to get user info ? Just wanna cross check.
I think its the /v2/keys calls keyview set

Yeah it's key, I dropped the plural so the old hooks worked still under keys (the one we will remove). It also made sense since we were querying on a singular key instead of getting many keys back.

technosophos · 2016-02-09T20:17:51Z

Over the long run we should tighten up security on the controller endpoint, but that's a task for a different day.

I am 💯 behind this one. I am so glad to see etcd and confd pulled out. I also did a very careful read through the SSH handling, and I think this method is good.

technosophos · 2016-02-09T20:18:35Z

And thanks, @aledbf. This was a huge PR to work through.

arschles · 2016-02-09T20:52:33Z

@aledbf I'm testing this now

arschles · 2016-02-09T21:36:18Z

@aledbf I tested with a deis/workflow#352 build and a build of this branch. The git push failed, with this in the builder logs:

[info] Checking closer.
[info] Accepted connection.
[debug] Starting ssh authentication
[debug] Checking auth for user key ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDIqRuNwhjntdJWuAVykr/873X8zzKo6Ms1Vx70BQx0wir8TpaLZEY6CqqKDrMbHZ2Z0ZV2ZITs9fC81GqIdmmDFXZxNfx+B1lSR3ZpmZpQpprtSCevZXihIgy+ND50Hp/wk+3VU54FxhudIlJgpPjb/o5vQFhiyM3ynR5gH3slWVaq9C0TkgXCnTzukGzSTeL7wYPNmLomkrAS0nk0yRfoUZcwmD++HMgEmYlhTbnMlkB3nxzEf/JQxhY6xCHrbtNbRkINCY21dHrsrr/MvBvD8FoKnKpxHX2+HNXZbe7Xl87L9o1OuXrtR1crvq+r+1fPjaynGir07zr9mgJPxouPL/e4ppxTL//vt1kVkWWkh/B+GyXEmP38bQGYMpEA7cAndlxPlOki35JYwDNn5CENQpDp8F4+JsKIzAF1zmkBIA7ngg0cSHHilNgwZXmX7h+7nngLgFIpP8h7A9fCpAKhHUfFUj+Zgl9Xm44+sZOwVBnVijK326TgVDFTUXjE/Xwny+3ERgYwBfOwOKmusNFnS0XHbmh+qa/+D8qge5bKilq48pKHzwngM/U6OwMxmSXTuHclLLen3Ime30TOiPzAhokrVNz/Z3VAkfBuJHby68SAKUgczUEU81wz5wFEt1n1sIJ5V49KMRGaSWb+eWvW81yA7NkDSjnsMLa/IF/ADQ==
[error] Failed handshake: EOF (&{{0xc820226770}})

And this in the workflow logs:

10.168.1.1 "GET /v2/hooks/key/c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFDQVFESXFSdU53aGpudGRKV3VBVnlrci84NzNYOHp6S282TXMxVng3MEJReDB3aXI4VHBhTFpFWTZDcXFLRHJNYkhaMlowWlYyWklUczlmQzgxR3FJZG1tREZYWnhOZngrQjFsU1IzWnBtWnBRcHBydFNDZXZaWGloSWd5K05ENTBIcC93ayszVlU1NEZ4aHVkSWxKZ3BQamIvbzV2UUZoaXlNM3luUjVnSDNzbFdWYXE5QzBUa2dYQ25UenVrR3pTVGVMN3dZUE5tTG9ta3JBUzBuazB5UmZvVVpjd21EKytITWdFbVlsaFRibk1sa0Izbnh6RWYvSlF4aFk2eENIcmJ0TmJSa0lOQ1kyMWRIcnNyci9NdkJ2RDhGb0tuS3B4SFgyK0hOWFpiZTdYbDg3TDlvMU91WHJ0UjFjcnZxK3IrMWZQamF5bkdpcjA3enI5bWdKUHhvdVBML2U0cHB4VEwvL3Z0MWtWa1dXa2gvQitHeVhFbVAzOGJRR1lNcEVBN2NBbmRseFBsT2tpMzVKWXdETm41Q0VOUXBEcDhGNCtKc0tJekFGMXpta0JJQTduZ2cwY1NISGlsTmd3WlhtWDdoKzdubmdMZ0ZJcFA4aDdBOWZDcEFLaEhVZkZVaitaZ2w5WG00NCtzWk93VkJuVmlqSzMyNlRnVkRGVFVYakUvWHdueSszRVJnWXdCZk93T0ttdXNORm5TMFhIYm1oK3FhLytEOHFnZTViS2lscTQ4cEtIenduZ00vVTZPd014bVNYVHVIY2xMTGVuM0ltZTMwVE9pUHpBaG9rclZOei9aM1ZBa2ZCdUpIYnk2OFNBS1VnY3pVRVU4MXd6NXdGRXQxbjFzSUo1VjQ5S01SR2FTV2IrZVd2VzgxeUE3TmtEU2puc01MYS9JRi9BRFE9PQo HTTP/1.1" 400 - "deis-builder"

And, expected output from the actual git push operation:

^CENG000656:example-go aaronschlesinger$ git push deis master
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Any ideas on what went wrong?

aledbf · 2016-02-09T21:46:28Z

@arschles yes

Any ideas on what went wrong?

yes, here https://github.com/deis/builder/pull/148/files#diff-7549650024dfe83fbb2b4fdf653c2d49R43
workflow is using https://docs.python.org/3/library/base64.html#base64.urlsafe_b64decode
and the error in python is:

old-mbp:builder aledbf$ python
Python 2.7.10 (default, Aug 22 2015, 20:33:39)
[GCC 4.2.1 Compatible Apple LLVM 7.0.0 (clang-700.0.59.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import base64
>>> base64.urlsafe_b64decode("c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFDQVFESXFSdU53aGpudGRKV3VBVnlrci84NzNYOHp6S282TXMxVng3MEJReDB3aXI4VHBhTFpFWTZDcXFLRHJNYkhaMlowWlYyWklUczlmQzgxR3FJZG1tREZYWnhOZngrQjFsU1IzWnBtWnBRcHBydFNDZXZaWGloSWd5K05ENTBIcC93ayszVlU1NEZ4aHVkSWxKZ3BQamIvbzV2UUZoaXlNM3luUjVnSDNzbFdWYXE5QzBUa2dYQ25UenVrR3pTVGVMN3dZUE5tTG9ta3JBUzBuazB5UmZvVVpjd21EKytITWdFbVlsaFRibk1sa0Izbnh6RWYvSlF4aFk2eENIcmJ0TmJSa0lOQ1kyMWRIcnNyci9NdkJ2RDhGb0tuS3B4SFgyK0hOWFpiZTdYbDg3TDlvMU91WHJ0UjFjcnZxK3IrMWZQamF5bkdpcjA3enI5bWdKUHhvdVBML2U0cHB4VEwvL3Z0MWtWa1dXa2gvQitHeVhFbVAzOGJRR1lNcEVBN2NBbmRseFBsT2tpMzVKWXdETm41Q0VOUXBEcDhGNCtKc0tJekFGMXpta0JJQTduZ2cwY1NISGlsTmd3WlhtWDdoKzdubmdMZ0ZJcFA4aDdBOWZDcEFLaEhVZkZVaitaZ2w5WG00NCtzWk93VkJuVmlqSzMyNlRnVkRGVFVYakUvWHdueSszRVJnWXdCZk93T0ttdXNORm5TMFhIYm1oK3FhLytEOHFnZTViS2lscTQ4cEtIenduZ00vVTZPd014bVNYVHVIY2xMTGVuM0ltZTMwVE9pUHpBaG9rclZOei9aM1ZBa2ZCdUpIYnk2OFNBS1VnY3pVRVU4MXd6NXdGRXQxbjFzSUo1VjQ5S01SR2FTV2IrZVd2VzgxeUE3TmtEU2puc01MYS9JRi9BRFE9PQo".strip()).
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/base64.py", line 112, in urlsafe_b64decode
    return b64decode(s, '-_')
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/base64.py", line 76, in b64decode
    raise TypeError(msg)
TypeError: Incorrect padding

arschles · 2016-02-09T21:48:16Z

@aledbf I suspected after talking with @helgi. I'm gonna try using base64.StdEncoding and base64.URLEncoding to see if either works, as both are un-padded. Will report back.

…print to controller Instead of encoded public key

…for fingerprint back

technosophos · 2016-02-09T23:05:55Z

Can we just put the key in the body and not base64 encode it?

Edit: I'm okay with the fingerprint strategy, too.

arschles · 2016-02-09T23:09:02Z

@technosophos yes, we could do that, but I'd like to keep this as a standard GET request, compliant with good REST design. I've opened aledbf#1 (to merge into the branch represented here) to switch to using a fingerprint in the URL path. deis/workflow#352 has been merged, so aledbf#1 will need to be merged into this branch before this can be merged.

Using a build of aledbf#1 against the standard deis-dev helm chart, I verified that the git push succeeds.

technosophos · 2016-02-09T23:10:22Z

Packing a huge amount of data into the URL is not good design.

arschles · 2016-02-09T23:13:12Z

It was a huge amount of data, but now it a standard public key fingerprint. I wouldn't consider that a huge amount of data, and certainly doesn't come close to any path length restrictions I know of.

technosophos · 2016-02-09T23:32:12Z

I am definitely okay with the fingerprint, provided @helgi does not object.

helgi · 2016-02-09T23:32:36Z

Already implemented and merged into workflow using fingerprint

arschles · 2016-02-09T23:45:53Z

💯 @helgi

ref(pkg/controller/utils.go,pkg/sshd/sshd.go): send public key fingerprint to the controller

mboersma · 2016-02-10T00:17:07Z

pkg/controller/utils.go

+type UserInfo struct {
+	Username    string
+	Key         string
+	FingerPrint string


This should probably be Fingerprint without InterCaps (like Username). Not worth holding up this PR though.

@mboersma can you file an issue for that?

ref(builder): remove etcd and confd

arschles · 2016-02-10T00:25:28Z

@aledbf FYI I forgot to mark as LGTM2 before I merged, but it was definitely fine beforehand ;)

slack added the in progress label Feb 5, 2016

mboersma assigned aledbf Feb 5, 2016

aledbf force-pushed the remove-etcd branch 3 times, most recently from 0e702af to 371e58f Compare February 8, 2016 18:00

aledbf mentioned this pull request Feb 8, 2016

add new url to interact with deis/builder deis/controller#351

Closed

aledbf force-pushed the remove-etcd branch from 371e58f to f49d604 Compare February 8, 2016 21:36

aledbf added the awaiting review label Feb 9, 2016

aledbf removed the in progress label Feb 9, 2016

aledbf changed the title ~~ref(builder): remove etcd and confd (WIP)~~ ref(builder): remove etcd and confd Feb 9, 2016

aledbf mentioned this pull request Feb 9, 2016

[Proposal]: add a secret to store the builder ssh keys #157

Closed

arschles reviewed Feb 9, 2016
View reviewed changes

pkg/commands.go

log.Info(c, "Woke up.")

return true, nil

}

Copy link

Member

arschles Feb 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy that this is being removed!

💯

arschles reviewed Feb 9, 2016
View reviewed changes

aledbf force-pushed the remove-etcd branch 5 times, most recently from a48280a to d2bec45 Compare February 9, 2016 19:34

arschles reviewed Feb 9, 2016
View reviewed changes

smothiki reviewed Feb 9, 2016
View reviewed changes

technosophos added the LGTM1 label Feb 9, 2016

arschles added the needs manual testing label Feb 9, 2016

ref(pkg/controller/utils.go,pkg/sshd/sshd.go): send public key finger…

87f49a2

…print to controller Instead of encoded public key

arschles mentioned this pull request Feb 9, 2016

ref(pkg/controller/utils.go,pkg/sshd/sshd.go): send public key fingerprint to the controller aledbf/builder#1

Merged

fix(pkg/controller/utils_test.go,pkg/sshd/server_test.go): add tests …

b13b3b1

…for fingerprint back

Merge pull request #1 from arschles/remove-etcd-changes

07dd30c

ref(pkg/controller/utils.go,pkg/sshd/sshd.go): send public key fingerprint to the controller

mboersma reviewed Feb 10, 2016
View reviewed changes

arschles added a commit that referenced this pull request Feb 10, 2016

Merge pull request #148 from aledbf/remove-etcd

c1a419c

ref(builder): remove etcd and confd

arschles merged commit c1a419c into deis:master Feb 10, 2016

arschles added LGTM2 and removed awaiting review labels Feb 10, 2016

mboersma mentioned this pull request Feb 10, 2016

Rename "FingerPrint" to "Fingerprint" #159

Closed

aledbf mentioned this pull request Feb 10, 2016

Send fingerprint in /v2/hooks/key/{key} instead of the key itself #158

Closed

aledbf deleted the remove-etcd branch February 10, 2016 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(builder): remove etcd and confd #148

ref(builder): remove etcd and confd #148

aledbf commented Feb 5, 2016

aledbf commented Feb 9, 2016

aledbf commented Feb 9, 2016

krancour commented Feb 9, 2016

arschles Feb 9, 2016

aledbf commented Feb 9, 2016

arschles Feb 9, 2016

arschles commented Feb 9, 2016

arschles Feb 9, 2016

arschles commented Feb 9, 2016

smothiki Feb 9, 2016

helgi Feb 9, 2016

technosophos commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

arschles commented Feb 9, 2016

aledbf commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

helgi commented Feb 9, 2016

arschles commented Feb 9, 2016

mboersma Feb 10, 2016

arschles Feb 10, 2016

arschles commented Feb 10, 2016

ref(builder): remove etcd and confd #148

ref(builder): remove etcd and confd #148

Conversation

aledbf commented Feb 5, 2016

aledbf commented Feb 9, 2016

aledbf commented Feb 9, 2016

krancour commented Feb 9, 2016

arschles Feb 9, 2016

Choose a reason for hiding this comment

aledbf commented Feb 9, 2016

arschles Feb 9, 2016

Choose a reason for hiding this comment

arschles commented Feb 9, 2016

arschles Feb 9, 2016

Choose a reason for hiding this comment

arschles commented Feb 9, 2016

smothiki Feb 9, 2016

Choose a reason for hiding this comment

helgi Feb 9, 2016

Choose a reason for hiding this comment

technosophos commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

arschles commented Feb 9, 2016

aledbf commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

arschles commented Feb 9, 2016

technosophos commented Feb 9, 2016

helgi commented Feb 9, 2016

arschles commented Feb 9, 2016

mboersma Feb 10, 2016

Choose a reason for hiding this comment

arschles Feb 10, 2016

Choose a reason for hiding this comment

arschles commented Feb 10, 2016