Skip to content

Conversation

@wgabor0427
Copy link
Contributor

@wgabor0427 wgabor0427 commented Oct 22, 2025

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Oct 22, 2025

@wgabor0427 wgabor0427 changed the title OSDOCS-16585 created doc for config network policy for oeprand OSDOCS-16585 created doc for config network policy for operand Oct 22, 2025
Copy link

@bharath-b-rh bharath-b-rh left a comment

Choose a reason for hiding this comment

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

LGTM, except for couple of nits.

@siddhibhor-56 Could you please validate the suggestions made.

Comment on lines 30 to 40
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
name: cluster
spec:
networkPolicies:
- name: allow-external-secrets-egress
componentName: CoreController
policyTypes:
- Egress
egress: # Allow all egress traffic

Choose a reason for hiding this comment

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

Suggested change
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
name: cluster
spec:
networkPolicies:
- name: allow-external-secrets-egress
componentName: CoreController
policyTypes:
- Egress
egress: # Allow all egress traffic
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
name: cluster
spec:
controllerConfig:
networkPolicies:
- name: allow-external-secrets-egress
componentName: ExternalSecretsCoreController
egress: # Allow all egress traffic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

$ oc edit externalsecretsconfigs.operator.openshift.io cluster
----

. Set the policy by editing the `networkPolicies` section. The following example shows how to allow egress to {aws-first} endpoints, the Kubernetes API server, and the Domain Name Service (DNS).

Choose a reason for hiding this comment

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

Suggested change
. Set the policy by editing the `networkPolicies` section. The following example shows how to allow egress to {aws-first} endpoints, the Kubernetes API server, and the Domain Name Service (DNS).
. Set the policy by editing the `networkPolicies` section. The following example shows how to allow egress to {aws-first} endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 30 to 49
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
labels:
app.kubernetes.io/name: cluster
app.kubernetes.io/managed-by: external-secrets-operator-e2e
name: cluster
spec:
controllerConfig:
networkPolicies:
- name: allow-external-secrets-egress
componentName: ExternalSecretsCoreController
egress:
# Allow egress to Kubernetes API server, AWS endpoints, and DNS
- to: []
ports:
- protocol: TCP
port: 6443 # Kubernetes API
- protocol: TCP
port: 443 # HTTPS (AWS Secrets Manager)

Choose a reason for hiding this comment

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

Suggested change
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
labels:
app.kubernetes.io/name: cluster
app.kubernetes.io/managed-by: external-secrets-operator-e2e
name: cluster
spec:
controllerConfig:
networkPolicies:
- name: allow-external-secrets-egress
componentName: ExternalSecretsCoreController
egress:
# Allow egress to Kubernetes API server, AWS endpoints, and DNS
- to: []
ports:
- protocol: TCP
port: 6443 # Kubernetes API
- protocol: TCP
port: 443 # HTTPS (AWS Secrets Manager)
apiVersion: operator.openshift.io/v1alpha1
kind: ExternalSecretsConfig
metadata:
labels:
app.kubernetes.io/name: cluster
app.kubernetes.io/managed-by: external-secrets-operator-e2e
name: cluster
spec:
controllerConfig:
networkPolicies:
- componentName: ExternalSecretsCoreController
egress:
- ports:
- port: 443 # HTTPS (AWS Secrets Manager)
protocol: TCP
name: allow-external-secrets-egress

Choose a reason for hiding this comment

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

we need to add DNS port for both TCP and UDP
- protocol: TCP
port: 5353 # DNS
- protocol: UDP
port: 5353 # DNS

Choose a reason for hiding this comment

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

It's now added by default correct, why does user need to include it here?

Choose a reason for hiding this comment

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

oh yes correct,Added by default no need to include

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


componentName:: name for the core controller specified as `ExternalSecretsCoreController`.

Egress rules must include the necessary ports, such as Transmission Control Protocol (TCP) port 6443 for the Kubernetes API and TCP port 443 (HTTPS) for services like the {aws-short} Secrets Manager.

Choose a reason for hiding this comment

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

Suggested change
Egress rules must include the necessary ports, such as Transmission Control Protocol (TCP) port 6443 for the Kubernetes API and TCP port 443 (HTTPS) for services like the {aws-short} Secrets Manager.
Egress rules must include the necessary ports, such as Transmission Control Protocol (TCP) 443 (HTTPS) for services like the {aws-short} Secrets Manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

| `external-secrets`
| 8080
| 6443
| Accesses metrics and communicates with the API server

Choose a reason for hiding this comment

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

@wgabor0427 Could you please help reword the description column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. @bharath-b-rh Reworded the descriptions. Please review and comment on. Thanks.

Comment on lines 33 to 35
labels:
app.kubernetes.io/name: cluster
app.kubernetes.io/managed-by: external-secrets-operator-e2e

Choose a reason for hiding this comment

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

Sorry missed in the previous review. Please remove the labels field.

Suggested change
labels:
app.kubernetes.io/name: cluster
app.kubernetes.io/managed-by: external-secrets-operator-e2e

@wgabor0427 wgabor0427 force-pushed the OSDOCS-16585 branch 4 times, most recently from ddcbfcc to 7eb54d2 Compare October 29, 2025 14:13
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2025
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 29, 2025
@bharath-b-rh
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 29, 2025

@wgabor0427: all tests passed!

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.

@emmajiafan
Copy link

/lgtm

@wgabor0427
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Nov 3, 2025
@stevsmit stevsmit added merge-review-in-progress Signifies that the merge review team is reviewing this PR branch/enterprise-4.20 and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Nov 4, 2025
@stevsmit stevsmit added this to the Continuous Release milestone Nov 4, 2025

* An `ExternalSecretsConfig` must be predefined.

* You must be able to define specific egress rules, including desitination ports and protocols
Copy link
Member

@stevsmit stevsmit Nov 4, 2025

Choose a reason for hiding this comment

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

Suggested change
* You must be able to define specific egress rules, including desitination ports and protocols
* You must be able to define specific egress rules, including destination ports and protocols.


* An `ExternalSecretsConfig` must be predefined.

* You must be able to define specific egress rules, including desitination ports and protocols
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* You must be able to define specific egress rules, including desitination ports and protocols
* You must be able to define specific egress rules, including destination ports and protocols.

- name: allow-external-secrets-egress
----

componentName:: name for the core controller specified as `ExternalSecretsCoreController`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this would technically have "where" about it. See https://redhat-documentation.github.io/supplementary-style-guide/#explain-commands-variables-in-code-blocks. They also usually start with "Specifies. . ."


toc::[]

The {external-secrets-operator} includes pre-defined `NetworkPolicies` for security, but you must configure additonal, custom policies through the `ExternalSecretsConfig` custom resource to set the external-secrets controller egress allow policies to communicate with external providers. These configurable policies are set via the `ExternalSecretsConfig` custom resource to establish the egress allow policy.
Copy link
Member

Choose a reason for hiding this comment

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

First sentence is very long and hard to get through/understand, IMO.

@stevsmit
Copy link
Member

stevsmit commented Nov 4, 2025

Bill is bricked from fixing these and the PR is urgent. I'm merging under the conditions that we fix these in a future PR.

@stevsmit stevsmit added ok-to-merge and removed merge-review-in-progress Signifies that the merge review team is reviewing this PR labels Nov 4, 2025
@stevsmit stevsmit merged commit 3fec8f4 into openshift:main Nov 4, 2025
2 checks passed
@stevsmit
Copy link
Member

stevsmit commented Nov 4, 2025

/cherry-pick enterprise-4.21

@stevsmit
Copy link
Member

stevsmit commented Nov 4, 2025

/cherry-pick enterprise-4.20

@openshift-cherrypick-robot

@stevsmit: new pull request created: #101690

In response to this:

/cherry-pick enterprise-4.21

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.

@openshift-cherrypick-robot

@stevsmit: new pull request created: #101691

In response to this:

/cherry-pick enterprise-4.20

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.

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

Labels

branch/enterprise-4.20 lgtm Indicates that a PR is ready to be merged. ok-to-merge size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants