Skip to content
Open
4 changes: 3 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,10 @@ bundle-push: ## Push the bundle image.

.PHONY: protoc
PROTOC = $(shell pwd)/bin/proto/bin/protoc
# map Go arch names to the ones used by protobuf release artifacts
PROTOC_ARCH = $(shell go env GOARCH | sed -e 's/amd64/x86_64/' -e 's/arm64/aarch_64/')
protoc: protoc-gen-go protoc-gen-go-grpc ## Download protoc (protocol buffers tool needed for gRPC)
test -f ${PROTOC} || (cd $(shell pwd)/bin/proto && curl -sSLo protoc.zip https://github.com/protocolbuffers/protobuf/releases/download/v3.16.0/protoc-3.16.0-linux-x86_64.zip && unzip protoc.zip && rm protoc.zip)
test -f ${PROTOC} || (cd $(shell pwd)/bin/proto && curl -sSLo protoc.zip https://github.com/protocolbuffers/protobuf/releases/download/v3.16.0/protoc-3.16.0-linux-$(PROTOC_ARCH).zip && unzip -o protoc.zip && rm protoc.zip)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

.PHONY: protoc-gen-go
PROTOC_GEN_GO = $(shell pwd)/bin/proto/bin/protoc-gen-go
Expand Down
9 changes: 9 additions & 0 deletions api/v1alpha1/selfnoderemediationconfig_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,15 @@ type SelfNodeRemediationConfigSpec struct {
// +optional
EndpointHealthCheckUrl string `json:"endpointHealthCheckUrl,omitempty"`

// PreferredAddressTypes is a list of node address types, that self node remediation agents which run on control-plane nodes will use to try to access the kubelet if they can't contact their peers.
// Takes the values accepted on `node.status.addresses.type`, or the special value `"NodeName"`, in which case the node's `metadata.name` will be used.
// Addresses are currently only read on startup, so a pod restart is required if node addresses change.
// This is a part of self diagnostics which will decide whether the node should be remediated or not.
// If empty, it will be equivalent to ["NodeName"] (the previous behaviour).
// +optional
// +kubebuilder:validation:items:Enum=NodeName;Hostname;InternalDNS;ExternalDNS;InternalIP;ExternalIP
PreferredAddressTypes []string `json:"preferredAddressTypes,omitempty"`

// HostPort is used for internal communication between SNR agents.
// +kubebuilder:default:=30001
// +kubebuilder:validation:Minimum=1
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,22 @@ spec:
Valid time units are "ms", "s", "m", "h".
pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$
type: string
preferredAddressTypes:
description: |-
PreferredAddressTypes is a list of node address types, that self node remediation agents which run on control-plane nodes will use to try to access the kubelet if they can't contact their peers.
Takes the values accepted on `node.status.addresses.type`, or the special value `"NodeName"`, in which case the node's `metadata.name` will be used.
This is a part of self diagnostics which will decide whether the node should be remediated or not.
If empty, it will be equivalent to ["NodeName"] (the previous behaviour).
items:
enum:
- NodeName
- Hostname
- InternalDNS
- ExternalDNS
- InternalIP
- ExternalIP
type: string
type: array
safeTimeToAssumeNodeRebootedSeconds:
description: |-
SafeTimeToAssumeNodeRebootedSeconds is the time after which the healthy self node remediation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,22 @@ spec:
Valid time units are "ms", "s", "m", "h".
pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$
type: string
preferredAddressTypes:
description: |-
PreferredAddressTypes is a list of node address types, that self node remediation agents which run on control-plane nodes will use to try to access the kubelet if they can't contact their peers.
Takes the values accepted on `node.status.addresses.type`, or the special value `"NodeName"`, in which case the node's `metadata.name` will be used.
This is a part of self diagnostics which will decide whether the node should be remediated or not.
If empty, it will be equivalent to ["NodeName"] (the previous behaviour).
items:
enum:
- NodeName
- Hostname
- InternalDNS
- ExternalDNS
- InternalIP
- ExternalIP
type: string
type: array
safeTimeToAssumeNodeRebootedSeconds:
description: |-
SafeTimeToAssumeNodeRebootedSeconds is the time after which the healthy self node remediation
Expand Down
4 changes: 3 additions & 1 deletion install/self-node-remediation-deamonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ spec:
value: {{.IsSoftwareRebootEnabled}}
- name: END_POINT_HEALTH_CHECK_URL
value: {{.EndpointHealthCheckUrl}}
- name: PREFERRED_ADDRESS_TYPES
value: {{.PreferredAddressTypes | join ","}}
- name: HOST_PORT
value: "{{.HostPort}}"
- name: MIN_PEERS_FOR_REMEDIATION
Expand Down Expand Up @@ -108,4 +110,4 @@ spec:
effect: "NoSchedule"
- key: "node-role.kubernetes.io/control-plane"
operator: "Exists"
effect: "NoSchedule"
effect: "NoSchedule"
8 changes: 8 additions & 0 deletions internal/controller/selfnoderemediationconfig_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"os"
"strings"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -133,6 +134,10 @@ func (r *SelfNodeRemediationConfigReconciler) SetupWithManager(mgr ctrl.Manager)
Complete(r)
}

func join(sep string, s []string) string {
return strings.Join(s, sep)
}

func (r *SelfNodeRemediationConfigReconciler) syncConfigDaemonSet(ctx context.Context, snrConfig *selfnoderemediationv1alpha1.SelfNodeRemediationConfig) error {
logger := r.Log.WithName("syncConfigDaemonset")
logger.Info("Start to sync config daemonset")
Expand All @@ -155,10 +160,13 @@ func (r *SelfNodeRemediationConfigReconciler) syncConfigDaemonSet(ctx context.Co
data.Data["PeerRequestTimeout"] = snrConfig.Spec.PeerRequestTimeout.Nanoseconds()
data.Data["MaxApiErrorThreshold"] = snrConfig.Spec.MaxApiErrorThreshold
data.Data["EndpointHealthCheckUrl"] = snrConfig.Spec.EndpointHealthCheckUrl
data.Data["PreferredAddressTypes"] = snrConfig.Spec.PreferredAddressTypes
data.Data["MinPeersForRemediation"] = snrConfig.Spec.MinPeersForRemediation
data.Data["HostPort"] = snrConfig.Spec.HostPort
data.Data["IsSoftwareRebootEnabled"] = fmt.Sprintf("\"%t\"", snrConfig.Spec.IsSoftwareRebootEnabled)

data.Funcs["join"] = join

objs, err := render.Dir(r.InstallFileFolder, &data)
if err != nil {
logger.Error(err, "Fail to render config daemon manifests")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ var _ = Describe("SNR Config Test", func() {
Expect(container.Image).To(Equal(shared.DsDummyImageName))
envVars := getEnvVarMap(container.Env)
Expect(envVars["WATCHDOG_PATH"].Value).To(Equal(config.Spec.WatchdogFilePath))
Expect(envVars["PREFERRED_ADDRESS_TYPES"].Value).To(Equal(""))

Expect(len(ds.OwnerReferences)).To(Equal(1))
Expect(ds.OwnerReferences[0].Name).To(Equal(config.Name))
Expand All @@ -123,6 +124,23 @@ var _ = Describe("SNR Config Test", func() {
Expect(container.SecurityContext.Privileged).To(Equal(pointer.Bool(true)))
Expect(container.SecurityContext.ReadOnlyRootFilesystem).To(Equal(pointer.Bool(true)))
})
When("Configuration has customized address types", func() {
BeforeEach(func() {
config.Spec.PreferredAddressTypes = []string{"InternalDNS", "InternalIP"}
})
It("Daemonset should have comma-separated address types in env var", func() {
Eventually(func(g Gomega) {
ds = &appsv1.DaemonSet{}
g.Expect(k8sClient.Get(context.Background(), dsKey, ds)).Should(BeNil())

dsContainers := ds.Spec.Template.Spec.Containers
g.Expect(len(dsContainers)).To(BeNumerically("==", 1))
container := dsContainers[0]
envVars := getEnvVarMap(container.Env)
g.Expect(envVars["PREFERRED_ADDRESS_TYPES"].Value).To(Equal("InternalDNS,InternalIP"))
}, 10*time.Second, 250*time.Millisecond).Should(Succeed())
})
})
When("Configuration has customized tolerations", func() {
var expectedToleration corev1.Toleration
BeforeEach(func() {
Expand Down
50 changes: 45 additions & 5 deletions internal/controlplane/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"crypto/tls"
"errors"
"fmt"
"net"
"net/http"
"os"
"strings"
"time"

"github.com/go-logr/logr"
Expand All @@ -22,13 +24,16 @@ import (
)

const (
kubeletPort = "10250"
defaultKubeletPort = "10250"
)

// Manager contains logic and info needed to fence and remediate controlplane nodes
type Manager struct {
nodeName string
nodeRole peers.Role
preferredAddressTypes []string
nodeAddresses []corev1.NodeAddress
kubeletPort string
endpointHealthCheckUrl string
wasEndpointAccessibleAtStart bool
client client.Client
Expand All @@ -37,9 +42,22 @@ type Manager struct {

// NewManager inits a new Manager return nil if init fails
func NewManager(nodeName string, myClient client.Client) *Manager {
var preferredAddressTypes []string
rawPreferredAddressTypes := os.Getenv("PREFERRED_ADDRESS_TYPES")
if rawPreferredAddressTypes != "" {
preferredAddressTypes = strings.Split(rawPreferredAddressTypes, ",")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: strings.Split doesn't trim whitespace. If someone manually patches the pod env var with "InternalIP, NodeName" (space after comma), " NodeName" won't match "NodeName".

Via the normal CRD→template path this can't happen (the join template produces no spaces), but for defensive robustness:

for i, t := range preferredAddressTypes {
    preferredAddressTypes[i] = strings.TrimSpace(t)
}

} else {
preferredAddressTypes = []string{"NodeName"}
}
for i, addressType := range preferredAddressTypes {
preferredAddressTypes[i] = strings.TrimSpace(addressType)
}

return &Manager{
nodeName: nodeName,
endpointHealthCheckUrl: os.Getenv("END_POINT_HEALTH_CHECK_URL"),
preferredAddressTypes: preferredAddressTypes,
kubeletPort: defaultKubeletPort,
client: myClient,
wasEndpointAccessibleAtStart: false,
log: ctrl.Log.WithName("controlPlane").WithName("Manager"),
Expand Down Expand Up @@ -125,6 +143,7 @@ func (manager *Manager) initializeManager() error {
return wrapWithInitError(err)
}
manager.setNodeRole(node)
manager.nodeAddresses = node.Status.Addresses

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

note: nodeAddresses is cached at init and never refreshed. If node addresses change during the pod's lifetime (IP rotation, secondary NIC), the kubelet check uses stale data.

In practice, this is fine as node addresses rarely change, and the SNR daemonset restarts on config changes. But it's worth documenting this in the CRD field description so users know a pod restart is needed to pick up address changes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've added a note to the CRD field. It would be nice to automatically update these, but agree that this is rare and probably not worth the complexity until someone complains about it (especially for control-plane nodes, which is the only place this matters, because AFAIK etcd requires stable addresses for peers).


manager.wasEndpointAccessibleAtStart = manager.isEndpointAccessible()
return nil
Expand Down Expand Up @@ -166,24 +185,45 @@ func (manager *Manager) isEndpointAccessible() bool {
}

func (manager *Manager) isKubeletServiceRunning() bool {
url := fmt.Sprintf("https://%s:%s/pods", manager.nodeName, kubeletPort)
for _, addressType := range manager.preferredAddressTypes {
if addressType == "NodeName" {
if manager.isKubeletServiceRunningOnAddress(manager.nodeName) {
return true
}
} else {
nodeAddressType := corev1.NodeAddressType(addressType)
for _, address := range manager.nodeAddresses {
if address.Type == nodeAddressType {
if manager.isKubeletServiceRunningOnAddress(address.Address) {
return true
}
}
}
}
}

return false
}

func (manager *Manager) isKubeletServiceRunningOnAddress(address string) bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pre-existing / follow-up: The http.Client created inside this function (line 213) has no Timeout, and the request uses no context.Context with deadline. If a target address accepts TCP but never responds TLS, this blocks indefinitely — and with the new multi-address iteration, a single hanging connection also blocks fallback to later address types.

Not a blocker for this PR (the no-timeout client is pre-existing on main), but worth a follow-up:

httpClient := &http.Client{
    Transport: tr,
    Timeout:   10 * time.Second,
}

url := fmt.Sprintf("https://%s/pods", net.JoinHostPort(address, manager.kubeletPort))
tr := &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
MinVersion: certificates.TLSMinVersion,
},
}
httpClient := &http.Client{Transport: tr}
httpClient := &http.Client{Transport: tr, Timeout: 10 * time.Second}

req, err := http.NewRequest("GET", url, nil)
if err != nil {
manager.log.Error(err, "failed to create a kubelet service request", "node name", manager.nodeName)
manager.log.Error(err, "failed to create a kubelet service request", "address", address)
return false
}

resp, err := httpClient.Do(req)
if err != nil {
manager.log.Error(err, "kubelet service is down", "node name", manager.nodeName)
manager.log.Error(err, "kubelet service is down", "address", address)
return false
}
defer resp.Body.Close()
Expand Down
Loading
Loading