[serverless-1.33] fix: PaC build #783
Conversation
* Fixed detection of PaC controller URL for unprivileged users. * Use fn.Deploy.Image before fn.Image since the fn.Image may not be populated. Signed-off-by: Matej Vašek <matejvasek@gmail.com>
matejvasek
requested review from
matzew,
lkingland and
jrangelramos
and removed request for
vyasgun
|
PTAL @dsimansk @lkingland @gauron99 |
| @@ -36,7 +36,7 @@ func DetectPACInstallation(ctx context.Context, wantedNamespace string) (bool, s | |||
| return false, "", err | |||
| } | |||
|
|
|||
| _, err = clientPac.Repositories("").List(ctx, metav1.ListOptions{}) | |||
| _, err = clientPac.Repositories(cns).List(ctx, metav1.ListOptions{}) | |||
There was a problem hiding this comment.
rationale: unprivileged user most likely cannot scan all the namespaces, but they most likely can scan current namespace.
| @@ -154,7 +154,12 @@ func (pp *PipelinesProvider) createClusterPACResources(ctx context.Context, f fn | |||
| labels = pp.decorator.UpdateLabels(f, labels) | |||
| } | |||
|
|
|||
| registry, err := docker.GetRegistry(f.Image) | |||
| img := f.Deploy.Image | |||
There was a problem hiding this comment.
rationale: f.Image appears to be empty , f.Deploy.Image appears to contain the desired value instead.
There was a problem hiding this comment.
This caused error for all users not only unprivileged ones!
|
I added explanatory comments as to why each change is needed. |
|
/lgtm |
|
One caveat: this patch assumes that PaC is installed in |
Signed-off-by: Matej Vašek <mvasek@redhat.com>
|
this looks reasonable. /lgtm Are there some tests for this? Or do we have some other e2e tests /cc @jrangelramos |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jrangelramos, matejvasek, matzew 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 |
|
/override "On Cluster Test (ubuntu-latest)" |
|
@matejvasek: Overrode contexts on behalf of matejvasek: On Cluster Test (ubuntu-latest) In response to this:
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-sigs/prow repository. |
|
/override "On Cluster RT Test (ubuntu-latest, s2i)" |
|
@matejvasek: Overrode contexts on behalf of matejvasek: On Cluster RT Test (ubuntu-latest, s2i) In response to this:
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-sigs/prow repository. |
openshift-merge-bot
bot
merged commit aa90e5e
into
serverless-1.33
fn.Deploy.Imagebeforefn.Imagesince thefn.Imagemay not be populated.