Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Sep 5, 2018

Catching up with openshift/installer#208.

I've replaced the inputs.yaml copy with a sed invocation to avoid leaking the internal pull secret into the output artifacts. I'm not sure how sensitive it is though, maybe we don't mind leaking the secret?

/hold

We don't want to merge this until the installer PR lands.

@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 Sep 5, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wking
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 5, 2018
Catching up with openshift/installer@8a84eb2e (*: Replace
PullSecretPath with PullSecret, 2018-09-05, openshift/installer#208).

The jq call ensures the secret JSON is on a single line, otherwise
we'd need to be a bit more careful about injecting it into the YAML as
a valid string literal.

I've replaced the inputs.yaml copy with a sed invocation to avoid
leaking the internal pull secret into the output artifacts.  I'm not
sure how sensitive it is though, maybe we don't mind leaking the
secret?
@wking wking force-pushed the pull-secret-string branch from fa61f1b to 4b5af22 Compare September 5, 2018 19:23
@stevekuznetsov
Copy link
Contributor

/uncc

@openshift-ci-robot openshift-ci-robot removed the request for review from stevekuznetsov September 5, 2018 22:14
@wking
Copy link
Member Author

wking commented Sep 6, 2018

openshift/installer#208 landed, so we're good to go here.

/hold cancel

@stevekuznetsov, @smarterclayton, can one of take a look?

@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 Sep 6, 2018
@wking
Copy link
Member Author

wking commented Sep 6, 2018

Maybe I should drop the sed, because we're already leaking the pull secret over here. It will be nice when we can pull all our images anonymously. How close to that are we?

@smarterclayton
Copy link
Contributor

The secret isn't that secure, but it would be better to accept it as a file.

Images in 4.0 will continue to be protected, and the ci infra will likely start using that mechanism as well to ensure it is always functioning.

@wking
Copy link
Member Author

wking commented Sep 7, 2018

The secret isn't that secure, but it would be better to accept it as a file.

Thoughts on openshift/installer#205 then? Don't set the pull secret there at all?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2018
@openshift-bot
Copy link
Contributor

@wking: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking
Copy link
Member Author

wking commented Oct 4, 2018

This is obsolete since #1677. While that is still inserting the pull secret as a path, openshift_install is still immediately reading it into a string variable. We're probably still leaking that secret in the CI logs (e.g. somewhere in here), but if that doesn't bother anyone in the release team, that's fine with me ;).

@wking wking closed this Oct 4, 2018
@wking wking deleted the pull-secret-string branch October 4, 2018 05:08
derekhiggins pushed a commit to derekhiggins/release that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants