[Deployment] Add secure=false explicitly in manifests for better observability

[Bobgy]

/cc @discordianfish
This reverts the revert for #3168

---

This change is 

[Bobgy]

/approve
for standalone manifest

[Bobgy]

/assign @rmgogogo @jingzhang36
for approval

[discordianfish]

/lgtm

[Bobgy]

Updated GCP managed store setting, because we run a minio proxy for connecting to the real gcs bucket, secure should be set to false in this case I believe.

[Bobgy]

/retest
image failed building

[Bobgy]

/retest

[discordianfish]

/lgtm

[Bobgy]

Hi @discordianfish, I discussed with @rmgogogo who had some disagreement with this change.
Our agreement is that:

1. the main issue you raised was users using secure=false without awareness when connecting to 3rd party services directly like s3 or gcs. Therefore, my understanding of the goal for this PR is to increase observability of this secure configuration. Adding it to all manifests achieves this goal, rather than changing the server's default value.
2. prefer not doing unnecessary (explained why unnecessary in 1.) backward compatible change

So I updated the change to only change manifests. What do you think?

[discordianfish]

@Bobgy My goal was to make pipelines not use insecure defaults. I'd strongly suggest changing the default, even if it means breaking backward compatibility (which is something you can never prevent). In fact, many people for whom this breaks might actually be at risk right now due to the insecure transport. In our case we only realize this because we had a TLS enforcement policy on our bucket. Without that, we would have sent PII unencrypted over the wire.

[Bobgy]

@Bobgy My goal was to make pipelines not use insecure defaults. I'd strongly suggest changing the default, even if it means breaking backward compatibility (which is something you can never prevent). In fact, many people for whom this breaks might actually be at risk right now due to the insecure transport. In our case we only realize this because we had a TLS enforcement policy on our bucket. Without that, we would have sent PII unencrypted over the wire.

Thanks! I see your goal now.
Still not fully sure if your previous change helps that situation, I believe when users upgrade KFP, they should upgrade manifests and binary at the same time. Therefore, they cannot realize they accidentally used secure=false.
If they are following best practice of forking, reviewing and merging KFP manifests updates, they will find out this suspicious field as long as we update manifests.

[discordianfish]

Still not fully sure if your previous change helps that situation, I believe when users upgrade KFP, they should upgrade manifests and binary at the same time. Therefore, they cannot realize they accidentally used secure=false.

They should but there is nothing that would ensure that.

If they are following best practice of forking, reviewing and merging KFP manifests updates, they will find out this suspicious field as long as we update manifests.

It's better than nothing and would have prevented me from (trying to) running it without encryption, but it's still best practice to ship software with secure defaults. At the end of the day, the apiserver is provided as it's own binary/image, so by default it's insecure. That's a problem.

What exactly is your concern of changing the defaults though? I know it's a breaking change but:

1. As you said, if people following the best practice of updating their manifests with the apiserver, they won't have a problem
2. If they don't update, it might break - but only because they were using an insecure setup before

[Bobgy]

If they are following best practice of forking, reviewing and merging KFP manifests updates, they will find out this suspicious field as long as we update manifests.

It's better than nothing and would have prevented me from (trying to) running it without encryption, but it's still best practice to ship software with secure defaults. At the end of the day, the apiserver is provided as it's own binary/image, so by default it's insecure. That's a problem.

I wonder how did you learn to set up the manifest using this binary. Binary itself has never been a supported path to deploy KFP. All of our public docs refer to manifests.

What exactly is your concern of changing the defaults though? I know it's a breaking change but:

1. As you said, if people following the best practice of updating their manifests with the apiserver, they won't have a problem
2. If they don't update, it might break - but only because they were using an insecure setup before

The only concern is to avoid unnecessary backward-incompatible change. Personally, initially I'm okay with both, but @rmgogogo had a strong opinion against this.
/cc @IronPan @rmgogogo
I think we should solve the real problem of manifest/documentation, rather than a default value change that breaks people upgrading binary without manifest change (both those who should use true and who shouldn't use true).

[discordianfish]

Shipping something that defaults to http for something that could include PII is IMO putting users at risk. I take any bet that there are users today using AWS buckets without encryption because of this. I'm fine to agree to disagree on this, but then somebody else should give their lgtm.

[Bobgy]

Shipping something that defaults to http for something that could include PII is IMO putting users at risk. I take any bet that there are users today using AWS buckets without encryption because of this. I'm fine to agree to disagree on this, but then somebody else should give their lgtm.

@rmgogogo do you change your mind reading the discussion here?

@discordianfish I wonder how did you learn to set up the manifest using this binary. I think we should add something in that learning path?

Current commitment is we ship manifest and binary which works out of box for GCP without external services.
3rd party contributes to aws manifests in https://github.com/e2fyi/kubeflow-aws/blob/master/pipelines/base/manifest/ml-pipeline-apiserver-configmap.yaml#L21 for standalone and in Kubeflow aws manifest. Those are places we should fix IMO.

[discordianfish]

@discordianfish I wonder how did you learn to set up the manifest using this binary. I think we should add something in that learning path?

I've used the official kubeflow manifests which didn't mention the 'secure' option, so yeah adding it explicitly is a step forward either way.

[Bobgy]

Thanks for understanding

[rmgogogo]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, rmgogogo

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:

• manifests/gcp_marketplace/OWNERS [rmgogogo]
• manifests/kustomize/OWNERS [Bobgy,rmgogogo]

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

[rmgogogo]

/lgtm

[Bobgy]

/retest

[Bobgy]

/retest

[discordianfish]

Looks like we have the same issue in the frontend server, which also defaults to ssl=false: https://github.com/kubeflow/pipelines/blob/26f51decb83fa70b635b5f366367c7ada56509e7/frontend/server/configs.ts#L57

In that case though, the gcp manifests don't seem to enable TLS: https://github.com/kubeflow/manifests/blob/26f51decb83fa70b635b5f366367c7ada56509e7/pipeline/pipelines-ui/overlays/gcp/deployment.yaml

That's a good real life example why I don't think these services should default to non-tls.

[discordianfish]

Not so sure about this anymore, it seems to use different config structs for gcp and aws which don't specify that, so it falls back to minio's secure defaults.