-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fix jwks object path in S3 for IRSA #11649
Conversation
Hi @h3poteto. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
|
/cc @rifelpet |
If I remember my own reasoning correctly, it is to have a common prefix for both the |
Wouldn't the |
I agree. This is leftovers from when that VFS was intended to be shared for other things as well. |
@h3poteto i.e the block around line 125 in |
OK, I will try it. |
pkg/model/issuerdiscovery.go
Outdated
@@ -80,7 +80,7 @@ func (b *IssuerDiscoveryModelBuilder) Build(c *fi.ModelBuilderContext) error { | |||
keysFile := &fitasks.ManagedFile{ | |||
Contents: keys, | |||
Lifecycle: b.Lifecycle, | |||
Location: fi.String("/openid/v1/jwks"), | |||
Location: fi.String("oidc/openid/v1/jwks"), |
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.
@h3poteto please also change the .well-known
path below on line 93. Also, please remove the appending of "oidc" around line 125 in pkg/model/iam/subject.go
6d4feaa
to
dfe286c
Compare
280974a
to
a7a659d
Compare
@olemarkus @johngmyers How about this? |
1974dfe
to
3143907
Compare
/retest |
3143907
to
d30c350
Compare
util/pkg/vfs/s3fs.go
Outdated
@@ -447,7 +447,8 @@ func (p *S3Path) GetHTTPsUrl() (string, error) { | |||
} | |||
p.bucketDetails = bucketDetails | |||
} | |||
return fmt.Sprintf("https://%s.s3.%s.amazonaws.com/%s", p.bucketDetails.name, p.bucketDetails.region, p.Key()), nil | |||
baseURL := fmt.Sprintf("https://%s.s3.%s.amazonaws.com", p.bucketDetails.name, p.bucketDetails.region) | |||
return path.Join(baseURL, p.Key()), nil |
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.
fmt.Println(path.Join("https://example.com", "/path"))
returns https:/example.com/path
which has too few /
after the https:
.
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.
Perhaps use a URL
struct literal.
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.
Oh...sorry. I fixed it.
d52ec60
d30c350
to
361b02f
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.
Thank you.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers 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 |
…1649-upstream-release-1.21 Automated cherry pick of #11649: Fix issuer and jwks object path for IRSA
This is a small change.
jwks object path is wrong when I specify serviceAccountIssuerDiscovery according to https://kops.sigs.k8s.io/cluster_spec/#service-account-issuer-discovery-and-aws-iam-roles-for-service-accounts-irsa .
For example, when I specify following spec:
kube-apiserver's parameters are
But, there is no object in
https://h3poteto-playground-oidc.s3.us-east-1.amazonaws.com/oidc/openid/v1/jwks
. At the momen the jwks object is located in/openid/v1/jwks
. It should be/oidc/openid/v1/jwks
, so I fixed the path.