Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kong] Don't recreate service account #31

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

yasn77
Copy link
Contributor

@yasn77 yasn77 commented Jan 31, 2020

See issue #30

charts/kong/README.md Outdated Show resolved Hide resolved
charts/kong/README.md Outdated Show resolved Hide resolved
@rainest
Copy link
Contributor

rainest commented Feb 1, 2020

Bother, that was a convenient hack.

I think we should still provide a path for users to still have a managed ServiceAccount if they enable the controller or PSP on an existing installation, but those conversions are hopefully rare enough that we should not need to handle them automatically in the standard chart.

We can excise the existing hook ServiceAccount from migrations-pre-upgrade.yaml and create a new entry in FAQs.md that mentions the "Error creating: pods "kong-pre-upgrade-migrations-" is forbidden: error looking up service account" message users will encounter. This FAQ entry would replace the proposed "Service Accounts" section in README.md, with the intent that README.md has instructions for standard operation and FAQs.md covers common failure cases.

In the FAQ entry, we can include the existing temp user block and instruct users how to add it temporarily. They'll need to pull down a local copy of the chart, append the block to migrations-pre-upgrade.yaml, and upgrade using that temporary modified local copy. After the initial upgrade, they should revert back to using kong/kong or their regular local copy for all subsequent upgrades. This is cumbersome, but again, hopefully infrequent.

Revisiting some of my prior comments, I think that's still better than either of the other alternatives. Using a temporary user name is possible, but users presumably want their PSP to apply to migrations Jobs going forward after they enable it, and a temporary SA complicates that. We could also add an additional option in values.yaml for creating a temporary SA and instruct users to only use it in the one case, but I think that'd only be warranted if we expected this to happen often.

@rainest rainest changed the base branch from master to next February 1, 2020 01:46
@yasn77 yasn77 force-pushed the fix_serviceaccount_deletion branch from 9cb453a to 5cb7b02 Compare February 2, 2020 21:44
@yasn77
Copy link
Contributor Author

yasn77 commented Feb 2, 2020

Revisiting some of my prior comments, I think that's still better than either of the other alternatives. Using a temporary user name is possible, but users presumably want their PSP to apply to migrations Jobs going forward after they enable it, and a temporary SA complicates that. We could also add an additional option in values.yaml for creating a temporary SA and instruct users to only use it in the one case, but I think that'd only be warranted if we expected this to happen often.

@rainest I agree. It is probably better to leave this as documentation rather than get the code to do it for the user. In most cases, if a user is aware of the issue, it isn't a big issue to create a service account and use that

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Diff to move instructions to FAQ with example error and instructions for temporary modifications (along with some other minor fixes) at https://gist.github.com/rainest/0c2c918decc08d785e19467e5ca465a5

@yasn77 could you apply that?

Helm upgrade recreates service account, which in turn recreates the secret token
for the service account. This breaks current deployments. Below are symptoms:

Before Upgrade:
--------------

```
kubectl -n kong get serviceaccount -l app.kubernetes.io/name=kong-app -o jsonpath='{.items[].secrets}'
[map[name:kong-tst-kong-app-token-k7qv2]]
```

```
kubectl -n kong get pod kong-tst-kong-app-cc9787b65-fwzrd -o json | jq '.spec.containers[] | select(.name == "ingress-controller").volumeMounts'
[
  {
    "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
    "name": "kong-tst-kong-app-token-k7qv2",
    "readOnly": true
  }
]
```

(Note: Token is the same)

After Upgrade:
-------------

```
kubectl -n kong get serviceaccount -l app.kubernetes.io/name=kong-app -o jsonpath='{.items[].secrets}'
[map[name:kong-tst-kong-app-token-rrzfh]]
```

```
kubectl -n kong get pod kong-tst-kong-app-cc9787b65-fwzrd -o json | jq '.spec.containers[] | select(.name == "ingress-controller").volumeMounts'
[
  {
    "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount",
    "name": "kong-tst-kong-app-token-k7qv2",
    "readOnly": true
  }
]
```

(Note: Tokens are different)

From the logs (after upgrade):

Tiller:
------
```
[tiller] 2020/01/31 17:01:46 deleting pre-upgrade hook kong-tst-kong-app for release kong-tst due to "before-hook-creation" policy
[kube] 2020/01/31 17:01:46 Starting delete for "kong-tst-kong-app" ServiceAccount
[kube] 2020/01/31 17:01:46 Waiting for 60 seconds for delete to be completed
[kube] 2020/01/31 17:01:48 building resources from manifest
[kube] 2020/01/31 17:01:48 creating 1 resource(s)
[kube] 2020/01/31 17:01:48 Watching for changes to ServiceAccount kong-tst-kong-app with timeout of 5m0s
[kube] 2020/01/31 17:01:48 Add/Modify event for kong-tst-kong-app: ADDED
```

From IngresController:
----------------------
```
E0131 18:09:30.368424       1 reflector.go:125] pkg/mod/k8s.io/client-go@v0.0.0-20190819141724-e14f31a72a77/tools/cache/reflector.go:98: Failed to list *v1beta1.Ingress: Unauthorized
E0131 18:09:31.206518       1 reflector.go:125] pkg/mod/k8s.io/client-go@v0.0.0-20190819141724-e14f31a72a77/tools/cache/reflector.go:98: Failed to list *v1.KongCredential: Unauthorized
E0131 18:09:31.371432       1 reflector.go:125] pkg/mod/k8s.io/client-go@v0.0.0-20190819141724-e14f31a72a77/tools/cache/reflector.go:98: Failed to list *v1beta1.Ingress: Unauthorized
E0131 18:09:32.214002       1 reflector.go:125] pkg/mod/k8s.io/client-go@v0.0.0-20190819141724-e14f31a72a77/tools/cache/reflector.go:98: Failed to list *v1.KongCredential: Unauthorized
E0131 18:09:32.379662       1 reflector.go:125] pkg/mod/k8s.io/client-go@v0.0.0-20190819141724-e14f31a72a77/tools/cache/reflector.go:98: Failed to list *v1beta1.Ingress: Unauthorized
```

This is due to a pre hook that creates the service account and then deletes
(then is created again).

Stop this behaviour and document manual upgrades.
@yasn77 yasn77 force-pushed the fix_serviceaccount_deletion branch from e531f53 to 1a70365 Compare February 4, 2020 21:33
@yasn77
Copy link
Contributor Author

yasn77 commented Feb 4, 2020

Diff to move instructions to FAQ with example error and instructions for temporary modifications (along with some other minor fixes) at https://gist.github.com/rainest/0c2c918decc08d785e19467e5ca465a5

@yasn77 could you apply that?

Done 👍

@rainest rainest self-requested a review February 4, 2020 21:34
@rainest rainest merged commit a1d895b into Kong:next Feb 4, 2020
@yasn77 yasn77 deleted the fix_serviceaccount_deletion branch February 5, 2020 15:00
rainest pushed a commit that referenced this pull request Feb 11, 2020
Remove the temporary ServiceAccount created via hook during pre-upgrade
migrations. This was previously in place to automatically handle users
upgrading from a configuration that did not use a ServiceAccount to one that
did. However, this temporary account overwrites credentials for the real
account if left in the template.

Add instructions for temporarily modifying the template to re-add this hook to
the FAQ, along with examples of the error that will appear.
@hbagdi
Copy link
Member

hbagdi commented Apr 22, 2020

@yasn77 Thank you for your contribution. Please fill in the contributor form to claim your Kong swag: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants