Fix HTTP credentials passing #11538
Merged
Fix HTTP credentials passing #11538
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac 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 |
openshift-ci
bot
added
the
approved
mtrmac
changed the title
|
NOTE: The comments in the pkg/auth package talk about the AuthHeader using multiple values and ConfigHeader using a map, but the actual code supports multiple header values the other way around. Which one is correct? I’ve been assuming it’s the code. |
mtrmac
force-pushed
the
http-credentials
branch
from
b362e5d to
6df6c60
Compare
openshift-ci
bot
added
the
needs-rebase
mtrmac
force-pushed
the
http-credentials
branch
from
6df6c60 to
9c65002
Compare
openshift-ci
bot
removed
the
needs-rebase
mtrmac
force-pushed
the
http-credentials
branch
from
9c65002 to
1e1fa05
Compare
openshift-ci
bot
added
the
needs-rebase
|
A friendly reminder that this PR had no activity for 30 days. |
|
@mtrmac any progress on this? |
mtrmac
force-pushed
the
http-credentials
branch
from
1e1fa05 to
09477b3
Compare
openshift-ci
bot
removed
the
needs-rebase
mtrmac
force-pushed
the
http-credentials
branch
from
09477b3 to
494a87f
Compare
|
OK, rebased and enough with the refactoring. The PR now contains a fairly extensive refactoring of
Some parts of the above, especially of the refactor, might not be ideal, or might be actively unwanted based on concerns I’m not aware of. Please don’t hesitate to say so, I’d be happy to split this into multiple PRs, drop parts entirely, or change in any other way requested. |
mtrmac
changed the title
mtrmac
force-pushed
the
http-credentials
branch
from
494a87f to
f51b8d8
Compare
openshift-ci
bot
added
the
needs-rebase
|
A friendly reminder that this PR had no activity for 30 days. |
mtrmac
force-pushed
the
http-credentials
branch
from
f51b8d8 to
c2cbb15
Compare
openshift-ci
bot
added
needs-rebase
mtrmac
force-pushed
the
http-credentials
branch
from
c2cbb15 to
7f05979
Compare
Almost every caller is using it only to wrap an error in exactly the same way, so move that error context into GetCredentials and simplify the users. (The one other caller, build, was even wrapping the error incorrectly talking about query parameters; so let it use the same text as the others.) Signed-off-by: Miloslav Trmač <[email protected]>
Don't create a single-element map only for the only caller to laboriously extract an element of that map; just return a single entry. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
In the "no input" case, return a constant instead of continuing with the decode/convert path, converting empty data. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
…uthHeader Both have a single caller, so there's no point in looking up the header value twice. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Use separate lines, and use the provided .String() API. Should not change behaivor. Signed-off-by: Miloslav Trmač <[email protected]>
It's possibly a bit more expensive, but semantically safer because it does header normalization. And we'll regain the cost by not looking up the value repeatedly. Signed-off-by: Miloslav Trmač <[email protected]>
... and have GetCredentials pass the values down to getConfigCredentials and getAuthCredentials. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
We'll share even more code here in the future. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This shares the code, and makes getConfigCredentials and getAuthCredentials side-effect free and possibly easier to test. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... which can be called independently. For now, there are no new callers, to test that the behavior has not changed. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
…Header) All callers hard-code a header value, so this is actually shorter. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... which can be called independently. For now, there are no new callers, to test that the behavior has not changed. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
All callers hard-code a header value, so this is actually shorter. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
It is no longer used. Split the existing tests into MakeXRegistryConfigHeader and MakeXRegistryAuthHeader variants. For now we don't modify the implementations at all, to make review simpler; cleanups will follow. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
which used to contain more context, but now are just a pointless copy. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Having a parameter that modifies the provides types.SystemContext seems rather unexpected and risky to have around - and the only user of that is actually a no-op; so, remove that option and simplify. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... now that two of the three cases are the same. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Having a parameter that modifies the provides types.SystemContext
seems rather unexpected and risky to have around - and the only
user of that is actually a no-op, others only provide a nil
SystemContext; so, remove that option and simplify (well, somewhat;
many callers now have extra &types.SystemContext{AuthFilePath}
boilerplate; at least that's consistent with that code carrying
a TODO to create a larger-scope SystemContext).
Should not change behavior.
Signed-off-by: Miloslav Trmač <[email protected]>
... now that they have no public users. Also remove the HeaderAuthName type, we don't need the type-safety so much for private constants, and using plain strings results in less visual noise. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
http-credentials
branch
from
93d7be0 to
5bbcfaf
Compare
openshift-ci
bot
removed
the
needs-rebase
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This primarily fixes the incorrect normalization added in #11430 .
That requires UNMERGED containers/image#1373Separately I abuse this PR for testing UNMERGED and mostly unrelated containers/common#763 .Also, this adds a lot of unit tests to the HTTP credential passing code, and proposes a fairly significant WIP refactoring.
Still outstanding:auth.Make…Header(at the very least don’t modify caller’sSystemContext)auth.MakeXRegistryAuthHeaderyet further into two, to emphasize the exclusive sets of parameters instead of pretending to handle both.See individual commit messages for details.