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

gather rendered assets as part of install-gather #1646

Merged
merged 1 commit into from
Apr 26, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 17, 2019

I've got a broken pull openshift/machine-config-operator#626, but in general, any future asset rendering error requires the rendered assets to make a determination of the failure. This gathers those.

@sdodson
/assign @vrutkovs

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 17, 2019
@deads2k deads2k force-pushed the gather-01-rendered branch 2 times, most recently from e9cf296 to e15ef23 Compare April 17, 2019 16:25
Copy link
Member

@vrutkovs vrutkovs left a comment

Choose a reason for hiding this comment

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

/lgtm

cc wking abhinavdahiya

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 17, 2019

/retest

@@ -25,6 +25,10 @@ do
sudo podman inspect "${container}" >& "${ARTIFACTS}/bootstrap/pods/${container}.inspect"
done

echo "Gathering rendered assets..."
mkdir -p "${ARTIFACTS}/rendered-assets"
cp -r /etc/kubernetes/ "${ARTIFACTS}/rendered-assets"
Copy link
Member

Choose a reason for hiding this comment

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

This feels like it might gather secrets that we don't want gathered by default. Do you have a list handy of what this is pulling in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like it might gather secrets that we don't want gathered by default. Do you have a list handy of what this is pulling in?

@sferich888 Do we filter sensitive information before or after we gather? If before, any advice for @vrutkovs? This is beyond my bash abilities

Copy link
Member

Choose a reason for hiding this comment

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

In vsphere CI we filter out sensitive info in teardown container. If all filenames with sensitive data is known it should be removed from /tmp/artifacts before tar cx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In vsphere CI we filter out sensitive info in teardown container. If all filenames with sensitive data is known it should be removed from /tmp/artifacts before tar cx

I thought so, but let's see what eric says.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the cloud creds are the only thing the installer asks for that are not generated.

So..... only scrub that?

@deads2k
Copy link
Contributor Author

deads2k commented Apr 17, 2019

/hold

to sort out secret eliding and whether the storage location works on failed renders

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 17, 2019

@wking I spoke with Eric. The only truly confidential information we have are the cloud creds and image pull secrets. What files are they in so I can delete them. The rest of the certs are created for each install.

@wking
Copy link
Member

wking commented Apr 17, 2019

The only truly confidential information we have are the cloud creds and image pull secrets. What files are they in so I can delete them.

This is what goes up:

$ jq -r '.storage.files[].path' /tmp/we/bootstrap.ign | grep 'secret\|docker'
/root/.docker/config.json
/opt/openshift/manifests/kube-system-secret-etcd-client-ca-deprecated.yaml
/opt/openshift/manifests/kube-system-secret-etcd-client.yaml
/opt/openshift/manifests/kube-system-secret-etcd-signer-client.yaml
/opt/openshift/manifests/kube-system-secret-etcd-signer.yaml
/opt/openshift/manifests/machine-config-server-tls-secret.yaml
/opt/openshift/manifests/openshift-config-secret-etcd-metric-client.yaml
/opt/openshift/manifests/openshift-config-secret-pull-secret.yaml
/opt/openshift/openshift/99_cloud-creds-secret.yaml
/opt/openshift/openshift/99_kubeadmin-password-secret.yaml
/opt/openshift/openshift/99_role-cloud-creds-secret-reader.yaml
/opt/openshift/openshift/99_openshift-cluster-api_master-user-data-secret.yaml
/opt/openshift/openshift/99_openshift-cluster-api_worker-user-data-secret.yaml

On the bootstrap machine we copy some thing around, e.g. moving the admin kubeconfig under /etc/kubernetes. But I don't see us moving either the cloud creds or pull secret under /etc/kubernetes.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 17, 2019

are we generally happy with removing the kubeconfig and then find . -name "*secret*" | xargs rm ?

@sferich888
Copy link
Contributor

Drop /root/.docker/config.json as well, but given the list @wking provided, searching the way you are should be ok.

Why don't we review an archive of what this collects currently to confirm that this is ok? Who/Where can I get one of these? CI?

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 18, 2019

now with secret eliding

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 18, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 18, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2019
@trown
Copy link

trown commented Apr 18, 2019

/test e2e-openstack

@trown
Copy link

trown commented Apr 18, 2019

/test e2e-openstack

@wking
Copy link
Member

wking commented Apr 26, 2019

/test e2e-aws-upgrade

@abhinavdahiya
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2019
@abhinavdahiya
Copy link
Contributor

@wking also had some comments, will let him lgtm

@wking
Copy link
Member

wking commented Apr 26, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, deads2k, vrutkovs, wking

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:
  • OWNERS [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c87b389 into openshift:master Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.