-
Notifications
You must be signed in to change notification settings - Fork 297
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
Fixes #36827 - Tokens for container registries now return OAuth2-compatible headers #10768
Conversation
Note: This shouldn't be merged until the complimentary capsule changes are approved (SAT-20757, PR to follow). |
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.
Need to test it still, but I have a couple comments for now:
app/controllers/katello/api/registry/registry_proxies_controller.rb
Outdated
Show resolved
Hide resolved
app/controllers/katello/api/registry/registry_proxies_controller.rb
Outdated
Show resolved
Hide resolved
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.
Looking great from a test perspective for auth'd users:
{"token":"$...GUS","expires_in":359,"issued_at":"2023-10-27T18:37:19+00:00"}
Once those comments are resolved this should be good to go.
551529e
to
9c2ea54
Compare
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.
Looking good to me! Let's just wait to merge until Katello/smart_proxy_container_gateway#27 is ready.
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.
Consider n-1 support where, at least in downstream, we support running Foreman version n with a Foreman Proxy version n-1. Because of release cycles that comes down to n-2 in upstream.
When I see Katello/smart_proxy_container_gateway#27 then I think removing the expires_at
field here means you break n-1 compatibility proxies with smart_proxy_container_gateway
.
I think the relevant spec (which you helpfully linked, greatly appreciated!) part is https://distribution.github.io/distribution/spec/auth/token/#requesting-a-token and I suspect (though it's not explicitly mentioned) that additional fields are silently ignored. My recommendation would be to keep expires_at
for now with a comment that it's outside the spec, but kept for backwards compatibility.
app/controllers/katello/api/registry/registry_proxies_controller.rb
Outdated
Show resolved
Hide resolved
@ekohl that's a great point, we need to make sure n-1 smart proxy support is still supported. That means a smart proxy running the older version of container gateway should be able to work with this new code. I agree that the best way to deal with this would be to keep the expires_at field around. |
I just pushed a new version which is confirmed working with n-1 smart proxies. Will update testing steps momentarily. Thanks for all the good feedback! |
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.
Your commit message is very long. I'd recommend reading https://cbea.ms/git-commit/#limit-50 (actually the whole post is full of good ideas)
app/controllers/katello/api/registry/registry_proxies_controller.rb
Outdated
Show resolved
Hide resolved
I can fix this up on the next rebase + squash. Is the standard practice to continually touch up the original commit message instead of concatenating messages for new commits? |
…atible headers - 'expires_at' has been depricated and is slated for removal in 4.13. We're keeping it for now to maintain smart proxy upgrade compatibility. - 'expires_at' is now 'expires_in'. This represents second count after issued_at time when the token expires. - All token times are now rendered in rfc3339 format.
Yes it is. The commit message is what shows up in |
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.
Looking good to me (again) !
{"token":"$2a$10$1b6453892473a467d0737uawQSD8Z6HKUQo.p3cKq3H5eWXFf2GUS","expires_in":532332,"issued_at":"2023-10-27T18:35:11+00:00","expires_at":"2023-11-02T22:27:24+00:00"}
What are the changes introduced in this pull request?
When podman commands request tokens from Katello, the tokens now return the correct specification. In particular:
Considerations taken when implementing this change?
n/a
What are the testing steps for this pull request?
podman search <server>/<docker repo name>
to get the repo address.podman pull <repo address>
to ensure everything is working properly. There should be no pull errors. If your repo doesn't have a 'latest' version, you may need to append:<version>
to the command.app/controllers/katello/api/registry/registry_proxies_controller.rb
and enterrequire 'byebug'; byebug
on line 152 (before response.headers).podman pull <repo address>
. Head to the rails console. Byebug should have stopped execution.expiration_seconds
to view the return value of theexpires_in
feld. This should be around 60 seconds (could be 59ish).issued_at
field. This should be an RFC3339-formatted datetime.podman pull <repo address>
and ensure that this fails due to not being authenticated.podman login <server>
to login.podman pull <repo address>
to ensure everything is working properly.podman pull <repo address>
. In the rails console, verify the same two fields on the token.expiration_seconds
should now be around 6 minutes, but you can still expect an RFC3339 datetime fromissue_time
.podman search <server>/<docker repo name>
to get the repo address.podman pull <repo address>
. Follow this up with a podman run command to test the container if you wish.podman pull <repo address>
on the smart proxy and ensure that this fails due to not being authenticated.podman login <proxy>
to login.podman pull <repo address>
to ensure everything is working properly.