Skip to content

Conversation

mprahl
Copy link
Collaborator

@mprahl mprahl commented Sep 25, 2025

Description of your changes:

Checklist:

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mprahl. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

help="KFP pipeline server host (e.g., https://<host>). Defaults to the value of the KFP_SERVER_HOST environment variable.")
parser.add_argument("--token", default=os.getenv("KFP_BEARER_TOKEN"), help="Bearer token for authentication. Defaults to the value of the KFP_BEARER_TOKEN environment variable.")
parser.add_argument("--ca-bundle", default=os.getenv("CA_BUNDLE"), help="Path to custom CA bundle file. Defaults to the value of the CA_BUNDLE environment variable")
parser.add_argument("--insecure", "--skip-tls-verify", dest="skip_tls_verify", action="store_true",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we're using both --insecure and --skip-tls-verify flags here? Both seem to be achieving the same functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason in particular. We can pick one.

parser.add_argument("--kfp-server-host", default=os.getenv("KFP_SERVER_HOST"), required=True,
help="KFP pipeline server host (e.g., https://<host>). Defaults to the value of the KFP_SERVER_HOST environment variable.")
parser.add_argument("--token", default=os.getenv("KFP_BEARER_TOKEN"), help="Bearer token for authentication. Defaults to the value of the KFP_BEARER_TOKEN environment variable.")
parser.add_argument("--ca-bundle", default=os.getenv("CA_BUNDLE"), help="Path to custom CA bundle file. Defaults to the value of the CA_BUNDLE environment variable")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code allows --ca-bundle and --insecure to be used together, aren't these supposed to be mutually exclusive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, --insecure takes priority in the code.


# Suppress urllib3 warnings when explicitly running with insecure TLS
if args.skip_tls_verify:
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a warning here so that the users are explicitly reminded they're running in insecure mode:

print("WARNING: TLS certificate verification is disabled. This connection is insecure!", file=sys.stderr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants