Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WRKLDS-605: e2e: Config v1 client shim for static configuration manifests with read-only operations #27714

Conversation

ingvagabund
Copy link
Member

@ingvagabund ingvagabund commented Feb 9, 2023

Implement a config client shim that allows to intersect requests from a client and replace requests over static manifests with read-only operation over the manifests. The request replacement is no-op for object outside the of static manifest directory. In case a mutation request is required over static manifests, an error is returned.

The static manifests directory is pointed to through STATIC_CONFIG_MANIFEST_DIR environment variable. Currently only manifests from [infrastructure|network].config.openshift.io/v1 are supported as static manifests.

Exemplary manifests for MicroShift deployment:

apiVersion: "config.openshift.io/v1"
kind: Infrastructure
metadata:
  name: cluster
spec:
  platformSpec:
    type: None
status:
  apiServerURL: ???
  controlPlaneTopology: SingleReplica
  infrastructureTopology: SingleReplica
  platform: None
  platformStatus:
    type: None
apiVersion: config.openshift.io/v1
kind: Network
metadata:
  name: cluster
spec:
  networkType: OVNKubernetes
status:
  networkType: OVNKubernetes

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 9, 2023
@ingvagabund ingvagabund force-pushed the e2e-env-with-config.openshift.io branch 2 times, most recently from c17604f to c8be741 Compare February 9, 2023 15:22
@ingvagabund ingvagabund changed the title wip: e2e: cluster infrastructure getter layer WRKLDS-605: wip: e2e: cluster infrastructure getter layer Feb 9, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 9, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 9, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

infrastructure field environment variable
Status.PlatformStatus.Type PLATFORM_TYPE

TBD
/hold

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/test-infra repository.

@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 9, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

infrastructure field environment variable
Status.PlatformStatus.Type PLATFORM_TYPE
Status.APIServerURL unclear

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 9, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.PlatformStatus.Type PLATFORM_TYPE
infrastructure.config.openshift.io Status.APIServerURL unclear
network.config.openshift.io Status.NetworkType NETWORK_TYPE

TBD
/hold

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/test-infra repository.

@ingvagabund ingvagabund force-pushed the e2e-env-with-config.openshift.io branch 2 times, most recently from 67b4bd9 to aa75bd1 Compare February 9, 2023 19:42
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 11, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.PlatformStatus.Type PLATFORM_TYPE
infrastructure.config.openshift.io Status.APIServerURL unclear
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 13, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.PlatformStatus.Type PLATFORM_TYPE
infrastructure.config.openshift.io Status.APIServerURL unclear
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 13, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.APIServerURL unclear
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 14, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.APIServerURL APISSERVER_URL
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 14, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.APIServerURL APISERVER_URL
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 14, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

Current mapping (TBD):

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.APIServerURL API_SERVER_URL
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@ingvagabund ingvagabund force-pushed the e2e-env-with-config.openshift.io branch from aa75bd1 to 34d7491 Compare February 14, 2023 11:54
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023
@openshift-ci openshift-ci bot added the vendor-update Touching vendor dir or related files label Feb 14, 2023
@ingvagabund ingvagabund force-pushed the e2e-env-with-config.openshift.io branch 2 times, most recently from 84c6ce4 to bd8e3ed Compare February 14, 2023 13:31
@ingvagabund
Copy link
Member Author

ingvagabund commented Feb 14, 2023

We can either have the envs as we have now. Or, create one huge json string passed through a single env variable.
Also, probably better to rename ClusterInfrastructure -> ClusterDetails.
The env naming is still open to debate.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 15, 2023

@ingvagabund: This pull request references WRKLDS-605 which is a valid jira issue.

In response to this:

Abstract from a specific way of getting information about a cluster. Provide two ways of getting e.g. clusterplatform type. Either by reading infrastructure.config.io/cluster object. Or, be reading corresponding environment variables.

In clusters where operators continuously reconcile the cluster state corresponding config.openshift.io/infrastructure, resp. config.openshift.io/network CRs are getting properly updated. Whenever the CR gets updated by a malicious agent, the reconciling operators will make sure the CR are corrected.

Whereas, in clusters without the reconciling operators, anyone can invalidate the CRs and provide invalid cluster state to consuming components. For that, using environment variables to capture the cluster state instead of deploying read-only CRs provides a similar level of protection.

Current mapping:

kind infrastructure field environment variable
infrastructure.config.openshift.io Status.APIServerURL API_SERVER_URL
network.config.openshift.io Status.NetworkType NETWORK_TYPE
infrastructure.config.openshift.io Status.ControlPlaneTopology CONTROL_PLANE_TOPOLOGY
infrastructure.config.openshift.io Status.InfrastructureTopology INFRASTRUCTURE_TOPOLOGY
infrastructure.config.openshift.io Status.PlatformStatus PLATFORM_STATUS

TBD
/hold

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/test-infra repository.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023
@ingvagabund ingvagabund force-pushed the e2e-env-with-config.openshift.io branch from bd8e3ed to 31b76a6 Compare February 15, 2023 16:36
@ingvagabund
Copy link
Member Author

/retest-required

2 similar comments
@ingvagabund
Copy link
Member Author

/retest-required

@ingvagabund
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD a695ea0 and 2 for PR HEAD c568b58 in total

@atiratree
Copy link
Member

failure due to openshift/release#37909 - should be ok now
/retest-required

@ingvagabund
Copy link
Member Author

