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

[doc] 4.5.3.1.2 Update the Authentication Connector - do not use kubectl edit #671

Closed
Martin-Weiss opened this issue Jan 17, 2020 · 8 comments
Assignees
Labels
AdminGuide Fix will change the Admin Guide Enhancement Fix is an enhancement v4 CaaSP v4
Milestone

Comments

@Martin-Weiss
Copy link
Contributor

4.5.3.1.2 Update the Authentication Connector

https://susedoc.github.io/doc-caasp/master/caasp-admin/single-html/#_sec.admin.security.rbac.update


From a best practice point of view we should never use kubectl edit.

Could we change this to "adjust yaml and apply yaml"?

@r0ckarong
Copy link
Contributor

@cclhsu Why was kubectl chosen here? Can we simply replace this with changing the yaml file?

@r0ckarong r0ckarong added AdminGuide Fix will change the Admin Guide Enhancement Fix is an enhancement Reported v4 CaaSP v4 labels Jan 17, 2020
@innobead
Copy link
Contributor

This should be only for runtime change when @cclhsu worked on this documentation. However dex is part of addon, so it would be better to change the addon manifest then apply instead of runtime only by kubectl edit. So this turns out @Martin-Weiss question.

@cclhsu please help evaluate then change the doc if need. Thanks.

@cclhsu
Copy link
Contributor

cclhsu commented Feb 4, 2020

I believe we encountered one scenario 591 when addon upgrade where yaml in ~/clusters/<CLUSTER_NAME> is not consisted with resource in running cluster. In such cases, user required to download the existing oidc-dex-config ConfigMap before addon upgrade, then re-apply the downloaded oidc-dex-config ConfigMap after addon upgraded. Or just simply edit the config as current document does.

@Martin-Weiss
Copy link
Contributor Author

We should never recommend to use "edit" as this causes the deployments to be "non-immutable" and "non-reproducible" anymore.
But yes - there is a problem that the skuba addon yaml files get updated once we do a skuba addon upgrade. This is still an open point where engineering is working on (I believe the idea is to use kustomize in the future..)

@innobead
Copy link
Contributor

innobead commented Feb 4, 2020

Yes as @Martin-Weiss said, there is an on-going effort about kustomize to have merge strategy when upgrading new addons with skuba. (ref: SUSE/skuba#858)

Also understand @cclhsu said, but even we ask user to modify addon manifest, that is still not a source of truth, because it's just located in an operator's workstation w/ single copy and w/o any version control (ex: skuba does have gitops DNA yet).

So I would suggest to do below as a workaround for the current situation.

  1. for any addon manifest update, guide user update the manifest in cluster definition folder then apply.
  2. remind user before upgrading addons, need to backup the current ones at runtime, then restore w/ modification back after upgrading because of known addon update limitation (no kustomize integrated yet).

@Martin-Weiss
Copy link
Contributor Author

  1. ensure the gangway secret and the dex secret config match!

I have seen that for some reason the gangway secret changed and re-applying the old dex secret config will break authentication in case the secret in the configmap changed in the meantime..

@r0ckarong
Copy link
Contributor

Closed with #712

@r0ckarong r0ckarong added this to the Sprint 24 milestone Mar 4, 2020
@innobead
Copy link
Contributor

innobead commented Mar 4, 2020

JFYI, kustomize just getting merged in skuba master branch, so it means there will be some improvement in the future and doc updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AdminGuide Fix will change the Admin Guide Enhancement Fix is an enhancement v4 CaaSP v4
Projects
None yet
Development

No branches or pull requests

4 participants