search

openshift / api

Canonical location of the OpenShift API definition.
http://www.openshift.org
Apache License 2.0
94 stars 517 forks source link

CORS-3689: API changes in config/v1 to hold eipAllocations in cluster ingress object. #2043

Closed miheer closed 1 week ago

miheer commented 1 month ago

The eipAllocations when set by installer for default AWS NLB IngressController are set in the cluster ingress object the for the ingress operator to refer and then create a default Ingress Controller. This API field holds the eipAllocations value set by installer which were set via install-config.yaml

openshift-ci-robot commented 1 month ago

@miheer: This pull request references CORS-3689 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 story to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/api/pull/2043): >The eipAllocations when set by installer for default AWS NLB IngressController are set in the cluster ingress object the for the ingress operator to refer and then create a default Ingress Controller. This API field holds the eipAllocations value set by installer which were set via install-config.yaml Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 1 month ago

Hello @miheer! 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[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: miheer Once this PR has been reviewed and has the lgtm label, please assign spadgett for approval. 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: - **[OWNERS](https://github.com/openshift/api/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
miheer commented 1 month ago

@JoelSpeed @gcs278 PTAL

miheer commented 1 month ago

@JoelSpeed I have made the requested changes. PTAL.

openshift-ci[bot] commented 1 month ago

@miheer: 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/verify-crd-schema 5b311254a87995086f1affc9b298046d95104933 link true /test verify-crd-schema
ci/prow/e2e-aws-serial-techpreview 5b311254a87995086f1affc9b298046d95104933 link true /test e2e-aws-serial-techpreview
ci/prow/verify 5b311254a87995086f1affc9b298046d95104933 link true /test verify
ci/prow/e2e-aws-ovn 5b311254a87995086f1affc9b298046d95104933 link true /test e2e-aws-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
miheer commented 1 month ago

@JoelSpeed the test ci/prow/verify is failing with the following error: INSUFFICIENT CI testing for "InstallEIPForDefaultNLBIngressController". F1022 01:20:04.603312 172416 root.go:64] Error running codegen: error: "install should succeed: overall" only passed 94%, need at least 95% for "InstallEIPForDefaultNLBIngressController" on {azure amd64 ha } make: *** [Makefile:56: verify-scripts] Error 255

Does it mean I need to add more tests ?

JoelSpeed commented 4 weeks ago

@JoelSpeed the test ci/prow/verify is failing with the following error: INSUFFICIENT CI testing for "InstallEIPForDefaultNLBIngressController". F1022 01:20:04.603312 172416 root.go:64] Error running codegen: error: "install should succeed: overall" only passed 94%, need at least 95% for "InstallEIPForDefaultNLBIngressController" on {azure amd64 ha } make: *** [Makefile:56: verify-scripts] Error 255

Does it mean I need to add more tests ?

Have you added any tests to openshift/origin that are gated by the feature gate you're introducing here?

miheer commented 3 weeks ago

here in this API do we represent whether an NLB is int

Actually we don't set scope via installer. So, that is why I did not feel the need to add a field for this. The CIO by default creates a ingress controller with external scope. If a default ingresscontroller is created by the installer with eips the scope will be set to external by default. If someone later tries to change the scope from the operator/v1 ingress i.e Ingress Controller then it won't be allowed.

miheer commented 3 weeks ago

Generally looks good to me. I have a lot of open questions that I have explored in https://docs.google.com/document/d/1Y-Z8gaKYv5dczbdwvXanbzBl-HjZ7zasRC0WaJGBWkA/edit?tab=t.0#heading=h.sya755lm2fbe about the IngressConfig API.

The main one that I hope to get answered soon that affects this PR is "Should we split Ingress Config into defaulting for the default IngressController and defaulting for user-created IngressControllers?"

My concern is that IngressConfig.spec.loadBalancer is growing with new fields that have inconsistent purposes. I'd hope to have a discussion with the API team soon about this. @JoelSpeed Maybe the answer is to keep on the path we are (continue with this PR as is) and look for a path for deprecation or a V2.

Should we split Ingress Config into defaulting for the default IngressController and defaulting for user-created IngressControllers? [Grant] I think our Ingress Config API could benefit from two structures that accomplish two different things Naming is going to be really difficult. How do you distinguish between default for default ICs and default for user-created ICs?. spec.defaultIngressControllerDefaults and spec.userCreatedIngressControllerDefaults? [Joel] We should definitely do this, to make the UX of creating the ingress config much more obvious to the end user, though as mentioned, naming is hard, we can discuss this in a wider forum to come up with something everyone is happy with, ingressControllerDefaults with fields of default and userCreated could be the way, but the default is unfortunate, we’d have to document it well Reference: https://docs.google.com/document/d/1Y-Z8gaKYv5dczbdwvXanbzBl-HjZ7zasRC0WaJGBWkA/edit?tab=t.0#heading=h.1196heeu6pjk| Answer: @JoelSpeed @deads2k @gcs278 Shall I split it ? However this will deprecate the old API so do we handle this ?

miheer commented 3 weeks ago

@JoelSpeed the test ci/prow/verify is failing with the following error: INSUFFICIENT CI testing for "InstallEIPForDefaultNLBIngressController". F1022 01:20:04.603312 172416 root.go:64] Error running codegen: error: "install should succeed: overall" only passed 94%, need at least 95% for "InstallEIPForDefaultNLBIngressController" on {azure amd64 ha } make: *** [Makefile:56: verify-scripts] Error 255 Does it mean I need to add more tests ?

Have you added any tests to openshift/origin that are gated by the feature gate you're introducing here?

No, can you point me exactly where the tests are needed ?

JoelSpeed commented 3 weeks ago

If someone later tries to change the scope from the operator/v1 ingress i.e Ingress Controller then it won't be allowed.

How as an end user, would I change the scope from external to internal?

No, can you point me exactly where the tests are needed ?

Tests are expected in an appropriate location with origin

miheer commented 1 week ago

/close

openshift-ci[bot] commented 1 week ago

@miheer: Closed this PR.

In response to [this](https://github.com/openshift/api/pull/2043#issuecomment-2470479363): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.