Skip to content

Conversation

sebrandon1
Copy link
Member

Related to: openshift/enhancements#1773

This is my first attempt at a FeatureGate so I'm expecting this to need a bunch of work.

Additions to API Specification and Validation:

  • Added HTTP01ChallengeProxy configuration to the APIServerSpec struct, allowing users to enable and configure the HTTP01 challenge proxy for API endpoint certificates. This includes options for DefaultDeployment and CustomDeployment modes. (config/v1/types_apiserver.go, [1] [2]

Feature Gate Registration:

  • Registered the new HTTP01ChallengeProxy feature gate in features.go, enabling it in the TechPreviewNoUpgrade scope and providing metadata such as a contact person and enhancement proposal link. (features/features.go, features/features.goR869-R876)

Test Cases for Validation:

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 1, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 1, 2025

@sebrandon1: This pull request references CNF-13731 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set.

In response to this:

Related to: openshift/enhancements#1773

This is my first attempt at a FeatureGate so I'm expecting this to need a bunch of work.

Additions to API Specification and Validation:

  • Added HTTP01ChallengeProxy configuration to the APIServerSpec struct, allowing users to enable and configure the HTTP01 challenge proxy for API endpoint certificates. This includes options for DefaultDeployment and CustomDeployment modes. (config/v1/types_apiserver.go, [1] [2]

Feature Gate Registration:

  • Registered the new HTTP01ChallengeProxy feature gate in features.go, enabling it in the TechPreviewNoUpgrade scope and providing metadata such as a contact person and enhancement proposal link. (features/features.go, features/features.goR869-R876)

Test Cases for Validation:

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Aug 1, 2025

Hello @sebrandon1! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 1, 2025
Copy link
Contributor

openshift-ci bot commented Aug 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sebrandon1
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the 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

@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2025
@sebrandon1 sebrandon1 force-pushed the feature_gate branch 5 times, most recently from be55466 to cb15245 Compare August 4, 2025 22:19
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2025
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

I'm surprised to see so many protobuf changes here. Something seems off with this many changes being made.

@JoelSpeed is probably more familiar with what might be going on here than I.

Because you are making changes to a CRD, you should be able to run PROTO_OPTIONAL=true make update to make generation updates without touching the protobuf stuff

@JoelSpeed
Copy link
Contributor

Proto changes appear to be unrelated, verify thinks changing them is wrong and is suggesting to change them back

Possibly running with a newer or incompatible version?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2025
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 6, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2025
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 6, 2025
@sebrandon1 sebrandon1 force-pushed the feature_gate branch 2 times, most recently from 0b955f2 to a5d3810 Compare August 7, 2025 16:07
@sebrandon1
Copy link
Member Author

/retest

@JoelSpeed
Copy link
Contributor

/test lint

@sebrandon1 sebrandon1 force-pushed the feature_gate branch 3 times, most recently from ebd75c8 to e9136e8 Compare August 11, 2025 22:28
@sebrandon1
Copy link
Member Author

/retest

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Once the linter and test issues are fixed, LGTM

@JoelSpeed
Copy link
Contributor

API types look good, but it looks like some of the test cases are expecting things that aren't true (optional port), so lets get those fixed up and please squash the various fixups

@sebrandon1 sebrandon1 force-pushed the feature_gate branch 5 times, most recently from d6a8c58 to 654300a Compare August 26, 2025 20:13
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Leaving some comments based on my analysis of the CI failures

@everettraven
Copy link
Contributor

False positive on new required field with optional parent.

/override ci/prow/verify-crd-schema

Copy link
Contributor

openshift-ci bot commented Aug 27, 2025

@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema

In response to this:

False positive on new required field with optional parent.

/override ci/prow/verify-crd-schema

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.

@sebrandon1 sebrandon1 force-pushed the feature_gate branch 3 times, most recently from b67caf9 to f30ea16 Compare August 28, 2025 21:12
Fix HTTP01ChallengeProxy integration test structure and enable feature gate in TechPreviewNoUpgrade

- Fixed test file structure to include required crdName metadata for integration tests
- Enabled HTTP01ChallengeProxy feature gate in both DevPreviewNoUpgrade and TechPreviewNoUpgrade
- Regenerated feature gate manifests via make update
- Resolved CI failure: missing required field crdName in test spec

Make internalPort optional to resolve API compatibility error

- Changed internalPort from required to optional (*int32 with omitempty)
- This resolves the NoNewRequiredFields API compatibility violation
- Updated test case to reflect optional field behavior
- Users can now omit internalPort for custom deployments
- Regenerated deepcopy functions and OpenAPI schemas

Address comments 1

Update codegen crds

Adjust back to required

Remove pointer

Address comments for linter
Copy link
Contributor

openshift-ci bot commented Aug 29, 2025

@sebrandon1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-e2e-aws-ovn 34d645a link false /test okd-scos-e2e-aws-ovn
ci/prow/verify-crd-schema 34d645a link true /test verify-crd-schema
ci/prow/verify 34d645a link true /test verify

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants