-
Notifications
You must be signed in to change notification settings - Fork 244
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
Copy kubeconfig to temporary file and add permissions #3462
Copy kubeconfig to temporary file and add permissions #3462
Conversation
893b1d6
to
6a0e9c1
Compare
6a0e9c1
to
30f6bc8
Compare
# Copy kubeconfig to temporary kubeconfig file | ||
# Read, Write and Execute permission to temporary kubeconfig file | ||
mkdir -p ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig | ||
cp ${DEFAULT_INSTALLER_ASSETS_DIR}/auth/kubeconfig ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't you delete the /auth/kubeconfig? wouldnt it be confusing to have two kubeconfigs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't you delete the /auth/kubeconfig? wouldnt it be confusing to have two kubeconfigs?
This is original default kubeconfig provided by the installer. Due to write permission on the file in multistage test infra i need to copy that file to some location along with write permission. Then the new kubeconfig path will be updated through the env var KUBECONFIG.
Even i had the same thought to removing it, but as per my understanding though deleting the original kubeconfig brings no harm to the cluster setup but may be this original fresh kubeconfig can be used for some other purposes.
Moreover i would say lets not delete anything which is default to the cluster, otherwise it may brings some unknown issues which i don't know atm.
# Read, Write and Execute permission to temporary kubeconfig file | ||
mkdir -p ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig | ||
cp ${DEFAULT_INSTALLER_ASSETS_DIR}/auth/kubeconfig ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig | ||
chmod 764 ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 764?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I gave unnecessary write permission to the file. Read and write permission would be enough for kubeconfig file (666 ?). Anyway let me check what is the default permission of kubeconfig file used in template based test infra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ ls -l
total 32
-rw-r--r--@ 1 amit staff 23 Jun 14 20:18 kubeadmin-password
-rw-r--r--@ 1 amit staff 12193 Jun 14 20:18 kubeconfig
So it would be 644
. Try this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @amitkrout I will add 644 permission for the file 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prietyc123 Please check it locally thoroughly, otherwise it will break the master. Update the doc if needed while following the doc for setting up local environment for running test on 4.* cluster.
@amitkrout +1, I have run
|
cp ${DEFAULT_INSTALLER_ASSETS_DIR}/auth/kubeconfig ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig | ||
chmod 764 ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig | ||
chmod 644 ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp/kubeconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see how it goes.
I still doubt this will pass though as you might need to give write access to the group as openshift uses randomuid and hence permission on gid will be more important. This also means group owner of the file will have to be root.
Again let's wait for CI before deciding
@@ -13,8 +13,13 @@ KUBEADMIN_PASSWORD_FILE=${KUBEADMIN_PASSWORD_FILE:-"${DEFAULT_INSTALLER_ASSETS_D | |||
# Default values | |||
OC_STABLE_LOGIN="false" | |||
CI_OPERATOR_HUB_PROJECT="ci-operator-hub-project" | |||
# Copy kubeconfig to temporary kubeconfig file | |||
# Read, Write and Execute permission to temporary kubeconfig file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the message
7b7497a
to
ba75ac3
Compare
@@ -13,8 +13,13 @@ KUBEADMIN_PASSWORD_FILE=${KUBEADMIN_PASSWORD_FILE:-"${DEFAULT_INSTALLER_ASSETS_D | |||
# Default values | |||
OC_STABLE_LOGIN="false" | |||
CI_OPERATOR_HUB_PROJECT="ci-operator-hub-project" | |||
# Copy kubeconfig to temporary kubeconfig file | |||
# Read and Write permission to temporary kubeconfig file | |||
mkdir -p ${DEFAULT_INSTALLER_ASSETS_DIR}/tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of tmp can we name the dir what it stand for. For example kubeconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with tmp because its not the original kubeconfig
, someone might confuse with this. As we are copying the kubeconfig
and using it for the particular test, IMO keeping this under tmp
make more sense than in dir like kubeconfig
.
Codecov Report
@@ Coverage Diff @@
## master #3462 +/- ##
==========================================
- Coverage 46.48% 46.45% -0.04%
==========================================
Files 112 112
Lines 11223 11237 +14
==========================================
+ Hits 5217 5220 +3
- Misses 5505 5513 +8
- Partials 501 504 +3
Continue to review full report at Codecov.
|
@prietyc123 Your changes won't solve the purpose. Hang on. |
@prietyc123 Your change does not work in CI because kubeconfig is not located in the path that you are copying from (local setup). You need not to worry about the local copy of kubconfig as it is provided by the cluster itself I mean you just need to copy the kubeconfig and kube-password file in the auth dir to configure and run odo locally. Your current change hitting the issue of
In multi stage test infra default multistage test infra default
Template based test infra default
So you just need to add a else condition here https://github.com/openshift/odo/blob/master/scripts/configure-installer-tests-cluster.sh#L40
|
Ahh... I missed the kubeconfig mounted on secret part. Yes, we need to replace it in the way you suggested. However I will be inclined to give permission as |
Instead of assuming we can check what level of permission
|
According to templated based test I will be using |
b5165ba
to
7e6f328
Compare
7e6f328
to
95cb375
Compare
/hold cancel |
@prietyc123 I am able to reproduce the failure https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/3462/pull-ci-openshift-odo-master-v4.4-integration-e2e/1278975901698101248#1:build-log.txt%3A489 locally too. I am not sure why Steps to reproduce
|
or
The login step is impossible in prow CI because there is no such environment variable that exposes the kubeadmin password or password file. It works locally
|
@prietyc123 This can be handled through the login test script itself. Just need to add an developer login in the teardown step here in https://github.com/openshift/odo/blob/master/tests/integration/loginlogout/cmd_login_logout_test.go#L41 For example:
|
export KUBECONFIG=$TMP_DIR/kubeconfig | ||
|
||
# Login as admin user | ||
oc login -u $KUBEADMIN_USER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it. It prompts for password.
83bd3b6
to
50771bd
Compare
/retest |
Ok let me try these steps |
/retest |
50771bd
to
8e30f14
Compare
/retest |
prow failure /retest |
/retest |
1 similar comment
/retest |
@girishramnani It seems after your last change on watch test file i am hitting this issue https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/3462/pull-ci-openshift-odo-master-v4.3-integration-e2e/1280067878812389376#1:build-log.txt%3A632 in my pr which even does not touch the watch test. Can you please check the reason for failure. Its hard for me to debug the failure. |
/retest |
/lgtm |
/retest |
CI is passing. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amitkrout The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Copy kubeconfig to temporary file and grant the permissions to read write execute * Limiting the file permission to read write only * Updating copy kubeconfig to temp dir on the basis of review comment * Fixing login kubeconfig issue
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
This pr gives read write permission for kubeconfig file which is blocking us for multi-stage testing. Detailed explanation in comment
Which issue(s) this PR fixes:
Fixes NA
How to test changes / Special notes to the reviewer:
Copying kubeconfig to tmp file and accessing it should work with template based test too.