/retest-required

1 similar comment
@ingvagabund
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 0fea3c9 and 1 for PR HEAD c568b58 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD d64f5ed and 0 for PR HEAD c568b58 in total

@openshift-ci-robot
Copy link

/hold

Revision c568b58 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2023
@ingvagabund
Copy link
Member Author

/hold cancel
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4c4f855 and 2 for PR HEAD c568b58 in total

@ingvagabund
Copy link
Member Author

/retest-required
/hold cancel

@ingvagabund
Copy link
Member Author

/retest-required

1 similar comment
@ingvagabund
Copy link
Member Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 1, 2023

@ingvagabund: 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/e2e-openstack-ovn c568b58 link false /test e2e-openstack-ovn
ci/prow/e2e-metal-ipi-sdn c568b58 link false /test e2e-metal-ipi-sdn
ci/prow/e2e-aws-ovn-single-node c568b58 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-etcd-scaling c568b58 link false /test e2e-aws-ovn-etcd-scaling
ci/prow/e2e-gcp-ovn-etcd-scaling c568b58 link false /test e2e-gcp-ovn-etcd-scaling
ci/prow/e2e-aws-ovn-upgrade c568b58 link false /test e2e-aws-ovn-upgrade
ci/prow/e2e-vsphere-ovn-etcd-scaling c568b58 link false /test e2e-vsphere-ovn-etcd-scaling
ci/prow/e2e-azure-ovn-etcd-scaling c568b58 link false /test e2e-azure-ovn-etcd-scaling

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

@ingvagabund
Copy link
Member Author

/retest-required

@openshift-merge-robot openshift-merge-robot merged commit 4e43d18 into openshift:master Apr 2, 2023
@ingvagabund ingvagabund deleted the e2e-env-with-config.openshift.io branch April 2, 2023 19:48
@ingvagabund
Copy link
Member Author

/cherrypick release-4.13

@openshift-cherrypick-robot

@ingvagabund: #27714 failed to apply on top of branch "release-4.13":

Applying: e2e: inject static config manifests if present
Using index info to reconstruct a base tree...
M	test/extended/util/annotate/generated/zz_generated.annotations.go
M	test/extended/util/annotate/rules.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
Auto-merging test/extended/util/annotate/rules.go
Auto-merging test/extended/util/annotate/generated/zz_generated.annotations.go
CONFLICT (content): Merge conflict in test/extended/util/annotate/generated/zz_generated.annotations.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 e2e: inject static config manifests if present
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.13

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/test-infra repository.

ingvagabund added a commit to ingvagabund/origin that referenced this pull request Apr 2, 2023
…ests with read-only operations (openshift#27714)

* e2e: inject static config manifests if present

* Apply request on the level of individual config v1 kinds

* Add config.openshift.io/v1 Networks client shim

* Collect static manifests only when the manifest directory is not empty

* Make make verify happy

* Address comments

Add support for field selectors for List and Watch requests
Fix naming

* Document expected content of the static manifest directory

* Discovery shim for config.openshift.io/v1

* Check abundance of events, check existence of a real object in a list

* Unit test config v1 shim discovery methods

* make update-gofmt

* Return early without config v1 kinds

* Do not use ginkgo.Expect for checking collectConfigManifestsFromDir error

The ginkgo handler is not yet initialized properly

* Set staticConfigManifestDir for NewCLIWithFramework

* Update annotate rules to exclude [sig-network][Feature:EgressIP][apigroup:operator.openshift.io] [internal-targets]
tjungblu pushed a commit to tjungblu/origin that referenced this pull request Apr 11, 2023
…ests with read-only operations (openshift#27714)

* e2e: inject static config manifests if present

* Apply request on the level of individual config v1 kinds

* Add config.openshift.io/v1 Networks client shim

* Collect static manifests only when the manifest directory is not empty

* Make make verify happy

* Address comments

Add support for field selectors for List and Watch requests
Fix naming

* Document expected content of the static manifest directory

* Discovery shim for config.openshift.io/v1

* Check abundance of events, check existence of a real object in a list

* Unit test config v1 shim discovery methods

* make update-gofmt

* Return early without config v1 kinds

* Do not use ginkgo.Expect for checking collectConfigManifestsFromDir error

The ginkgo handler is not yet initialized properly

* Set staticConfigManifestDir for NewCLIWithFramework

* Update annotate rules to exclude [sig-network][Feature:EgressIP][apigroup:operator.openshift.io] [internal-targets]
tjungblu pushed a commit to tjungblu/origin that referenced this pull request Apr 11, 2023
…ests with read-only operations (openshift#27714)

* e2e: inject static config manifests if present

* Apply request on the level of individual config v1 kinds

* Add config.openshift.io/v1 Networks client shim

* Collect static manifests only when the manifest directory is not empty

* Make make verify happy

* Address comments

Add support for field selectors for List and Watch requests
Fix naming

* Document expected content of the static manifest directory

* Discovery shim for config.openshift.io/v1

* Check abundance of events, check existence of a real object in a list

* Unit test config v1 shim discovery methods

* make update-gofmt

* Return early without config v1 kinds

* Do not use ginkgo.Expect for checking collectConfigManifestsFromDir error

The ginkgo handler is not yet initialized properly

* Set staticConfigManifestDir for NewCLIWithFramework

* Update annotate rules to exclude [sig-network][Feature:EgressIP][apigroup:operator.openshift.io] [internal-targets]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants